Selection of dependency types with cmd line option [#727 continuation]#789
Selection of dependency types with cmd line option [#727 continuation]#789nuclearsandwich merged 18 commits intoros-infrastructure:masterfrom mateus-amarante:select-dep-type
Conversation
…dep_type_catkin_package
|
@nuclearsandwich friendly ping. This is the realization of #727 (comment) |
|
Should #727 be closed? |
src/rosdep2/main.py
Outdated
| 'If specified end-of-life distros are being ' | ||
| 'fetched too.') | ||
| parser.add_option('-t', '--dependency-types', dest='dependency_types', | ||
| type="choice", choices=("build", "buildtool", "build_export", "exec", "run", "test", "doc"), |
There was a problem hiding this comment.
should this be nargs='+' - which should let you omit the default=[] because it will be an empty list when not specified. I don't think that type="choice" is correct - shouldn't it be type=str (which I think is the default)?
Additionally (just thinking out loud) - is there any place that we could query these choices from rather than hardcoding them here? May be better to have a single source of truth for the types of dependencies that are allowed, and if it ever changes this code wouldn't rot.
There was a problem hiding this comment.
You are right. It is better to have these names defined in a single place. Do you have a suggestion on where they should live?
There was a problem hiding this comment.
The source of truth seems to be catking_pkg.package.Package (https://github.com/ros-infrastructure/catkin_pkg/blob/master/src/catkin_pkg/package.py#L53) - which has the attribute __slots__, that might be the best place to parse these values from, though it may not be best practice to go grabbing dunder variables. I'm not super experienced with the source of this catkin infra, input from a maintainer would be helpful on the best approach.
src/rosdep2/rospkg_loader.py
Outdated
| else: | ||
| return dep_type in ['buildtool', 'build', 'build_export', 'exec', 'test'] | ||
|
|
||
| self.include_build_depends = check_dep('build') |
There was a problem hiding this comment.
this could probably be
# these should probably be constants defined in one place and used across this PR
all_dep_types = {'buildtool', 'build', 'build_export', 'exec', 'test', 'doc'}
default_dep_types = all_dep_types - {'doc'}
self.include_dep_types = all_dep_types.intersection(set(dependency_types)) if dependency_types else default_dep_types
then below in get_rosdeps we could use a generic iteration as well
deps = sum((getattr(pkg, f'{d}_depends') for d in self.include_dep_types), [])
In fact, after writing the above, I see that one way to get all_dep_types as above could be to introspect the pkg type object with something like
all_dep_types = [x for x in dir(pkg) if x.endswith('_depends')]
There was a problem hiding this comment.
I was unhappy with this floating function... Very nice suggestions! I'll consider applying them.
There was a problem hiding this comment.
Implemented this suggestion in fa3fd33. I still didn't find a good format for all_dep_types. For now, it is a function in rospkg_load.py.
Yes. This is a replacement. |
Codecov Report
@@ Coverage Diff @@
## master #789 +/- ##
==========================================
+ Coverage 74.93% 74.98% +0.04%
==========================================
Files 42 42
Lines 3280 3286 +6
==========================================
+ Hits 2458 2464 +6
Misses 822 822
Continue to review full report at Codecov.
|
|
ping @nuclearsandwich i'd love to have this feature (and i think the implementation is good now - and #727 can be closed in favor of this PR |
130s
left a comment
130s
left a comment
There was a problem hiding this comment.
I came from answers.ros.org#q370946. Great feature!
test/test_rosdep_rospkg_loader.py
Outdated
|
|
||
| def test_rospkg_loader_all_dep_types(): | ||
| from rosdep2.rospkg_loader import all_dep_types | ||
| assert all_dep_types() == {'build', 'buildtool', 'build_export', 'buildtool_export', 'exec', 'test', 'doc'}, all_dep_types() |
There was a problem hiding this comment.
This is a test file so fixed values may make more sense, but I still want to raise this thoughts: Should the types be taken from the upstream catkin_pkg.package.Package like done in the source?
There was a problem hiding this comment.
I think this strategy isn't adequate because it would use the same principle the function being tested uses as if I was doing asssert all_dep_types() == all_dep_types(), which would be always true. Imagine if a new dependency type is added to Package and we don't want to consider it just yet. The tests would pass without our intention.
On the other hand, you might argue that this list could be constant in the source code and the test could check the list against all the dep types in Package. I think this is valid too and maybe it is in better agreement with my dissatisfaction with this function. I think the maintainers could help us with this decision.
There was a problem hiding this comment.
On the other hand, you might argue that this list could be constant in the source code and the test could check the list against all the dep types in
Package.
+1 for that (I'm not a maintainer).
| @@ -0,0 +1,19 @@ | |||
| <package format="2"> | |||
There was a problem hiding this comment.
| <package format="2"> | |
| <package format="3"> |
Maybe nit, and open to counter suggestion; my personal preference is new development to happen in format 3.
There was a problem hiding this comment.
All the dependency types considered in the new feature were introduced in format 2. It is the minimum format required for this feature. Format 3 is compatible with it so it should work too (we could change it without breaking the tests). However, it is important to highlight that I'm ignoring the new dependency type group_depend, introduced in format 3. I was not sure how I would handle it, so I excluded it from this update.
If the current behavior is considered adequate for format 3 given the group_depend ignoring, I'm ok with changing it.
There was a problem hiding this comment.
I think it makes sense to leave group dependencies out of this feature for the time being since the current implementation only has enough information to evaluate them in a source workspace context and it's not clear how rosdep should resolve them generally.
For manifest format I think the common convention is to use the earliest format which contains the features that are needed for the package and for the sake of making sure we maintain format 2 support I think that makes sense for these tests if not for new packages in general.
|
@nuclearsandwich: what would it take to get this in? More reviews? More testing? is there anything else that needs attention here? |
wjwwood
left a comment
wjwwood
left a comment
There was a problem hiding this comment.
I think I need to wait for another maintainer to have a look, or maybe test it. But I had a glance at it, hopefully that helps them merge with confidence.
cottsay
left a comment
cottsay
left a comment
There was a problem hiding this comment.
I like the feature that this PR implements, but I'm not sold on the "default types" enumeration introduced by fa3fd33 for a couple of reasons:
- I don't consider the use of
__slots__to be part of catkin_pkg's API, but rather an implementation optimization. - It could lead to an unexpected change in behavior if additional dependency types are introduced in catkin_pkg. It's worth a separate discussion.
It's not that I think it's the wrong thing to do, but it's really a separate change from what this PR is trying to implement. I'd like to see it moved to a separate PR so that we can move forward with getting this one merged.
|
@cottsay Would it be possible to have some feedback on this? It would be a really great addition to rosdep. Thank you. |
|
@cottsay @dirk-thomas @wjwwood friendly reminder :) |
|
Seems reasonable to me, but up to the current rosdep maintainers. |
|
@wjwwood can you remind us who the current maintainers are? There is no package.xml for rosdep - I was under the impression that is was you, @tfoote and @nuclearsandwich . This one's been in progress for about 18 months (if you include the original PR that this extends) - who has the final say? It sounds like everybody agrees that it looks fine. |
Who are the maintainers? @tfoote and Ken Conley are listed as authors, but there is no maintainer specified for Looks like @nuclearsandwich and @cottsay have released newer versions in the recent past? As others have mentioned, this PR seems to be the last piece needed to be able to start working towards (nicer) multi-stage Docker builds/images. For that use-case alone it would be very nice to get this in. What's keeping this from getting merged right now? @mateus-amarante seems to have made every requested change and there are multiple positive reviews. |
|
Friendly message to tell we are also waiting for this feature in the multi-stage Docker build context. |
nuclearsandwich
left a comment
nuclearsandwich
left a comment
There was a problem hiding this comment.
After reviewing this morning I have one comment and after talking it through offline with @cottsay I'm going to implement my own suggestion and have him do a review of the result. Then we'll be in mergeable shape.
There's a lingering question / concern that both of us have about whether catkin-pkg or rosdep should "own" the list of possible dependency types but as of now catkin-pkg doesn't have an explicit API for them and we agree that maintaining a list in rosdep is going to carry us forward for now.
I think a big indicator that reorganization will be required is if the list of dependencies starts getting used in other projects it will make sense to expose it directly in catkin-pkg.
| @@ -0,0 +1,19 @@ | |||
| <package format="2"> | |||
There was a problem hiding this comment.
I think it makes sense to leave group dependencies out of this feature for the time being since the current implementation only has enough information to evaluate them in a source workspace context and it's not clear how rosdep should resolve them generally.
For manifest format I think the common convention is to use the earliest format which contains the features that are needed for the package and for the sake of making sure we maintain format 2 support I think that makes sense for these tests if not for new packages in general.
Constants are used in rosdep and catkin_pkg for these kinds of data even though there is a risk that they can be modified. In addition to converting from a function to a constant I also relocated the constant definition to the catkin_packages module in rosdep in order to keep it closer to catkin_pkg related content.
|
🎉 👍 thanks @nalt and @mateus-amarante (and @nuclearsandwich for getting it in) |
@gavanderhoorn , I mocked up a change to the multistage example in the ros docs for the docker hub library image in order to take advantage of this new feature in rosdep. However when simply building and installing the talker listener demo, the final image size savings are not yet compelling: ruffsl/docs-1#1 Do you have a ROS example use case that could better showcase or demonstrate the space savings of only installing exec dependencies' in a final runner multistage image layer? |
13 participants
This is a refactor of #727 update (started by @nalt) which includes adding
--dependency-types/-tcmd line option to select the dependency types for commands likeinstall,keyandcheck. It supports specification of "build", "buildtool", "build_export", "run", "test", "exec" and "doc" types. This PR also adds related tests.