Conversation
|
Ready for review:
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. |
|
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: |
| '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], \ |
There was a problem hiding this comment.
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.
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.
"Standard" set of builds:
Set of builds with only Connext:
|
|
"Standard" set of builds:
Set of builds with only Connext:
|
02aa9e6 to
6203ad0
Compare
|
"Standard" set of builds:
Set of builds with only Connext:
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.
| errors.append("Package name '%s' does not follow naming " | ||
| 'conventions' % self.name) | ||
|
|
||
| version_regexp = '^[0-9]+\.[0-9_]+\.[0-9_]+$' |
There was a problem hiding this comment.
Should this be exposed as a constant for sharing with consuming code? So it doesn't necessarily need to be.
There was a problem hiding this comment.
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.
Based on the current draft of REP 149.