Prevent message packages from crates.io to be included during the build

[esteve]

As a side effect of #392, a set of message packages are now published on crates.io (see https://crates.io/crates/builtin_interfaces for example), although they've been yanked, this exposes an issue in our build system that anyone could register a crate named like a message package and potentially cause issues.

I've thought of something like this:

• rosidl_generator_rs stays the same
• a new base ros2_messages crate
• the ros2_messages crate can accept a list of features, each feature is the name of a message package
• the ros2_messages crate uses ament_rs to find the paths of the generated Rust files for each message package and uses include!() on those files
• downstream packages can access the messages via ros2_messages::buildin_interfaces, ros2_messages::std_msgs, etc.

I'm going to work on this urgently as the current situation needs to be fixed before it causes any breakage.

@jhdcs @maspe36 @mxgrey @luca-della-vedova thought?

[mxgrey]

This is an interesting idea. But it's such an unconventional use of crates.io and cargo that I'd need to think very carefully about potentially bad side effects.

Would we also publish a ros2_messages crate to crates.io?

• If so, what would it include? I'm not even sure how include!() works for published packages.
• If not, what stops someone from publishing one and breaking this new approach?

[esteve]

@mxgrey ros2_messages would be published, that'd be our way of gatekeeping anyone from abusing crates.io to inject any dependencies. I'd argue that our combination depending on message packages whose version is "*" and using colcon is far more unconventional.

If so, what would it include? I'm not even sure how include!() works for published packages.

It'd have code to read features from Cargo.toml and call include!(...) the appropriate messages. We already publish rclrs to crates.io and it contains a call to include!(...)

[jhdcs]

The problem is that no matter what, the interaction between colcon and cargo has always forced us to make unconventional approaches. They're both opinionated systems, and it feels like their opinions often don't match. @esteve already mentioned the black magic we're currently doing in the post above.

Personally, I feel like this fix is the right way to go. Even if it only turns out to be short-term, it gets people building again. Once it's out we all can try and brainstorm if a better solution exists.

[esteve]

Also, a nice bonus of this solution is that I think we'd no longer need to vendorize messages.

[jhdcs]

I like avoiding needing to vendorize messages. That should hopefully make things a bit easier on the end user as well...

[esteve]

Looks like this solution won't work, a dependency needs to explicitly declare its features, so a downstream can't pass arbitrary features to a dependency. Does anyone know how to pass arbitrary metadata to a dependency? Alternatively, I think we could pass compiler flags to rustc, that could be read by ros2_messages and therefore call include!(..) approrpriately for each message package.

[jhdcs]

I'm not seeing anything on passing arbitrary metadata to a dependency, but I'll try to keep looking...

[jhdcs]

I don't suppose the pkg-config crate would be useful for this?

https://crates.io/crates/pkg-config

[esteve]

I don't suppose the pkg-config crate would be useful for this?

Nice find! pkg-config might not be portable to platforms other than Unix, but I think it's a really good lead. Thanks!

[jhdcs]

Glad I could help! For extra info, I bumped into that package while browsing through the official Build Script Examples page:

https://doc.rust-lang.org/cargo/reference/build-script-examples.html

[esteve]

I'm leaning towards adding this functionality to a build script. Or perhaps to the cargo ament plugin, which could read the dependencies from package.xml and therefore declare the messages packages.

[jhdcs]

I like the idea of reading the dependencies from the package.xml if possible. Not sure how much more difficult that would be, though.

[esteve]

cargo ament build accepts a metadata.ros section (see https://github.com/ros2-rust/cargo-ament-build), which we can extend to add a messages field and declare the messages there. Eventually we could make it more magic and do that automatically, though.

[maspe36]

So I'm trying to understand our message pipeline thoroughly. I'm taking a look at colcon-ros-cargo currently. This is the colcon plugin that will ultimately call cargo build for packages that have a Cargo.toml and a package.xml.

One of the main jobs for this plugin, is to create this .cargo/config.toml file in whatever folder you're calling colcon build. This file, "patches" our dependencies, or in other words, overrides dependencies with other source file copies (1, 2). But its not just any dependency patch, its actually a patch to the version we would pull from crates.io.

An example snippet from my <workspace>/.cargo/config.toml

[patch.crates-io.rcl_interfaces]
path = "<workspace>/install/rcl_interfaces/share/rcl_interfaces/rust"

As I understand it, the intention here is to override any version of rcl_interfaces I may pull from crates.io for any colcon-ros-cargo (and maybe colcon-cargo as well?) ROS package, and use the version at the given path (This is a full crate, it has source files and a Cargo.toml).

<workspace>/install/rcl_interfaces/share/rcl_interfaces/rust
├── build.rs -> <workspace>/build/rcl_interfaces/rosidl_generator_rs/rcl_interfaces/rust/build.rs
├── Cargo.toml -> <workspace>/build/rcl_interfaces/rosidl_generator_rs/rcl_interfaces/rust/Cargo.toml
└── src
    ├── lib.rs -> <workspace>/build/rcl_interfaces/rosidl_generator_rs/rcl_interfaces/rust/src/lib.rs
    ├── msg.rs -> <workspace>/build/rcl_interfaces/rosidl_generator_rs/rcl_interfaces/rust/src/msg.rs
    └── srv.rs -> <workspace>/build/rcl_interfaces/rosidl_generator_rs/rcl_interfaces/rust/src/srv.rs

If my understanding is correct, then is this really a problem? I was unable to verify breakage on my end as they were already yanked before then. If we did see breakage, then it seems like there could be a bug in here

[esteve]

@maspe36 yeah, I think that we're ok for now, however the fact that builtin_interfaces and the other message packages were registered on crates.io tells us that our message generation story is not clear to users. The rationale for registering those crates was founded on the perception that our message generation pipeline is broken and needed fixing, which is not an entirely wrong assessment.

Right now we generate local crates and patch our dependencies via config.toml to pretend those crates exist. It's still confusing and perhaps something more explicit would help make it clear that messages are not meant to be treated as regular crates (i.e. don't publish them, don't add them to Cargo.toml, etc.), hence why I think that delegating access to messages to a single crate might mitigate it, additionally it might allow people to publish their projects as crates if they want, as the dependencies won't be local. In the end it's a problem of merging two dependency systems, so it'll never be easy.