-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Isolate build script metadata progation between std and non-std crates #16489
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
Conversation
| // If the depenency is a build-std dependency, it might not be in | ||
| // Cargo.toml. If its not present, we simply use the package name as we | ||
| // know its not renamed. |
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.
See rust-lang/rust#150739 (comment)
Though they are not using any dep link metadata at this moment.
(I am a bit paranoid, sorry 😬)
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 guess we can go the .filter() route mentioned in rust-lang/rust#150739 (comment) for now just to be safe?
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.
That disables dep metadata propagation for the entire std graph, no?
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.
Probably this filter(|dep| dep.unit.is_std == unit.is_std)?
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 so allow propagation for std crate -> std crate but not std crate -> user crate?
I wonder if any std crates use the build std magic not include the dependencies in their Cargo.toml?
Hopefully not?
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 wonder if any std crates use the build std magic not include the dependencies in Cargo.toml?
I guess not. std bootstrap doesn't use -Zbuild-std. It would be awkward if someone forked std and relied on -Zbuild-std attaching std root units to std units itself.
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.
Updated the impl to add the std/nonstd boundary, as well as the PR tilte+description.
91f7862 to
d5c1597
Compare
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.
Thanks!
I guess it is fine we have no test for this because we are seeing more -Zbuild-std test in rust-lang/rust
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?
This PR fixes a regression found in rust-lang/rust#150739 that was introduced in #16436.
For
-Zbuild-stdstd dependencies, we would panic due to the dependency not being present inCargo.toml.This PR adds handling fallback to the(see #16489 (comment))unit.pkg.name()if the unit both not present inCargo.tomlandis_std.This PR ensures that metadata propagation is only allowed between
std->stdcrates andnon-std->non-stdcrates.How to test and review this PR?
Previously we panic with
with this change we not compile successfully
r? @weihanglo