Use node namespace if no other was specified

[ivanpauno]

If no namespace is pushed (by PushRosNamespace), and the user doesn't use node_namespace argument of Node action, the default namespace specified by the node should be used.

I forgot about that, and in that case __ns:=/ was being added to the command (which is wrong).
I'm doing the corrections to avoid that.

---

(Edit) I finally didn't allow this

(Original)

If someone uses PushRosNamespace(''), and the node_namespace argument of Node action is the default (''), the namespace indicated by the node is used.

I thought it could by helpful for this case:

LaunchDescription([
    ...,
    PushRosNamespace(LaunchConfiguration(variable_name='namespace', default=''),
    Node(...),
    ...,
])

In that case, if the variable name doesn't exist, the namespace specified by the node will be used (and there is no need of copying it again in the launch file).

The other option is to raise an error when someone pass '' to PushRosNamespace.

[wjwwood]

I'm pretty sure you'll need type annotations here. Please setup and use mypy to check your code when making changes. We'll likely add a unit test for mypy when this pr is finished: ament/ament_lint#154

[ivanpauno]

Solved in 2301294

[orduno]

@ivanpauno thanks for the change, I tested and solves #50.

What do you think of the following use case with the modified timer_launch.py from #50:

from launch import LaunchDescription
from launch_ros.actions import PushRosNamespace
from launch.actions import GroupAction

import launch.actions
import launch.substitutions
import launch_ros.actions

def generate_launch_description():
    """Launch a talker and a listener."""
    return LaunchDescription([
        GroupAction([
            PushRosNamespace('launch_ns'),
            launch_ros.actions.Node(
                package='examples_rclcpp_minimal_timer',
                node_executable='timer_member_function',
                output='screen')
        ])
    ])

The node name is now /launch_ns/minimal_timer and not /launch_ns/timer_ns/minimal_timer.

[ivanpauno]

@ivanpauno thanks for the change, I tested and solves #50.

What do you think of the following use case with the modified timer_launch.py from #50:

from launch import LaunchDescription
from launch_ros.actions import PushRosNamespace
from launch.actions import GroupAction

import launch.actions
import launch.substitutions
import launch_ros.actions

def generate_launch_description():
    """Launch a talker and a listener."""
    return LaunchDescription([
        GroupAction([
            PushRosNamespace('launch_ns'),
            launch_ros.actions.Node(
                package='examples_rclcpp_minimal_timer',
                node_executable='timer_member_function',
                output='screen')
        ])
    ])

The node name is now /launch_ns/minimal_timer and not /launch_ns/timer_ns/minimal_timer.

I guess timer_ns was specified in the node source code, right?
If that's the case, I think this is fine as-is.
It worked in ROS 1 in the same way, didn't it?

[orduno]

I guess timer_ns was specified in the node source code, right?

yes

If that's the case, I think this is fine as-is.
It worked in ROS 1 in the same way, didn't it?

Not sure if that's the way it worked on ROS1, or if that's the way it's intended for ROS2 or the way it worked before PushRosNamespace was introduced.

@wjwwood If a namespace is specified at launch, should this overwrite any namespace specified in the node constructor? If so, is there a way we can append the namespaces, i.e. /launch_ns/constructor_ns/node_name

[wjwwood]

I don't think there is, I believe the __ns:= command line argument overrides the namespace, it does not extend it.

In C++ you can use a "sub-node" to get the effect you want. Launch will set the original node's namespace, but your sub-node will always extend the original node's namespace.

[orduno]

@wjwwood thanks for the input. One more question, arguments provided in the constructor options rclcpp::NodeOptions take precedence over the command line ones?

[wjwwood]

I had to dig a big to find this, but the arguments in the NodeOptions are called "local arguments" at the C API level, and they are "targeted at the node" where as global arguments are applied to all nodes, and the local ones are checked frist:

https://github.com/ros2/rcl/blob/e9279d191461cf998207cf1f571f5c408961cc1a/rcl/include/rcl/remap.h#L50-L54

But to answer your question directly, yes, the arguments given in the NodeOptions will take precedence over the global arguments which are usually coming from the command line.

[ivanpauno]

I've added namespace validation in PushRosNamespace.

---

I finally didn't allow PushRosNamespace(''), because it may be confusing:

LaunchDescription([
    ...,
    PushRosNamespace('my_ns'),
    PushRosNamespace(LaunchConfiguration(variable_name='SOME_NAMESPACE', default=''),
    Node(...),
    ...,
])

If SOME_NAMESPACE value is asd and the namespace specified in the node is node_ns, the result will be my_ns/asd.
If SOME_NAMESPACE takes the default value, the result will be my_ns.

If you delete the first PushRosNamespace action, the results will be asd and node_ns.

This may be very confusing, specially when combining PushRosNamespace before IncludeLaunchDescription.

[wjwwood]

I don't understand the issue with your most recent example @ivanpauno.

This may be very confusing, specially when combining PushRosNamespace before IncludeLaunchDescription.

I don't understand what's confusing, setting the namespace outside of a launch description (before the include action that included it) is an important feature, and if the PushRosNamespace ends up taking '' then it is essentially as if the action never occurred right? I don't find either behavior to be odd, honestly.

[ivanpauno]

I don't understand the issue with your most recent example @ivanpauno.

I will try to explain it in a clearer way. I think that accepting PushRosNamespace('') as a no-op is still fine, but in some cases it may be confusing.

---

Launch description of launch file 1:

LaunchDescription([
    ...,
    PushRosNamespace(LaunchConfiguration(variable_name='SOME_NAMESPACE', default=''),
    Node(...),
    ...,
])

Launch file 2:

LaunchDescription([
    ...,
    PushRosNamespace('my_ns')
    IncludeLaunchDescription(launch_file_path='<path/to/above/example>'),
    ...,
])

The node specified in the node source code is in all the cases node_ns.

Results when running launch file 1:

• SOME_NAMESPACE value is asd: asd.
• SOME_NAMESPACE takes the default: node_ns.

Results when running launch file 2:

• SOME_NAMESPACE value is asd: my_ns/asd.
• SOME_NAMESPACE takes the default: my_ns.

---

With this slightly modified launch file 1:

LaunchDescription([
    ...,
    PushRosNamespace(LaunchConfiguration(variable_name='SOME_NAMESPACE', default='node_ns'),
    Node(...),
    ...,
])

The results when running launch file 1 directly are the same.
Results when running launch file 2 are:

• SOME_NAMESPACE value is asd: my_ns/asd.
• SOME_NAMESPACE takes the default: my_ns/node_ns.

---

My original idea was that the first example should work as the second one.
If you think that PushRosNamespace('') working as a no-op is ok, I will re-add that feature.

[wjwwood]

I don't see any issue with the second example, it is essentially the same as if the "launch file 1" had no PushRosNamespace action in it. The namespace was overridden by an external launch file, which is totally fine.

[ivanpauno]

I don't see any issue with the second example, it is essentially the same as if the "launch file 1" had no PushRosNamespace action in it. The namespace was overridden by an external launch file, which is totally fine.

@wjwwood solved in f4cfcfe.

[ivanpauno]

CI (only fastrtps, up to launch launch_xml launch_yaml launch_ros test_launch_ros):

• Linux (flake8 failure)
• Linux-aarch64 (flake8 failure)
• macOS (flake8 failure)
• Windows (flake8 and unrelated)

[ivanpauno]

Double check flake8 pass:

• Linux

[wjwwood]

Please flake8

😆, lgtm