Re-Export Generated Messages in rclrs

[maspe36]

Summary

Working example of re-exporting of the generated interface crates found on the AMENT_PREFIX_PATH env var. With this change generated interface symbols are available directly from rclrs::, so for example, rclrs::std_msgs::msg::String is how you'd access the String type of the generated rust code for the std_msgs package.

This depends on

• Relative Module Path Resolution rosidl_rust#12 for fully relocatable symbols.
• Skip Generating Marker File for Reexported Crates cargo-ament-build#35 to have generated message crates omitted from patching w/ colcon-ros-cargo
• Updated the examples repo to build with the re-export messages changes examples#19

The way we find the generated code is as follows

1. For all entries in AMENT_PREFIX_PATH, look for a share/ directory.
2. For all entries in share/ look for <package>/rust/
3. If there is a Cargo.toml within that entry, with the metadata to re-export symbols, include! all source files within src/ such that all symbols resolve the same.

i.e. std_msgs::msg::String for a standalone crate build, is effectively the same symbol as rclrs::std_msgs::msg::String.

I believe this to be a viable alternative to patching for message packages, and (could) effectively eliminate the need for vendoring some messages.

Open questions

1. How does AMENT_PREFIX_PATH get populated? If its populated as colcon is building packages (which I believe to be the case), then this will also re-export symbols in your own workspace as well as sourced overlays.

2. Should this actually go into rclrs or a separate crate (i.e. ros_msgs?) By going into rclrs we eliminate the need for vendoring.

3. Its important to know that if we re-build rclrs, and the AMENT_PREFIX_PATH env var has changed, then the build.rs will be re-built, which will then re-build all of the generated code.

Honestly, I'm not sure if we can really get around this. Rust just really wants you to recompile the code you're using each time you invoke cargo. We can and should have everything in a colcon workspace share a target directory, but this won't stop all rebuilds. Its possible we expect compiled .rlibs in the share directory and we generate a file which has extern crate calls to force rustc to find the dependencies, but this will likely not work with any intellisense, and its overall pretty fragile IMO.

4. Can we get better error messages? If a user forgot to source something or expects an interface to exists but one doesn't, they are met with errors like this

  error[E0433]: failed to resolve: could not find `action_msgs` in the crate root
   --> rclrs/src/action/action_client.rs:5:6
    |
  5 |     {action_msgs::srv::CancelGoal_Response, builtin_interfaces::msg::Time},
    |      ^^^^^^^^^^^ could not find `action_msgs` in the crate root

[maspe36]

@esteve I believe this is essentially an implementation of the thought you had back in #394. We could also use feature flags instead of embedding metadata in the generated crates like I did. That may cut down on some rebuilds?

EDIT: Future me, we can only use feature flags for base ROS 2 messages (i.e. builtin_interfaces etc), we can't use features for user defined message packages.

[luca-della-vedova]

Very interesting approach! And that's the kind of diff that gets me excited 😄
I gave this a try and I have some comments.

First of all, removing colcon-ros-cargo to fix CI is not a fix and actually breaks CI, so it should be reverted and the CI issues should be solved.
Removing colcon-ros-cargo will actually skip all the ros-cargo packages, including rclrs, if you look at the logs from the green CI you will notice that these warnings are printed:

[0.461s] WARNING:colcon.colcon_core.verb:No task extension to 'build' a 'ros.ament_cargo' package

And the build log shows that rclrs was actually not built and not tested, so that change effectively made the CI a noop.
There are two test failures, one is a very underwhelming cargo fmt run, the other is a failure to compile a unit test (seems to me that line was added by accident?):

error[E0423]: expected value, found struct `crate::rcl_interfaces::msg::IntegerRange`
   --> rclrs/src/subscription.rs:537:17
    |
537 |           let _ = crate::rcl_interfaces::msg::IntegerRange;
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use struct literal syntax instead: `crate::rcl_interfaces::msg::IntegerRange { from_value: val, to_value: val, step: val }`
    |
   ::: ~/ws_rclrs/install/rcl_interfaces/share/rcl_interfaces/rust/src/msg.rs:52:1
    |
52  | / pub struct IntegerRange {
53  | |     pub from_value: i64,
54  | |     pub to_value: i64,
55  | |     pub step: u64,
56  | | }
    | |_- `crate::rcl_interfaces::msg::IntegerRange` defined here

For more information about this error, try `rustc --explain E0423`.
error: could not compile `rclrs` (lib test) due to 1 previous error

Removing the line and running cargo fmt makes colcon test succeed on my machine.
Now to the questions

Open questions

1. How does AMENT_PREFIX_PATH get populated? If its populated as colcon is building packages (which I believe to be the case), then this will also re-export symbols in your own workspace as well as sourced overlays.

It's... Interesting... I gave this a try and it seems that the content on the variable contains (together with the environment variable) the prefixes of the package that the current package depends on. My guess (and what I hope colcon would do) is that behind the scenes it looks for all the packages specified in package.xml and sources them before building the required package, so the prefix contains all the required dependencies but not more than that, which is pretty good behavior.

2. Should this actually go into rclrs or a separate crate (i.e. ros_msgs?) By going into rclrs we eliminate the need for vendoring.

I'm very partial about red diffs but what is imho even more great about bundling is removing these confusing vendoring issues.
A question still stands though, I thought some of the reason for vendoring was distributing the crate standalone on crates.io. How would we go about that with this approach?

3. Its important to know that if we re-build rclrs, and the AMENT_PREFIX_PATH env var has changed, then the build.rs will be re-built, which will then re-build all of the generated code.

