-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Any build scripts can now use cargo::metadata=KEY=VALUE #16436
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
Any build scripts can now use cargo::metadata=KEY=VALUE #16436
Conversation
47d8a0f to
c654a94
Compare
| assert_eq!(env::var("CARGO_DEP_A_FOO").unwrap(), "bar"); | ||
| assert_eq!(env::var("CARGO_DEP_A_BAR").unwrap(), "baz"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, one unresolved question is what to name this env variable
- Technically,
name_keyis ambiguous.foo_bar_alice_bobcould befoo_bar+alice_bob,foo+bar_alice_bob, etc and technically packages could conflict - Whether we should
envifythe package name and key or not. iirc we do it in some places but not others. Overall, I would prefer we would never do it so it matches the name people see inCargo.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Technically,
name_keyis ambiguous.foo_bar_alice_bobcould befoo_bar+alice_bob,foo+bar_alice_bob, etc and technically packages could conflict
I suppose one idea would be to using another delimiter between the name and key.
Maybe double underscore? foo_bar__alice_bob would now be different than foo__bar_alice_bob.
Not sure if there are any other delimiters that are compatible across enough platforms. I think POSIX is already pretty restrictive with what can be used an env var name.
- Whether we should
envifythe package name and key or not. iirc we do it in some places but not others. Overall, I would prefer we would never do it so it matches the name people see inCargo.toml
Given that env vars from cargo::metadata=KEY=VALUE are already envify'd in DEP_<links>_<key> I think it makes sense to keep that? I think it'd be more confusing if cargo::metadata=KEY=VALUE had multiple behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I highlighted these for being called out in the stabilization conversations and do not block this PR. I don't have concrete ideas to say exactly what should be done but want to raise these for a wider conversation.
00fac87 to
7253b99
Compare
| if dep.unit.mode.is_run_custom_build() { | ||
| let dep_metadata = build_runner.get_run_build_script_metadata(&dep.unit); | ||
|
|
||
| // FIXME: Do we need to handle dev-dependencies and/or build-dependencies? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling out for resolving
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do whatever we do for links
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent some time testing this and as far as I can tell links only propagates env vars set with cargo::metadata if its a regular dependency.
I can see we explicitly filter out dev-dependencies in:
cargo/src/cargo/core/compiler/unit_dependencies.rs
Lines 987 to 990 in 577ab6f
| // Skip dependencies induced via dev-dependencies since | |
| // connections between `links` and build scripts only happens | |
| // via normal dependencies. Otherwise since dev-dependencies can | |
| // be cyclic we could have cyclic build-script executions. |
and I believe that build-dependencies are implicitly not included when the unit graph is built.
I removed the FIXME comment, but lmk if you think there is something I missed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably have tests showing the behavior for both ways of accessing metadata. Here is what I used for experimenting:
#[cargo_test]
fn metadata_from_dep_kinds() {
let set_metadata = r#"
fn main() {
println!("cargo::metadata=key=value");
}
"#;
let get_metadata = r#"
fn main() {
println!("cargo::warning={:?}", std::env::var("DEP_FOO_KEY"));
}
"#;
let p = project()
.file(
"Cargo.toml",
r#"
[workspace]
resolver = "3"
members = ["links", "n", "b", "d"]
"#,
)
.file(
"links/Cargo.toml",
r#"
[package]
name = "links"
edition = "2024"
links = "foo"
"#,
)
.file("links/src/lib.rs", "")
.file("links/build.rs", set_metadata)
.file(
"n/Cargo.toml",
r#"
[package]
name = "n"
edition = "2024"
[dependencies]
links.path = "../links"
"#,
)
.file("n/src/lib.rs", "")
.file("n/build.rs", get_metadata)
.file(
"b/Cargo.toml",
r#"
[package]
name = "b"
edition = "2024"
[build-dependencies]
links.path = "../links"
"#,
)
.file("b/src/lib.rs", "")
.file("b/build.rs", get_metadata)
.file(
"d/Cargo.toml",
r#"
[package]
name = "d"
edition = "2024"
[dev-dependencies]
links.path = "../links"
"#,
)
.file("d/src/lib.rs", "")
.file("d/build.rs", get_metadata)
.build();
p.cargo("check --all-targets")
.with_stderr_data(
str![[r#"
[COMPILING] links v0.0.0 ([ROOT]/foo/links)
[COMPILING] n v0.0.0 ([ROOT]/foo/n)
[COMPILING] b v0.0.0 ([ROOT]/foo/b)
[COMPILING] d v0.0.0 ([ROOT]/foo/d)
[WARNING] n@0.0.0: Ok("value")
[WARNING] b@0.0.0: Err(NotPresent)
[WARNING] d@0.0.0: Err(NotPresent)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
"#]]
.unordered(),
)
.run();
}just needs the new CARGO_DEP env variables added to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh thanks, using cargo::warning so we can leverage the snapbox testing infra is clever.
I added this test to the test commit and modified it to include CARGO_DEP_LINKS_KEY in the feat commit.
Remaining `build`s: - linker is potentially involved - testing related to dev-dependencies (didn't look to see if `check --benches` could replace `bench`) Similarly, some `run`s` were removed so long as compilation verified the same details. Inspired by rust-lang#16436 which is adding more tests which don't need `build` but copied the existing ones.
### What does this PR try to resolve? Reduce test overhead by changing `build`s to `check`s. Similarly, some `run`s` were removed so long as compilation verified the same details. Inspired by #16436 which is adding more tests which don't need `build` but copied the existing ones. ### How to test and review this PR? Remaining `build`s: - linker is potentially involved - testing related to dev-dependencies (didn't look to see if `check --benches` could replace `bench`)
7253b99 to
180d3c5
Compare
This comment has been minimized.
This comment has been minimized.
| d.package_name() == dep.unit.pkg.name() | ||
| && d.source_id() == dep.unit.pkg.package_id().source_id() | ||
| && d.version_req().matches(unit.pkg.version()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that while investigating #16436 (comment) I noticed that only checking the package name could lead to inconsistent behavior if a package has 2 versions of a package imported under different names.
To mitigate this I added checks for source_id and version. This is close to comparing package_id (Dependency does not have package_id)
Note that Dependency only has a version requirement so we use .matches(). I think in practice we should only have 1 possible matching version as Cargo would error while building the unit graph if the same dependencies. (ref)
correct me if I am wrong
This comment has been minimized.
This comment has been minimized.
180d3c5 to
a919c32
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Update cargo submodule 19 commits in 94c368ad2b9db0f0da5bdd8421cea13786ce4412..623444427f04292f3a3c7eac560cfa65ba0bb88e 2025-12-26 19:39:15 +0000 to 2026-01-06 21:05:05 +0000 - docs(unstable): expand docs for `-Zbuild-analysis` (rust-lang/cargo#16476) - test: add `-Zunstable-options` with custom targets (rust-lang/cargo#16467) - feat(report): add cargo report rebuilds (rust-lang/cargo#16456) - feat(test-support): Use test name for dir when running tests (rust-lang/cargo#16121) - refactor: Migrate some cases to expect/reason (rust-lang/cargo#16461) - docs(build-script): clarify OUT_DIR is not cleaned between builds (rust-lang/cargo#16437) - chore: Update dependencies (rust-lang/cargo#16460) - Update handlebars to 6.4.0 (rust-lang/cargo#16457) - chore(deps): update alpine docker tag to v3.23 (rust-lang/cargo#16454) - Any build scripts can now use cargo::metadata=KEY=VALUE (rust-lang/cargo#16436) - fix(log): add `dependencies` field to `UnitRegistered` (rust-lang/cargo#16448) - Implement fine grain locking for `build-dir` (rust-lang/cargo#16155) - feat(resolver): List features when no close match (rust-lang/cargo#16445) - feat(report): new command `cargo report sessions` (rust-lang/cargo#16428) - feat (patch): Display where the patch was defined in patch-related error messages (rust-lang/cargo#16407) - test(build-rs): Reduce from 'build' to 'check' where possible (rust-lang/cargo#16444) - feat(toml): TOML 1.1 parse support (rust-lang/cargo#16415) - feat(report): support --manifest-path in `cargo report timings` (rust-lang/cargo#16441) - fix(vendor): recursively filter git files in subdirectories (rust-lang/cargo#16439) r? ghost
…lection (#16486) ### What does this PR try to resolve? Fixes a bug in the logic during (unit) dependency selection for build scripts introduced in #16436 This was discovered while updating the submodule in rust-lang/rust rust-lang/rust#150739 ### How to test and review this PR? ```sh cargo new foo cd foo cargo add cortex-m-semihosting cargo build ``` I plan to add test case for this in a follow up PR, but raising the fix PR to unblock the submodule update for now. r? @weihanglo
Update cargo submodule 24 commits in 94c368ad2b9db0f0da5bdd8421cea13786ce4412..fc4c92b64d1e0046b66cbdc747cc1c17af8b35a0 2025-12-26 19:39:15 +0000 to 2026-01-08 06:54:56 +0000 - Fixed incorrect version comparision during build script dependency selection (rust-lang/cargo#16486) - refactor: new type for unit index (rust-lang/cargo#16485) - feat(test): Make CARGO_BIN_EXE_ available at runtime (rust-lang/cargo#16421) - fix(package): detect dirty files when run from workspace member (rust-lang/cargo#16479) - fix(timing)!: remove `--timings=<FMT>` optional format values (rust-lang/cargo#16420) - docs(unstable): expand docs for `-Zbuild-analysis` (rust-lang/cargo#16476) - test: add `-Zunstable-options` with custom targets (rust-lang/cargo#16467) - feat(report): add cargo report rebuilds (rust-lang/cargo#16456) - feat(test-support): Use test name for dir when running tests (rust-lang/cargo#16121) - refactor: Migrate some cases to expect/reason (rust-lang/cargo#16461) - docs(build-script): clarify OUT_DIR is not cleaned between builds (rust-lang/cargo#16437) - chore: Update dependencies (rust-lang/cargo#16460) - Update handlebars to 6.4.0 (rust-lang/cargo#16457) - chore(deps): update alpine docker tag to v3.23 (rust-lang/cargo#16454) - Any build scripts can now use cargo::metadata=KEY=VALUE (rust-lang/cargo#16436) - fix(log): add `dependencies` field to `UnitRegistered` (rust-lang/cargo#16448) - Implement fine grain locking for `build-dir` (rust-lang/cargo#16155) - feat(resolver): List features when no close match (rust-lang/cargo#16445) - feat(report): new command `cargo report sessions` (rust-lang/cargo#16428) - feat (patch): Display where the patch was defined in patch-related error messages (rust-lang/cargo#16407) - test(build-rs): Reduce from 'build' to 'check' where possible (rust-lang/cargo#16444) - feat(toml): TOML 1.1 parse support (rust-lang/cargo#16415) - feat(report): support --manifest-path in `cargo report timings` (rust-lang/cargo#16441) - fix(vendor): recursively filter git files in subdirectories (rust-lang/cargo#16439)
#16489) ### What does this PR try to resolve? This PR fixes a regression found in rust-lang/rust#150739 that was introduced in #16436. For `-Zbuild-std` std dependencies, we would panic due to the dependency not being present in `Cargo.toml`. ~~This PR adds handling fallback to the `unit.pkg.name()` if the unit both not present in `Cargo.toml` and `is_std`.~~ (see #16489 (comment)) This PR ensures that metadata propagation is only allowed between `std->std` crates and `non-std->non-std` crates. ### How to test and review this PR? ``` cargo new foo cd foo cargo add cortex-m cargo -Zbuild-std build ``` Previously we panic with ``` thread 'main' (4072127) panicked at src/cargo/core/compiler/custom_build.rs:472:21: Dependency `compiler_builtins` not found in `foo`s dependencies note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` with this change we not compile successfully r? @weihanglo
Update cargo submodule 27 commits in 94c368ad2b9db0f0da5bdd8421cea13786ce4412..8c133afcd5e0d69932fe11f5907683723f8d361d 2025-12-26 19:39:15 +0000 to 2026-01-09 03:50:15 +0000 - Isolate build script metadata progation between std and non-std crates (rust-lang/cargo#16489) - Add Clippy like lint groups (rust-lang/cargo#16464) - feat: in-memory only `Manifest` (rust-lang/cargo#16409) - Fixed incorrect version comparision during build script dependency selection (rust-lang/cargo#16486) - refactor: new type for unit index (rust-lang/cargo#16485) - feat(test): Make CARGO_BIN_EXE_ available at runtime (rust-lang/cargo#16421) - fix(package): detect dirty files when run from workspace member (rust-lang/cargo#16479) - fix(timing)!: remove `--timings=<FMT>` optional format values (rust-lang/cargo#16420) - docs(unstable): expand docs for `-Zbuild-analysis` (rust-lang/cargo#16476) - test: add `-Zunstable-options` with custom targets (rust-lang/cargo#16467) - feat(report): add cargo report rebuilds (rust-lang/cargo#16456) - feat(test-support): Use test name for dir when running tests (rust-lang/cargo#16121) - refactor: Migrate some cases to expect/reason (rust-lang/cargo#16461) - docs(build-script): clarify OUT_DIR is not cleaned between builds (rust-lang/cargo#16437) - chore: Update dependencies (rust-lang/cargo#16460) - Update handlebars to 6.4.0 (rust-lang/cargo#16457) - chore(deps): update alpine docker tag to v3.23 (rust-lang/cargo#16454) - Any build scripts can now use cargo::metadata=KEY=VALUE (rust-lang/cargo#16436) - fix(log): add `dependencies` field to `UnitRegistered` (rust-lang/cargo#16448) - Implement fine grain locking for `build-dir` (rust-lang/cargo#16155) - feat(resolver): List features when no close match (rust-lang/cargo#16445) - feat(report): new command `cargo report sessions` (rust-lang/cargo#16428) - feat (patch): Display where the patch was defined in patch-related error messages (rust-lang/cargo#16407) - test(build-rs): Reduce from 'build' to 'check' where possible (rust-lang/cargo#16444) - feat(toml): TOML 1.1 parse support (rust-lang/cargo#16415) - feat(report): support --manifest-path in `cargo report timings` (rust-lang/cargo#16441) - fix(vendor): recursively filter git files in subdirectories (rust-lang/cargo#16439)
What does this PR try to resolve?
See #3544
-Zany-build-script-metadatafeatureCARGO_DEP_<name>_<key>env vars as described in DEP_FOO_KEY-like system that can work without "links" #3544 (comment) when-Zany-build-script-metadatais passedUnresolved questions
envify, see Any build scripts can now use cargo::metadata=KEY=VALUE #16436 (comment)How to test and review this PR?
See the e2e tests included in this PR.
As for the changes, they are not too bad.
meta: I picked this up to learn a bit more about the build script and unit graph code in Cargo.
r? @epage