Improve ros_launch support for launch files

[julianadrianheine]

Motivation

Previously, ros_launch required users to manually resolve Bazel runfile paths and use ExecuteProcess to launch nodes. This PR enables the standard ROS 2 launch pattern (launch_ros.actions.Node) to work with Bazel-built binaries by registering them in a fake ament prefix at build time.

For more details, you can check out this issue: #417

Implementation details

• Extends ament_index_share_files in ament_index.bzl to support an executables attribute, which symlinks Bazel-built binaries under <prefix>/lib/<package_name>/. This makes them discoverable by launch_ros.actions.Node(package=..., executable=...) without a colcon install space.
• Adds an executables parameter to ros_launch: a dict mapping ROS 2 package names to lists of executable targets. The macro automatically creates the ament index entries and wires up the runfiles.
• Removes the data parameter from ros_launch (executables registered via executables are automatically added to the binary's runfiles).
• Updates the example launch app to use the idiomatic launch_ros.actions.Node pattern instead of resolving and spawning binaries manually via ExecuteProcess + runfiles.

---

This change is 

[jwnimmer-tri]

FYI the package name is available as native.package_name() in macros. Possibly that is less brittle than making the user type it in?

[julianadrianheine]

FYI the package name is available as native.package_name() in macros. Possibly that is less brittle than making the user type it in?

That's a really good point. I will address this one, thanks!

[frneer]

@frneer reviewed 5 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on julianadrianheine).

---

bazel_ros2_rules/lib/ament_index.bzl line 160 at r1 (raw file):

    )

def ament_index_executables(name, package_name, srcs, prefix = "ament_index_share_files", **kwargs):

I'd suggest including the logic in the existing original rule. It seems pretty similar to me adding files to share/ and to /lib.

Maybe the difference should be in how we are consuming the files from. Consider using a specific attribute for executables, instead of reading it from data.

ros_launch(
    name = "my_launch",
    launch_file = "my_launch.py",
    executables = [":my_node"], 
)

Another thing to consider related to this is, with the current format all listed nodes will be part of the package name passed to the ros_launch but that is not necessarily the case, we might be bringing nodes from other packages.

I've thought of having sth like:

ros_launch(
    name = "my_launch",
    launch_file = "my_launch.py",
    executables = {
          "package_1":my_node"
          "package_2":my_other_node",
    }      
)

So you can do:

           launch_ros.actions.Node(
                package="package_1",
                executable="my_node",
            ),
           launch_ros.actions.Node(
                package="package_2",
                executable="my_other_node",
            ),

But at the same time, we don't really care which package it came from, we only care about resolving the correct executable path without relying the RLocation... so maybe keeping a single package name is good enough.

---

ros2_example_bazel_installed/ros2_example_apps/eg_launch.py line 17 at r1 (raw file):

        [
            # Running a talker written in python.
            ExecuteProcess(cmd=[talker_bin]),

this example should follow the new way of using ros_launch.

[julianadrianheine]

@julianadrianheine made 2 comments.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on frneer).

---

bazel_ros2_rules/lib/ament_index.bzl line 160 at r1 (raw file):

Previously, frneer (Francisco Rossi) wrote…

I'd suggest including the logic in the existing original rule. It seems pretty similar to me adding files to share/ and to /lib.

Maybe the difference should be in how we are consuming the files from. Consider using a specific attribute for executables, instead of reading it from data.

ros_launch(
    name = "my_launch",
    launch_file = "my_launch.py",
    executables = [":my_node"], 
)

Another thing to consider related to this is, with the current format all listed nodes will be part of the package name passed to the ros_launch but that is not necessarily the case, we might be bringing nodes from other packages.

I've thought of having sth like:

ros_launch(
    name = "my_launch",
    launch_file = "my_launch.py",
    executables = {
          "package_1":my_node"
          "package_2":my_other_node",
    }      
)

So you can do:

           launch_ros.actions.Node(
                package="package_1",
                executable="my_node",
            ),
           launch_ros.actions.Node(
                package="package_2",
                executable="my_other_node",
            ),

But at the same time, we don't really care which package it came from, we only care about resolving the correct executable path without relying the RLocation... so maybe keeping a single package name is good enough.

Done.

• Regarding the first suggestion: instead of introducing a separate ament_index_executables rule, I added an executables argument directly to ament_index_share_files. The same argument was also added to ros_launch, which calls ament_index_share_files internally when executables is provided.