Yeah this is not ideal, other people might disagree but personally I don't feel it is a definitive blocker, if the behavior is robust and developer friendly we can accept less than ideal compile times for now.
Probably a noob question but how does the script detect that the environment variable changed? It seems to work and I expected to see a rerun-if-env-changed directive but I couldn't find any.

Other general comments

I also tried to build the message_demo example and noticed that this doesn't work. I think the issue is that you are not including feature flags for message packages.

error[E0277]: the trait bound `rclrs::rclrs_example_msgs::msg::rmw::VariousTypes: serde::Deserialize<'de>` is not satisfied
    --> src/message_demo.rs:110:28
     |
110  |     let rmw_deserialized = serde_json::from_str(&rmw_serialized)?;
     |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `serde_core::de::Deserialize<'_>` is not implemented for `rclrs::rclrs_example_msgs::msg::rmw::VariousTypes`
     |
     = note: for local types consider adding `#[derive(serde::Deserialize)]` to your `rclrs::rclrs_example_msgs::msg::rmw::VariousTypes` type

And finally, I noticed that you implemented parsing of the environment variable into paths / splitting manually, have you considered using ament_rs to cut down on that boilerplate?

[luca-della-vedova]

This feels like the main hairy part to me, does this mean that we will require message packages to always have a * version requirement? Would specifying message versions like we do in our examples not work?
On the other hand, how do we make sure we are only re-exporting message packages? What happens if a user has a Rust library installed instead? Is there a risk that if I have a foo library that is:

• A colcon-ros-cargo package.
• Installed and in the prefix.
• Dependency (possibly with a * version).

Then rclrs will end up bundling it and exporting rclrs::foo?

[balthasarschuess]

I think this should answer the last question: ros2-rust/rosidl_rust@main...maspe36:rosidl_rust:feature/relative_symbol_resolution#diff-bd0ce76b9774a1e4b569a95ae620dfb3422798d60bcf4c01ed6f501c978e3426
There is a meta tag added to the packages generated by rosidl, that we filter for.

Regarding versioning: I don't think using anything else than a * dependency make sense here.
Since the dependency resolution is managed by colcon and should be specified in the package.xml.

In the context of standalone rclrs. Maybe we could work with feature flags, to only include vendored messages given the standalone feature.

[maspe36]

require message packages to always have a * version requirement?

Not necessarily, this is just what I did in the first pass as this is actually how we're specifying dependencies in the generated crates. We could evolve this to be an intersection of dependencies found in both the Cargo.toml and package.xml, or perhaps some other mechanism like metadata in the Cargo.toml.

On the other hand, how do we make sure we are only re-exporting message packages?

I don't think we really can fully prevent this because we do want users to be able to generate and install their own rust crates representing their custom message packages.

What happens if a user has a Rust library installed instead?

I have gone ahead and removed the logic to create the marker file for crates with the expected metadata. colcon-ros-cargo won't find the generated rust crates for message packages anymore.
ros2-rust/cargo-ament-build#35

If we decide on another approach then this will need to be revisited.

[maspe36]

First of all, removing colcon-ros-cargo to fix CI is not a fix and actually breaks CI

Yup, fixed this by making a change to cargo-ament-build to no longer install the rust_packages marker file for crates that have this reexport metadata
ros2-rust/cargo-ament-build#35

Verified that colcon build and colcon test now actually are running :D

Removing the line and running cargo fmt makes colcon test succeed on my machine.

Ah yeah, whoops. Fixed!

I thought some of the reason for vendoring was distributing the crate standalone on crates.io. How would we go about that with this approach?

This is an extremely valid point, thank you for bringing this up. I would propose supplying the absolute bare minimum required to satisfy compilation and having that be conditionally included if we're doing a build for docs.rs. I'll see how feasible this really is.

Probably a noob question but how does the script detect that the environment variable changed?

Not a noob question! In my early changes I was using env!, and by simply using the env! (or option_env!) macro build scripts will automatically rerun when the value is changed. I have since moved away from the macro as a first step towards being able to build for docs.rs.

As of 1.46, using env! and option_env! in source code will automatically detect changes and trigger rebuilds. rerun-if-env-changed is no longer needed for variables already referenced by these macros.

https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-env-changed

I also tried to build the message_demo example and noticed that this doesn't work. I think the issue is that you are not including feature flags for message packages.

We need the serde feature on for rclrs in that example I believe. Enabled the feature, and everything built (well, with warnings still). Went ahead and put up a PR with the required changes.
ros2-rust/examples#19

And finally, I noticed that you implemented parsing of the environment variable into paths / splitting manually, have you considered using ament_rs to cut down on that boilerplate?

I didn't find much value from this crate honestly, but I have gone ahead and used it. I could see improvements being made to it in the future.

[maspe36]

I have enabled cargo docs builds in 1ff04a1

I believe all outstanding comments have been fully addressed now

[mxgrey]

I haven't given this a test run yet, but conceptually the more I think about this approach the more I like it. It resolves every technical issue that I can think of, and doesn't introduce any new problems besides just being unconventional (or you could also say creative or resourceful).

Should this actually go into rclrs or a separate crate (i.e. ros_msgs?) By going into rclrs we eliminate the need for vendoring.

This would be my only feedback at this time. I believe very strongly that this should go into its own crate, maybe named ros_messages or rosidl (I would avoid ros_msgs because too many message packages end with _msgs, so the name might make users think this is actually a message package itself).

As a separate crate it would be feasible for r2r or other Rust-based ROS client library implementations to share the same message package pipeline. I think that would be a huge win for the ecosystem. As far as I can figure this would still fix the need for vendoring messages within rclrs.