[ament_index] Should fix / simplify injection of per-package `AMENT_PREFIX_PATH`

[EricCousineau-TRI]

I don't understand why we inject per-package AMENT_PREFIX_PATH. Can we simplify and report as few paths as possible, if ament_index recurses? (We know Drake's PackageMap does)

For example, in #269, I only depend on the data for rviz_default_plugins: https://github.com/RobotLocomotion/drake-ros/blob/543402f8ef5bf73e433e1ff56ff361fb73c14103/drake_ros_examples/BUILD.bazel#L23-L31

However, if I print out os.environ["AMENT_PREFIX_PATH"], I see all of the packages listed. For PackageMap, I see the following slew of errors:

$ bazel run //:parse_ros_model_test
<313 lines of similar errors>
WARNING  drake:None:0 Unable to open directory: {runfiles}/ros2/archive/ament_clang_tidy
WARNING  drake:None:0 Unable to open directory: {runfiles}/ros2/archive/ament_clang_format

Assigning @sloretz for disposition, \cc @adityapande-1995

FYI @adeeb10abbas @jwnimmer-tri

[EricCousineau-TRI]

Shane, can you provide a suggestion for the simplest route to success? Is there a way we can be more selective with what paths we provide?

Once you provide that, either Aditya or I can execute on it.

[sloretz]

Shane, can you provide a suggestion for the simplest route to success?

I'd be happy to!

The archive we're using at the moment is an isolated workspace. This means every package has a unique install location (usually install/<package name>), and so each has its own entry in AMENT_PREFIX_PATH.

The only way to get a single AMENT_PREFIX_PATH for all packages is to use a merged workspace. For example, /opt/ros/<distro> is a merged workspace, so using debian packages instead of an archive would mean there was only a single value in AMENT_PREFIX_PATH. It would also be possible to manually build an archive that uses a merged workspace instead of an isolated one (colcon build --merge-install).

Can we simplify and report as few paths as possible, [...] For example, in [...] I only depend on the data for rviz_default_plugins:

Right now the generated distro.bzl file puts all of the paths into a single variable for the dload shim to use. It might be possible to make this a per target thing (where the rules have an AmentIndex provider like the hydroelastic collisions PR uses).

However, I'm not aware of any performance issues with regards to long values of AMENT_PREFIX_PATH (aside from increasing length of time it takes to source a workspace when using some shells, like Windows cmd.exe). Uploading a merged workspace to Girder seems to me like it would be less work for a better result.

[EricCousineau-TRI]

Constraining the archive type does sound like an easier solution, but my ideal is that we need not constrain things this way. I think going the AmentIndex provider route might be better, but can be convinced others.

However, given that #133 landed, maybe it's not too hard?

[sloretz]

Constraining the archive type does sound like an easier solution, but my ideal is that we need not constrain things this way.

Instead of thinking this as a constraint on the archive type, I think it would be fair to call it a choice of the type of workspace Drake-ROS uses by default. I think users are already free to use either a merged or isolated workspace with Drake-ROS today.

I think going the AmentIndex provider route might be better, but can be convinced others.

Making each ROS package in the generated BUILD.bazel have an AmentIndex would lead to fewer entries in AMENT_PREFIX_PATH when using an Isolated ROS workspace as each target would have only the entries it needs, but how much better does it need to be to justify the cost of developing it?

If the metric for "better" is runtime performance, I'm not aware of any significant performance degradation with long values of AMENT_PREFIX_PATH. If human readability is the metric, anything that uses Drake-ROS is probably going to depend on @ros2//:rclcpp_cc directly or indirectly. Running colcon list --packages-up-to rclcpp | wc -l on my ROS 2 workspace returns 125 packages. My ROS workspace has 354 packages in total. An AMENT_PREFIX_PATH with 125 entries instead of 354 entries is still going to be hard for a human to read.

[EricCousineau-TRI]

If the workspace is merged and should effectively resolve to a shorter AMENT_PREFIX_PATH, I think that'd be a better option? And my desire is that using AmentIndex provider should be able to easily recognize that, and/or it should be recognized via shim mechanisms.

At this point, the maximal case (depending on all packages) should yield a path list whose size is <= what we currently have, at which point there should only be gains for this approach.

Does that make sense?

[sloretz]

If the workspace is merged and should effectively resolve to a shorter AMENT_PREFIX_PATH, I think that'd be a better option?

Yes, I think using a merged workspace is the best option for getting shorter values of AMENT_PREFIX_PATH

And my desire is that using AmentIndex provider should be able to easily recognize that, and/or it should be recognized via shim mechanisms.

I don't understand. What is it the AmentIndex provider should recognize?

[EricCousineau-TRI]

Ah, sorry - meant that AmentIndex should be able to deduplicate any paths that are listed twice, whether it be for a merged workspace or an isolated workspace.

[sloretz]

Ah, sorry - meant that AmentIndex should be able to deduplicate any paths that are listed twice, whether it be for a merged workspace or an isolated workspace.

Ah, are you saying that if there are two AmentIndex providers that point to the same prefix (say we have a target depending on two calls ament_index_share_files twice with the same prefix), then we should de-duplicate those paths before passing them to a dload shim?

If so, maybe we could do that by making this a set (or is depset the right bazel datastructure?):

https://github.com/RobotLocomotion/drake-ros/blob/00b0b03fef9a267ca02e6bb2bc3b6dde6fefa58b/bazel_ros2_rules/ros2/tools/ament_index.bzl#L27