-
Notifications
You must be signed in to change notification settings - Fork 127
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
Prevent message packages from crates.io to be included during the build #394
Comments
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
|
@mxgrey
It'd have code to read features from |
The problem is that no matter what, the interaction between 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. |
Also, a nice bonus of this solution is that I think we'd no longer need to vendorize messages. |
I like avoiding needing to vendorize messages. That should hopefully make things a bit easier on the end user as well... |
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 |
I'm not seeing anything on passing arbitrary metadata to a dependency, but I'll try to keep looking... |
I don't suppose the |
Nice find! |
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 |
I'm leaning towards adding this functionality to a build script. Or perhaps to the cargo ament plugin, which could read the dependencies from |
I like the idea of reading the dependencies from the |
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. |
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 One of the main jobs for this plugin, is to create this An example snippet from my [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
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 |
@maspe36 yeah, I think that we're ok for now, however the fact that Right now we generate local crates and patch our dependencies via |
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:
include!()
on those filesros2_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?
The text was updated successfully, but these errors were encountered: