member of rosidl_interfaces_packages group

[mikaelarguedas]

Follow-up of ament/ament_tools#168.
Opening this to gather feedback as of how we should call the group containing ros IDL files.
This PR uses interface_packages but I'm open to alternatives

[mikaelarguedas]

@ros2/team Waiting for feedback

[wjwwood]

In principle, lgtm

Just curious about the name.

[wjwwood]

Is this name set in a document somewhere? Or are we coming up with it for the first time here? If it's not too hard to change yet, I'd slightly prefer rosidl_interface_packages, since there might be other kinds of interfaces in the future that might want to make use of the group mechanism.

[clalancette]

I'm fine with either interface_packages or rosidl_interface_packages as William suggests.

[mikaelarguedas]

this name is not used or documented anywhere yet, the goal of this PR was to get feedback on what name we think makes most sense before starting using it.
In ROS 1 we abuse the term "message package" as sometimes we refer to packages containing services or actions definitions, that's why I used interfaces instead.
One thing to keep in mind is that ROS 1 packages migrating to format 3 may want to use something similar. The resulting question is "should the group name be the same in ROS 1 and ROS 2?" If no I agree that rosidl_interface_packages would be better than interface_packages , if yes we need a name that is generic enough to make sense for both ROS 1 and ROS 2 (that could be rosidl_interface_packages or something else, not sure yet)

[wjwwood]

Another question is, do we want/need different groups for packages which have messages, services, and actions?

[mikaelarguedas]

Indeed that's another good question.
If the answer is yes, this would also raise a few more points:

• our tools should be able to support overlapping groups / duplicated dependencies (already the case I think)
• should we support metagroups? e.g. interface_packages group depending on groups message_packages + services_packages + action_packages

[wjwwood]

As for a name that's appropriate for both ROS 1 and ROS 2, first I think rosidl and interface are generic enough for both, though you're right that in ROS 1 it was just as common to call them just "messages" or associate them with the "genmsg". Second, I believe the groups can use conditionals as well, so even if they needed to be different in ROS 1 and ROS 2 with the same package.xml then you could choose which group to use in each case.

[mikaelarguedas]

Second, I believe the groups can use conditionals as well, so even if they needed to be different in ROS 1 and ROS 2 with the same package.xml then you could choose which group to use in each case.

I was mostly wondering if we think they should be different or not. If they end up being different we can use conditions 👍

[sloretz]

What's the benefit of having different groups for messages, services, and actions? Is it planned to do code generation for them in separate packages?

The interface_packages groups seems special since it will have so many members in core packages. I think it would be a good idea to adopt a prefix to names like this. It avoids name collisions with third-party groups. Also, googling ros_interface_packages versus interface_packages might be more likely to return a search result to a page describing what the group is.

[mikaelarguedas]

@tfoote @dhood @sloretz @dirk-thomas would you like to pitch in here?

[mikaelarguedas]

the conclusion here was that there is no need or simple way to manage the granularity of the various groups at the moment and we don't want to impose to users to define membership to several groups of various granularity so we will create a single group for all the interfaces (messages, services, to-be actions).
Additionally it would be valuable to enforce in CMake that packages invoking rosidl_generate_interfaces have this member_of_group in their package.xml

Regarding the name, rosidl_interfaces_packages is explicit enough and is not restricted to ROS 2. Though we could use a different name in ROS 1 if we need and use the conditions provided by format 3 to express the different group membership in ROS1 and ROS2.

[mikaelarguedas]

I will do a follow up PR to add the check in rosidl_cmake, I think that check should require the member_of_group only for packages that install their generated interfaces. That's why I didnt apply this change to packages that generate interfaces for their tests and don't install them.

[wjwwood]

I think that check should require the member_of_group only for packages that install their generated interfaces.

👍 hopefully that's not too hard to do, let me know if I can help you with it.

[mikaelarguedas]

packaging job with these changes (though the bridge being built after everything else it doesnt prove that this change is taken into account properly)

ready for review

[mikaelarguedas]

hopefully that's not too hard to do, let me know if I can help you with it.

Thanks! I'll give it a shot later this week and let you know how it goes

changes from discussion have been applied and new review is required

[wjwwood]

After discussing in person with @mikaelarguedas and @dhood I think we concluded that rosidl_interface_packages sounds better than rosidl_interfaces_packages.

[clalancette]

After discussing in person with @mikaelarguedas and @dhood I think we concluded that rosidl_interface_packages sounds better than rosidl_interfaces_packages.

Fine by me. I'll hold off the reviews, then.

[mikaelarguedas]

Sounds good, making the change now and will tag the team here when ready for reviews

[mikaelarguedas]

@ros2/team group renamed, back in review then

[wjwwood]

Just re-reviewed them all, +1 to all of them.

[dirk-thomas]

Every group dependency expands to a set of packages. So each group name could potentially end with _packages (e.g. rmw_implementation_packages, foo_packages, etc.). Therefore this suffix is in my opinion not helpful and redundant. I would suggest to use the name rosidl_interfaces instead.

[mikaelarguedas]

This came from the fact that we wanted to remove potential confusion down the road. If we decide to have a different group for messages, services and actions they would be part of that "messages" and that seemed weird to us so we decided to have packages in the name. The same way in ROS 1 people refer to interface packages as "message packages" and again it would be weird to be part of "messages".

I'm not against revisiting as I see valid arguments for each. Using rosidl_interfaces would be shorter, match the name of the ament_index resource etc.
I opened this 3 weeks ago hoping that we would converge before the release though, it may be too late to reopen the discussion and get unanimity from the team so late in the freezing process...

[dirk-thomas]

@ros2/team Please comment if you agree or disagree to switch from rosidl_interface_packages to rosidl_interfaces. If we do get sufficient positive responses we can consider applying the change. Otherwise it can be done after the release (which implies breaking the naming scheme in a future release).

[nuclearsandwich]

👍 rosidl_interfaces

[mikaelarguedas]

+0, I don't feel very strongly one way or the other but would like to see both arguments being explored before asking for approval.

I think rosidl_interfaces makes more sense if we want to be consistent with the ament index resource and how to name other groups. From my perspective, we commonly say "rmw_implementation" or a "message generator" (in both case we refer to a ROS package without ambiguity) so the need of a suffix for those groups doesnt make sense as there is no potential ambiguity. OTOH we say a "message package" (we refer to a ROS package) that is different from a "message" (an IDL definition). So I think message, services and actions are the only potential groups that deserve a _packages suffix as we and the community commonly refer to them using the suffix "package" to disambiguate.

so +0 for me

[wjwwood]

I think rosidl_interface or rosidl_interfaces is fine, but I also think that rosidl_interface_packages (while somewhat redundant) is not harmful or incorrect.

+0

[dhood]

rmw implementation packages only have one rmw implementation in them, while interface packages typically have multiple interfaces, and it's the package that you want to depend on as opposed to the interface itself. That was my understanding for why there's a distinction: there isn't an interface called "std_msgs" for example. @mikaelarguedas pointed out offline that any dependencies in the package.xml are implicitly packages so I'm sure users could still handle it if we removed the _packages.

+0

[clalancette]

+0

[dirk-thomas]

We discussed potential other group names and based on the names we though are most reasonable for ros2/rosidl_typesupport#17 decided to keep the suffix _packages. One reason is consistency across group name and another is to avoid the ambiguity of e.g. rmw_implementation (a ROS package) vs. rmw_implementations (a group name) (see ros2/rmw_implementation#36).