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.

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.

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.

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.

That's more understandable to me, and since it is documented that should be fine. 👍, thanks for iterating on it.

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>.

Makes sense. dc26ceb lists all member of the group.

That looks good to me, 👍.