fix(build-std): determine root crates by target spec `std:bool` #14899

weihanglo · 2024-12-05T22:47:32Z

What does this PR try to resolve?

In #14183 Cargo starts bailing out if the metadata.std
field in a target spec JSON is set to false.
This is problematic because std for some targets are actually buildable
even they've declared as std-unsupported.

This patch removes the hard error, and instead determines the required
root crates by the metadata.std field. That is to say, if a target
is explicitly declared as std: false, -Zbuild-std will build core
and compiler-builtins only, no std will be built.

This patch doesn't change the behavior of -Zbuild-std with explicit
crates set. For example -Zbuild-std=std will force building std.

See Zulip discussion:
https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/help.20debugging.20a.20docs.2Ers.20issue.20with.20a.20new.20cargo.20error

How should we test and review this PR?

e18b64f and 125e873 might need a closer look.
Cargo relies on how std workspace is organized with these commits.

Here is an e2e test, if you are willing to test manually.

Use the current nightly toolchain rustc 1.85.0-nightly (acabb5248 2024-12-04)
rustup component add rust-src --toolchain nightly
rustup target add aarch64-unknown-none --toolchain nightly
Create a #![no_std] lib package.
Run target/debug/cargo build -Zbuild-std --target aarch64-unknown-none in that package. Previously it failed with a message building std is not supported on the following targets

The only functionality of `standard_lib::std_crates` is adding `test` to the requested crate list. `resolve_std` already unconditionally adds `sysroot` to the list, which always includes `test` in dependency graph. Therefore calling `std_crates` is redundant.

rustbot · 2024-12-05T22:47:37Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

weihanglo · 2024-12-05T22:57:13Z

tests/testsuite/standard_lib.rs

