New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for package format 3 #63

Merged
merged 2 commits into from Nov 14, 2017

Conversation

Projects
None yet
4 participants
@dirk-thomas
Member

dirk-thomas commented Oct 27, 2017

Based on the current draft of REP 149.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Oct 31, 2017

Member

Ready for review:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

I would propose to get this merged in order to use the newly specified feature and gain experience with it. If the REP changes until ratification the implementation can be updated in order to keep it in sync.

Member

dirk-thomas commented Oct 31, 2017

Ready for review:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

I would propose to get this merged in order to use the newly specified feature and gain experience with it. If the REP changes until ratification the implementation can be updated in order to keep it in sync.

@mikaelarguedas

This comment has been minimized.

Show comment
Hide comment
@mikaelarguedas

mikaelarguedas Oct 31, 2017

Contributor

Is it possible to run the jobs with the tests enable to make sure nothing weird happens in the process?

I tried to run a job with connext only (unchecked the box "use_fastrtps") and the job failed. Is that expected?

With this set of changes: Build Status
Without: Build Status

Contributor

mikaelarguedas commented Oct 31, 2017

Is it possible to run the jobs with the tests enable to make sure nothing weird happens in the process?

I tried to run a job with connext only (unchecked the box "use_fastrtps") and the job failed. Is that expected?

With this set of changes: Build Status
Without: Build Status

@@ -123,7 +123,7 @@ def parse_package_string(data, *, filename=None):
"Unable to handle '%s' format version '%d', please update the " \
'manifest file to at least format version 2' % \
(filename, pkg.package_format)
assert pkg.package_format in [2], \
assert pkg.package_format in [2, 3], \

This comment has been minimized.

@wjwwood

wjwwood Oct 31, 2017

Contributor

It would be nice to have some kind of test for this function (parse_package_string()), and have it updated for the new features in package.xml version 3. I guess those didn't make it over from catkin_pkg.

@wjwwood

wjwwood Oct 31, 2017

Contributor

It would be nice to have some kind of test for this function (parse_package_string()), and have it updated for the new features in package.xml version 3. I guess those didn't make it over from catkin_pkg.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 1, 2017

Member

I tried to run a job with connext only (unchecked the box "use_fastrtps") and the job failed. Is that expected?

Certainly not. Looking into the problem showed that there are more locations using the dependency members directly. Instead of updating the all code paths to be aware of group dependencies I decided to refactor the logic in ament_tools so that the group dependencies are being mapped into "direct" dependencies. So any other "downstream" code doesn't need to know about group dependencies.

Is it possible to run the jobs with the tests enable to make sure nothing weird happens in the process?

"Standard" set of builds:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Set of builds with only Connext:

  • Linux Build Status
  • macOS Build Status
  • Windows Build Status
Member

dirk-thomas commented Nov 1, 2017

I tried to run a job with connext only (unchecked the box "use_fastrtps") and the job failed. Is that expected?

Certainly not. Looking into the problem showed that there are more locations using the dependency members directly. Instead of updating the all code paths to be aware of group dependencies I decided to refactor the logic in ament_tools so that the group dependencies are being mapped into "direct" dependencies. So any other "downstream" code doesn't need to know about group dependencies.

Is it possible to run the jobs with the tests enable to make sure nothing weird happens in the process?

"Standard" set of builds:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Set of builds with only Connext:

  • Linux Build Status
  • macOS Build Status
  • Windows Build Status
@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 3, 2017

Member

"Standard" set of builds:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Set of builds with only Connext:

  • Linux Build Status
  • macOS Build Status
  • Windows Build Status
Member

dirk-thomas commented Nov 3, 2017

"Standard" set of builds:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Set of builds with only Connext:

  • Linux Build Status
  • macOS Build Status
  • Windows Build Status

@dirk-thomas dirk-thomas referenced this pull request Nov 7, 2017

Merged

install pyparsing #108

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 8, 2017

Member

"Standard" set of builds:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Set of builds with only Connext:

  • Linux Build Status
  • macOS Build Status
  • Windows Build Status

Please review the new PR and take another look at this patch as well as the one on ament_tools since they have changed quite a bit.

Member

dirk-thomas commented Nov 8, 2017

"Standard" set of builds:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Set of builds with only Connext:

  • Linux Build Status
  • macOS Build Status
  • Windows Build Status

Please review the new PR and take another look at this patch as well as the one on ament_tools since they have changed quite a bit.

Please re-review with recent changes.

@wjwwood

wjwwood approved these changes Nov 9, 2017

@@ -124,11 +153,17 @@ def validate(self):
errors.append("Package name '%s' does not follow naming "
'conventions' % self.name)
version_regexp = '^[0-9]+\.[0-9_]+\.[0-9_]+$'

This comment has been minimized.

@nuclearsandwich

nuclearsandwich Nov 10, 2017

Contributor

Should this be exposed as a constant for sharing with consuming code? So it doesn't necessarily need to be.

@nuclearsandwich

nuclearsandwich Nov 10, 2017

Contributor

Should this be exposed as a constant for sharing with consuming code? So it doesn't necessarily need to be.

This comment has been minimized.

@dirk-thomas

dirk-thomas Nov 10, 2017

Member

It was internal before this patch so I won't consider it part of this PR in order to keep the diff as small as possible.

@dirk-thomas

dirk-thomas Nov 10, 2017

Member

It was internal before this patch so I won't consider it part of this PR in order to keep the diff as small as possible.

@dirk-thomas dirk-thomas merged commit 26d2547 into master Nov 14, 2017

@dirk-thomas dirk-thomas deleted the rep149 branch Nov 14, 2017

@dirk-thomas dirk-thomas removed the in review label Nov 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment