Revisit injection of launch_ros description

[jacobperron]

Feature request

Feature description

This issue was raised as part of #97 and previously discussed in ros2/launch#208.

Both ros2 launch and ros2 test inject an included description that creates an rclpy node. This node is often useful for testing, though not always needed, and necessary to launch certain actions (e.g. LifecycleNode). The problem is that it can cause confusion and user error.

Related to testing, we end up with two types of launch test files: those that can be executed with launch_test and those that require using ros2 test. As evidenced by #97, this can lead to seemingly broken launch files, which are actually just user error (executing with launch_test instead of ros2 test).

Related to using ros2 launch, if a user decides to include the default description themselves (perhaps a rare case), they will end up starting multiple nodes with the same name. This currently results in the following warning, but may be escalated to an error in the future:

[WARN] [rcl.logging_rosout]: Publisher already registered for provided node name. If this is due to multiple nodes with the same name then all logs for that logger name will go out over the existing publisher. As soon as any node with that name is destructed it will unregister the publisher, preventing any further logs for that name from being published on the rosout topic.

I've opened this ticket to discuss alternatives to relying on the CLI tools for including launch_ros description.

Implementation considerations

Proposed solutions:

1. Remove the injection

• In this case, we rely on the users to provide the boiler plate for including the ROS-specific description if they need it. Example.
• Pros:
  • rostests can be run with launch_test or ros2 test.
  • Helps avoid accidentally including the default description more than once.
• Cons:
  • Extra boilerplate for the user.
  • Requires knowledge of which launch_ros actions require the inclusion of the default description.

2. Only inject when necessary

• Somehow know when an action requires the extra node provided by the default description and conditionally include it. This would require knowing when the description is already included so it is not included multiple times.
• Pros:
  • Same as (1).
• Cons:
  • Added complexity to launch and/or launch_ros.

I don't have any solid ideas at the moment for how to accomplish (2). But I think it makes sense in any solution that we guard against including the default description multiple times.

[hidmic]

I'd go for 2. IMHO launch_ros actions that rely on having an rclpy in the launch context should set it up themselves instead of assuming it has been taken care of already. Maybe drop the concept of a default launch description in favor of an idempotent launch_ros utility that initializes the launch context to sustain ROS specific actions?

[stonier]

2. SGTM so long as it isn't the rostest client itself doing the injection. Something along the lines of having launch_ros figure it out in an idempotent fashion as @hidmic describes sounds good to me too.

[ivanpauno]

Seems to have been fixed in #128, closing!