-fn test_std_on_unsupported_target() {
+#[cargo_test(build_std_mock, requires = "rustup")]
+fn build_std_with_no_arg_for_core_only_target() {
+    let has_rustup_aarch64_unknown_none = std::process::Command::new("rustup")


I hate this. This is added to my list of cargo-test-macro overhaul.

Overhaul cargo-test-macro or work on T-testing-devex to make it not needed anymore ;)

We have a standard pattern for alternative targets in cargo_test_support::cross_compile, should we tie into that?

That is actually a good idea, though I am not sure. The probe is based on building a simple cross-compile project. I don't want to disable all cross-compile test locally just because aarch64-unknow-none is missing.

We could have a cargo_test_support::build_std with its own disabled. More so I was wanting to highlight that pattern for consideration.

Yeah. I'll keep this in mind. If we get any new tests with the same pattern we can extract the common part then.

Instead of altering the original `-Zbuild-std` arg value, this defers the timing of detereming which crate to build to when Cargo generates std root units.

sysroot depends on `std`, `proc_macro`, and `test` [1], which includes everything we need from the default set of crates. Passing `sysroot` alone as spec should produce the same resolution. [1]: https://github.com/rust-lang/rust/blob/acabb5248231987ae1f0c215208d1005a5db402d/library/sysroot/Cargo.toml#L7-L11

Will let `std:bool` to determine necessary deps for `-Zbuild-std`, instead of erroring out.

In rust-lang#14183 Cargo starts bailing out if the `metadata.std` field in a target spec JSON is set to `false`. This is problematic because std for some targets are actually buildable even they've declared as std-unsupported. This patch removes the hard error, and instead determines the required root crates by the `metadata.std` field. That is to say, if a target is explicitly declared as `std: false`, `-Zbuild-std` will build `core` and `compiler-builtins` only, no `std` will be built. This patch doesn't change the behavior of `-Zbuild-std` with explicit crates set. For example `-Zbuild-std=std` will force building `std`. See Zulip discussion: https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/help.20debugging.20a.20docs.2Ers.20issue.20with.20a.20new.20cargo.20error

epage · 2024-12-06T15:51:24Z

Cargo relies on how std workspace is organized with these commits.

Could you expand on this. I didn't quite get how some of those parts fit together and tie back to this. Is this the sysroot stuff? How much of a problem is that?

weihanglo · 2024-12-06T21:09:17Z

Cargo relies on how std workspace is organized with these commits.

Could you expand on this. I didn't quite get how some of those parts fit together and tie back to this. Is this the sysroot stuff? How much of a problem is that?

It will be a problem if sysroot is removed from std workspace, or stop depending on std, or a Cargo features changes and one dependency is not activated anymore. The result of std dependency resolution will fail or miss important dependencies due to these reasons. This change makes it a bit more fragile because Cargo got broken whenever sysroot is broken, which before this we might have a little chance to build a half-broken std.

Update cargo 18 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..7847c03965260b5dcc8d93218d6af295a717abb6 2024-12-06 21:56:56 +0000 to 2024-12-13 18:06:39 +0000 - fix(base): Support bases in patches in virtual manifests (rust-lang/cargo#14931) - fix(resolver): Report invalid index entries (rust-lang/cargo#14927) - feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928) - fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923) - fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926) - fix(script): Don't override the release profile (rust-lang/cargo#14925) - feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917) - fix(resolver): Don't report all versions as rejected (rust-lang/cargo#14921) - fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897) - fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911) - a faster hash for ActivationsKey (rust-lang/cargo#14915) - feat(build-script): Pass CARGO_CFG_FEATURE (rust-lang/cargo#14902) - fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913) - chore: update auto-label to include build-rs crate (rust-lang/cargo#14912) - refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908) - feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910) - fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899) - SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)

Update cargo 19 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..769f622e12db0001431d8ae36d1093fb8727c5d9 2024-12-06 21:56:56 +0000 to 2024-12-14 04:27:35 +0000 - test(build-std): dont require rustup (rust-lang/cargo#14933) - fix(base): Support bases in patches in virtual manifests (rust-lang/cargo#14931) - fix(resolver): Report invalid index entries (rust-lang/cargo#14927) - feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928) - fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923) - fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926) - fix(script): Don't override the release profile (rust-lang/cargo#14925) - feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917) - fix(resolver): Don't report all versions as rejected (rust-lang/cargo#14921) - fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897) - fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911) - a faster hash for ActivationsKey (rust-lang/cargo#14915) - feat(build-script): Pass CARGO_CFG_FEATURE (rust-lang/cargo#14902) - fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913) - chore: update auto-label to include build-rs crate (rust-lang/cargo#14912) - refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908) - feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910) - fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899) - SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)

weihanglo added 2 commits December 5, 2024 13:41

refactor: rename parse_feature fn

c77edbf

rustbot assigned ehuss Dec 5, 2024

rustbot added A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-unstable Area: nightly unstable support Command-fetch S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2024

weihanglo commented Dec 5, 2024

View reviewed changes

weihanglo force-pushed the build-std branch from d744afe to f25bf82 Compare December 6, 2024 04:01

weihanglo added 4 commits December 5, 2024 23:15

refactor(build-std): defer std crates to build

30d11ce

Instead of altering the original `-Zbuild-std` arg value, this defers the timing of detereming which crate to build to when Cargo generates std root units.

fix(build-std): remove std unsupported check

1cd370c

Will let `std:bool` to determine necessary deps for `-Zbuild-std`, instead of erroring out.

weihanglo force-pushed the build-std branch from f25bf82 to feb398a Compare December 6, 2024 04:19

epage approved these changes Dec 9, 2024

View reviewed changes

epage added this pull request to the merge queue Dec 9, 2024

Merged via the queue into rust-lang:master with commit 58c2374 Dec 9, 2024
20 checks passed

weihanglo deleted the build-std branch December 10, 2024 16:27

weihanglo mentioned this pull request Dec 13, 2024

Update cargo rust-lang/rust#134278

Merged

rustbot added this to the 1.85.0 milestone Dec 14, 2024

weihanglo mentioned this pull request Dec 15, 2024

error: package ID specification panic_unwind did not match any packages with -Zbuild-std #14935

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(build-std): determine root crates by target spec `std:bool` #14899

fix(build-std): determine root crates by target spec `std:bool` #14899

weihanglo commented Dec 5, 2024 •

edited

Loading

rustbot commented Dec 5, 2024

weihanglo Dec 5, 2024

epage Dec 6, 2024

epage Dec 6, 2024

weihanglo Dec 6, 2024

epage Dec 6, 2024

weihanglo Dec 7, 2024

epage commented Dec 6, 2024

weihanglo commented Dec 6, 2024

fix(build-std): determine root crates by target spec std:bool #14899

fix(build-std): determine root crates by target spec std:bool #14899

Conversation

weihanglo commented Dec 5, 2024 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Dec 5, 2024

weihanglo Dec 5, 2024

Choose a reason for hiding this comment

epage Dec 6, 2024

Choose a reason for hiding this comment

epage Dec 6, 2024

Choose a reason for hiding this comment

weihanglo Dec 6, 2024

Choose a reason for hiding this comment

epage Dec 6, 2024

Choose a reason for hiding this comment

weihanglo Dec 7, 2024

Choose a reason for hiding this comment

epage commented Dec 6, 2024

weihanglo commented Dec 6, 2024

fix(build-std): determine root crates by target spec `std:bool` #14899

fix(build-std): determine root crates by target spec `std:bool` #14899

weihanglo commented Dec 5, 2024 •

edited

Loading