add support for package format 3 #168
Conversation
ament_tools/topological_order.py
Outdated
| for slot in ( | ||
| 'build_depends', 'build_export_depends', 'exec_depends' | ||
| ): | ||
| getattr(pkg, slot).append(copy.deepcopy(d)) |
There was a problem hiding this comment.
This seems a bit fragile, since there's no way to tell if a Package object has had its groups expanded or not.
Is this modifying the packages input data structure's elements (hard to tell with the redirection via the _PackageDecorator)?
If it is, I'd feel better about this change if it did not. Maybe a test to assert dependencies of the input Package objects in packages are not changed after being given to this function.
That way we could assert that sneaking these dependencies into the existing slots of the Package object is contained to this function and the functions it calls and they don't leak up out fo the function.
Alternatively, it would be more explicit, but more intrusive, to instead additionally check the group depends anywhere the depend tags are checked, rather than expanding the group depends into the existing slots.
There was a problem hiding this comment.
This is intentionally modifying the Package instances. So the side effect of the function is desired since that makes all other uses of the dependency transparent.
Expanding group dependencies later is with the current API currently not possible. For example when building one package the context of the other packages doesn't exist at the moment and a group dependency can't be resolved without these.
There was a problem hiding this comment.
This is intentionally modifying the Package instances. So the side effect of the function is desired since that makes all other uses of the dependency transparent.
That's even worse imo, because now code later may use the depends members of the Package class and based on the documentation (and the code in ament_package) about that object would conclude that the group dependencies are not part of them. This will be really hard to understand later when trying to figure out where dependencies in a Package object are coming from.
At the very least, it is not clear from comments, the code itself, or any documentation of this function that it modifies the input parameters in this way.
Expanding group dependencies later is with the current API currently not possible. For example when building one package the context of the other packages doesn't exist at the moment and a group dependency can't be resolved without these.
I would recommend that you instead expand the group dependencies into separate storage (either in new members of the Package object or as a separate object that is passed along with the Package object), then the code that needs to consider the group dependencies can check them separately from the normal dependencies, and code that doesn't want to consider the group dependencies does not have to change what it is doing.
Also, other tools will need to consider the group dependencies at some point, so maybe the logic to expand a group dependency into actual dependencies should live in the ament_package repository as a function that returns the expanded dependencies and takes the name of the group and the necessary context to expand them into dependencies. It could also be a method of the Package object which stores the result in the Package object and then you could have functions that return the combined normal + expanded group dependencies. Then you could update each place that needs to consider them.
That would allow this logic here to be reused and it would be more obvious when reading code that uses the Package object whether or not the group dependencies are being considered or not.
There was a problem hiding this comment.
I added a GroupDependency class which can store its members. A function can populate the members which requires a list of Packages to be passed. This needs to happen for the topological order so that part still mutates the passed in packages (https://github.com/ament/ament_tools/pull/168/files#diff-6d0e8f72766cf2fab285d79a89594f8fR188). Other code like the list_dependencies verb has to do the same.
I didn't run any tests or checked if any other code (beside list_dependencies and topological_order) needs to be modified to consider group dependencies. I first want to wait for feedback if this approach is acceptable.
There was a problem hiding this comment.
That's more understandable to me, and since it is documented that should be fine. 👍, thanks for iterating on it.
| @@ -47,6 +47,11 @@ def prepare_arguments(parser): | |||
| help='Show only test dependencies of a given package', | |||
| ) | |||
| parser.add_argument( | |||
| '--group-deps', | |||
| action='store_true', | |||
| help='Show only group dependencies of a given package', | |||
There was a problem hiding this comment.
This option is good, but if I interpret the usage of it correctly, it only shows you that a given package has a group dependency and the name of the group. It does not tell you which packages in the workspace are considered a member of that group.
So, it would be good to either expand the group dependency and display them as part of this option or have an option like list --group-of <group name>.
There was a problem hiding this comment.
Makes sense. dc26ceb lists all member of the group.
Please re-review with recent changes.
|
Waiting for review. |
|
I'll review this today |
|
Sorry, I don't think I got a message when the review was cleared. I can review it @mikaelarguedas, since I did the initial review. |
|
@wjwwood sounds good to me |
Connect to ament/ament_package#63