Skip to content

Declare rust_packages only when installing Rust IDL bindings #380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

mxgrey
Copy link
Collaborator

@mxgrey mxgrey commented Mar 20, 2024

As discussed here there was a strange error that made it impossible to use ros2_rust in an underlay workspace. Packages that have no association to Rust were declaring themselves in the rust_packages ament index. This was causing downstream colcon workspaces to redirect their .cargo/config.toml file towards non-existent directories. When cargo is directed towards non-existent directories, it considers the situation a build error and exits immediately.

After some challenging investigation, I narrowed the problem down to this: Packages that contain any message definitions were invoking the rosidl_generator_rs's extension to the ROS IDL generation pipeline. That extension included the line ament_index_register_resource("rust_packages"). For a normal message package that would be fine, but some packages have message definitions that are purely internal to the package for testing purposes and not installed (using the SKIP_INSTALL option of rosidl_generate_interfaces). Since the messages are not installed in these cases, we also do not install the Rust bindings, which makes perfect sense. However, if we declare the package under the rust_packages index, then colcon-ros-cargo will expect a Rust package even though one does not exist.

This PR solves the problem in an extremely simple way: The rosidl_generator_rs extension will only declare a package into the rust_packages index if the message package is being installed. I've done an A/B test on this change and confirmed that building an rclrs node in a downstream workspace is impossible without this, but works perfectly fine with the changes in this PR.

Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mxgrey thanks!

@esteve esteve merged commit 34da732 into ros2-rust:main Mar 20, 2024
@mxgrey mxgrey deleted the fix_rust_packages_resource branch March 21, 2024 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants