feat!: include only requested interface packages - #10
Open
azerupi wants to merge 1 commit into
Open
Conversation
Union `[package.metadata.ros-env] interfaces` from the consuming Cargo graph and include that closure instead of every generated crate on AMENT_PREFIX_PATH. Assisted-by: Cursor Grok 4.6 [Cursor] Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The code changes in this PR have been assisted by Cursor but I have reviewed and iterated on the code.
This is an experiment in response to multiple discussions we've had about this and PR #6 and your comments on it.
ros-envcurrently includes every generated interface crate it can find. This PR makes packages declare the set of interface packages they want explicitly. A crate declares the interface packages it needs in itsCargo.tomland all dependencies do the same and thenros-envincludes the union of what the whole dependency graph asked for.I also think these changes would pair very well with our desire to use a build-time interface generator.
Problem
Today the selection is every interface package that ros-env can find under
share/<package>/rustonAMENT_PREFIX_PATH.On my setup on a (I believe) close to stock Jazzy install there are 39 generated interface crates that contain 161 files for about ~89k lines of Rust. This all gets
include!()ed in ros-env.This has two consequences:
Solution
Declare what you use:
We can't do it with features, because the set of interface packages is open-ended and features are static. Therefore we use a metadata section.
Every crate in the graph may declare dependencies on interface packages that way and
ros-envincludes the union. A library names only what it needs, an application that pulls in three such libraries gets all three sets without restating them.A requested package that is not on
AMENT_PREFIX_PATHfails the build and names who asked for it, rather than silently omitting a module:High-level steps
The build script:
<prefix>/share/<package>/rust/Cargo.toml, first prefix on the search path wins, so overlays shadow underlays.cargo metadata --filter-platform <TARGET>against it and unions every[package.metadata.ros-env] interfacesplus the workspace-level table.*version requirementrosidl_generator_rsemits for them.$OUT_DIR/interfaces.rs, whichsrc/lib.rsincludes.cargo metadataruns with the workspace as the working directory so.cargo/config.tomland[patch]tables are picked up.If ROS is not sourced it will not generate a build error in
ros-envitself,ros_envwill be empty and the build emits a warning. Of course any crate using messages fromros-envwill fail to build.Locating the workspace
Cargo does not tell a dependency's build script which workspace is being built (rust-lang/cargo#3946). We derive it from
OUT_DIRwhen the target dir is in the cargo workspace which happens when building with cargo. But when building with colcon the layout is not recognised and it will fail. We could expand the heuristics but I opted to use a trick fromembuild::cargo::workspace_dir(esp-idf-sys).Adding this in a
.cargo/config.tomlin the package allows us to deterministically find the workspace that is built.colcon sets
--target-diroutside the source tree, socolcon buildneedsCARGO_WORKSPACE_DIR.colcon-ros-cargocould set it automatically.Breaking changes
Consumers must declare
[package.metadata.ros-env] interfaces. A graph that declares nothing gets an emptyros_envand a warning. I have considered being more backward compatible by falling back to including everything if the package or one of its dependencies didn't include this metadata. But I feel like this will just make the transition period last much longer so sinceros-envis still new and people are migrating to it it is better to force the new way immediately.The
[package.metadata.ros-env] include = trueopt-in on generated crates is gone. Discovery is now purely "is there ashare/<pkg>/rust/Cargo.toml".Builds where the target directory is outside the workspace now need
CARGO_WORKSPACE_DIRchange mentioned above.Known limitations
Two semver-incompatible
ros-envversions in one graph still produce mutually incompatible types, even from identical.msgdefinitions. I don't think there is any way around this.