• Regarding the second suggestion: the executables argument in ros_launch is now a dict mapping ROS 2 package names to lists of Bazel executable targets. This allows nodes from multiple packages to be registered in the ament resource index. It is worth noting that the package name key is not validated by Bazel, what actually gets built and linked is determined by the Bazel labels. The package name only needs to be consistent between the ros_launch call and the package= argument in the corresponding launch_ros.actions.Node call in the launch file. The conventional choice is to use the same package name as in a colcon build, which makes the launch file portable between both build systems without modification.

---

ros2_example_bazel_installed/ros2_example_apps/eg_launch.py line 17 at r1 (raw file):

Previously, frneer (Francisco Rossi) wrote…

this example should follow the new way of using ros_launch.

Done.

Alright, I just wanted to add an extra example showing that both approaches are still possible. But I guess the usage of RLocation will be deprecated

[frneer]

Awesome work! Left some final commnets.

@frneer reviewed 5 files and all commit messages, made 6 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on julianadrianheine).

---

bazel_ros2_rules/lib/ament_index.bzl line 101 at r2 (raw file):
Consider using executable attr label: https://bazel.build/rules/lib/toplevel/attr

executable: bool; default is False
True if the dependency has to be executable. This means the label must refer to an executable file, or to a rule that outputs an executable file. Access the label with ctx.executable.<attribute_name>.

Also allow_files should be left empty or set to false.

Suggestion:

        executables = attr.label_list(
            allow_empty = True,
            executable = True,

---

bazel_ros2_rules/lib/ament_index.bzl line 73 at r2 (raw file):

            root_symlinks[symlink_path] = file

    for exe in ctx.attr.executables:

nit: don't use acronyms if unnecessary, it improves clarity.

Suggestion:

    for executable in ctx.attr.executables:

---

bazel_ros2_rules/lib/ament_index.bzl line 83 at r2 (raw file):

            exe_file.basename,
        )
        root_symlinks[symlink_path] = exe_file

After applying the suggestion below we won't need to manually check if target is an executable.

Suggestion:

    for exe in ctx.attr.executables:
        symlink_path = paths.join(
            ctx.attr.prefix,
            "lib",
            ctx.attr.package_name,
            exe_file.basename,
        )
        root_symlinks[symlink_path] = exe.files_to_run.executable

---

bazel_ros2_rules/lib/private/ros_py.bzl line 236 at r2 (raw file):

    )

    for pkg_name, pkg_executables in executables.items():

nit: should we validate pkg_executables is in deed a list?

---

bazel_ros2_rules/lib/private/ros_py.bzl line 244 at r2 (raw file):

            visibility = ["//visibility:private"],
        )
        data = data + pkg_executables + [":" + index_target]

nit: maybe data is no longer a good naming?

[julianadrianheine]

@julianadrianheine made 5 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on frneer).

---

bazel_ros2_rules/lib/ament_index.bzl line 73 at r2 (raw file):

Previously, frneer (Francisco Rossi) wrote…

nit: don't use acronyms if unnecessary, it improves clarity.

Done.

---

bazel_ros2_rules/lib/ament_index.bzl line 83 at r2 (raw file):

Previously, frneer (Francisco Rossi) wrote…

After applying the suggestion below we won't need to manually check if target is an executable.

Done.

---

bazel_ros2_rules/lib/ament_index.bzl line 101 at r2 (raw file):

Previously, frneer (Francisco Rossi) wrote…

Consider using executable attr label: https://bazel.build/rules/lib/toplevel/attr

executable: bool; default is False
True if the dependency has to be executable. This means the label must refer to an executable file, or to a rule that outputs an executable file. Access the label with ctx.executable.<attribute_name>.

Also allow_files should be left empty or set to false.

Executable is part of label, but not of label_list actually.

---

bazel_ros2_rules/lib/private/ros_py.bzl line 236 at r2 (raw file):

Previously, frneer (Francisco Rossi) wrote…

nit: should we validate pkg_executables is in deed a list?

That makes sense. I will add a check here!

---

bazel_ros2_rules/lib/private/ros_py.bzl line 244 at r2 (raw file):

Previously, frneer (Francisco Rossi) wrote…

nit: maybe data is no longer a good naming?

That's a good point. data was left as an argument because it was needed for the xml launch files. But this is actually not needed, we can still use ros_launch with xml files as long as we build the file as

<launch>
<executable cmd="<pkg_name>/<node_name>" output="screen"/>
</launch>

We still need to create a list inside ros_launch to pass to py_binary

[frneer]

@frneer reviewed 3 files and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on julianadrianheine).

---

bazel_ros2_rules/lib/ament_index.bzl line 101 at r2 (raw file):

Previously, julianadrianheine (julianheine) wrote…

Executable is part of label, but not of label_list actually.

Roger, so now we'll fail when trying to access the attribute in case it's an executable right?

[julianadrianheine]

@julianadrianheine made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on frneer).

---

bazel_ros2_rules/lib/ament_index.bzl line 101 at r2 (raw file):

Previously, frneer (Francisco Rossi) wrote…

Roger, so now we'll fail when trying to access the attribute in case it's an executable right?

Sorry, I don't think I follow. Since we actually want these files to be executables, we shouldn't fail. Right?

[frneer]

@frneer made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on julianadrianheine).

---

bazel_ros2_rules/lib/ament_index.bzl line 101 at r2 (raw file):

Previously, julianadrianheine (julianheine) wrote…

Sorry, I don't think I follow. Since we actually want these files to be executables, we shouldn't fail. Right?

If you pass a non-executable that's not a file that won't be catch by allow_files=False

When passing a for example a cc_library you'll end up with this error:

ERROR: /home/frn/drake-ros/ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel:241:11: in ament_index_share_files rule //ros2_example_apps:_roslaunch_eg_py_ament_index_ros2_example_apps: 
Traceback (most recent call last):
	File "/home/frn/.cache/bazel/_bazel_frn/a779afcb46037298559646c7c2f921d4/external/bazel_ros2_rules+/lib/ament_index.bzl", line 78, column 47, in _ament_index_share_files_impl
		executable.files_to_run.executable.basename,
Error: 'NoneType' value has no field or method 'basename'

Which is not impossible to debug but I think we can have a better behavior here, maybe explicitly failing when the user passes a non-executable non-file target?

[julianadrianheine]

@julianadrianheine made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on frneer).

---

bazel_ros2_rules/lib/ament_index.bzl line 101 at r2 (raw file):

Previously, frneer (Francisco Rossi) wrote…

If you pass a non-executable that's not a file that won't be catch by allow_files=False

When passing a for example a cc_library you'll end up with this error:

ERROR: /home/frn/drake-ros/ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel:241:11: in ament_index_share_files rule //ros2_example_apps:_roslaunch_eg_py_ament_index_ros2_example_apps: 
Traceback (most recent call last):
	File "/home/frn/.cache/bazel/_bazel_frn/a779afcb46037298559646c7c2f921d4/external/bazel_ros2_rules+/lib/ament_index.bzl", line 78, column 47, in _ament_index_share_files_impl
		executable.files_to_run.executable.basename,
Error: 'NoneType' value has no field or method 'basename'

Which is not impossible to debug but I think we can have a better behavior here, maybe explicitly failing when the user passes a non-executable non-file target?

Oh I get your point now, thanks! Yes, we can add a check to verify if executable.files_to_run.executable == None

Now, if passing for example a cc_library the error is more explicit:

Traceback (most recent call last):
	File "/home/julianheine/.cache/bazel/_bazel_julianheine/564c480f292c1ed4c1cadf898631f510/external/bazel_ros2_rules+/lib/ament_index.bzl", line 75, column 17, in _ament_index_share_files_impl
		fail("{} is not an executable target".format(executable.label))
Error in fail: @@//ros2_example_apps:listener_cc is not an executable target

[frneer]

@frneer reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on julianadrianheine).

---

bazel_ros2_rules/lib/ament_index.bzl line 101 at r2 (raw file):

Previously, julianadrianheine (julianheine) wrote…

Oh I get your point now, thanks! Yes, we can add a check to verify if executable.files_to_run.executable == None

Now, if passing for example a cc_library the error is more explicit:

Traceback (most recent call last):
	File "/home/julianheine/.cache/bazel/_bazel_julianheine/564c480f292c1ed4c1cadf898631f510/external/bazel_ros2_rules+/lib/ament_index.bzl", line 75, column 17, in _ament_index_share_files_impl
		fail("{} is not an executable target".format(executable.label))
Error in fail: @@//ros2_example_apps:listener_cc is not an executable target

Awesome!