From ca5961462055f52882a3ef44fd531158c06779df Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Sun, 15 Dec 2024 19:01:30 -0500 Subject: [PATCH 1/4] test(build-std): Isolate output test to avoid spurious [BLOCKING] messages from concurrent runs 47c2095b1dd580a91e42cb6197b58a318526b8c4 didn't really fix the flakiness. build-std tests use the users `CARGO_HOME` for downloading registry dependencies of the standard library. This reduces disk needs of the tests, speeds up the tests, and reduces the number of network requests that could fail. However, this means all of the tests access the same locks for the package cache. In one test, we assert on the output and a `[BLOCKING]` message can show up, depending on test execution time from concurrent test runs. We are going to hack around this by having the one test that asserts on test output to use the standard `cargo-test-support` `CARGO_HOME`, rather than the users `CARGO_HOME`. There will then only be one process accessing the lock and no `[BLOCKING]` messages. --- tests/build-std/main.rs | 49 +++++++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/tests/build-std/main.rs b/tests/build-std/main.rs index f8d5e748ac5..32771a01a19 100644 --- a/tests/build-std/main.rs +++ b/tests/build-std/main.rs @@ -25,9 +25,11 @@ use cargo_test_support::{basic_manifest, paths, project, rustc_host, str, Execs} use std::env; use std::path::Path; -fn enable_build_std(e: &mut Execs, arg: Option<&str>) { - e.env_remove("CARGO_HOME"); - e.env_remove("HOME"); +fn enable_build_std(e: &mut Execs, arg: Option<&str>, isolated: bool) { + if !isolated { + e.env_remove("CARGO_HOME"); + e.env_remove("HOME"); + } // And finally actually enable `build-std` for now let arg = match arg { @@ -40,19 +42,41 @@ fn enable_build_std(e: &mut Execs, arg: Option<&str>) { // Helper methods used in the tests below trait BuildStd: Sized { + /// Set `-Zbuild-std` args and will download dependencies of the standard + /// library in users's `CARGO_HOME` (`~/.cargo/`) instead of isolated + /// environment `cargo-test-support` usually provides. + /// + /// The environment is not isolated is to avoid excessive network requests + /// and downloads. A side effect is `[BLOCKING]` will show up in stderr, + /// as a sign of package cahce lock contention when running other build-std + /// tests concurrently. fn build_std(&mut self) -> &mut Self; + + /// Like [`BuildStd::build_std`] and is able to specify what crates to build. fn build_std_arg(&mut self, arg: &str) -> &mut Self; + + /// Like [`BuildStd::build_std`] but use an isolated `CARGO_HOME` environment + /// to avoid package cache lock contention. + /// + /// Don't use this unless you really need to assert the full stderr + /// and avoid any `[BLOCKING]` message. + fn build_std_isolated(&mut self) -> &mut Self; fn target_host(&mut self) -> &mut Self; } impl BuildStd for Execs { fn build_std(&mut self) -> &mut Self { - enable_build_std(self, None); + enable_build_std(self, None, false); self } fn build_std_arg(&mut self, arg: &str) -> &mut Self { - enable_build_std(self, Some(arg)); + enable_build_std(self, Some(arg), false); + self + } + + fn build_std_isolated(&mut self) -> &mut Self { + enable_build_std(self, None, true); self } @@ -107,9 +131,12 @@ fn basic() { ) .build(); - p.cargo("check").build_std().target_host().run(); + // HACK: use an isolated the isolated CARGO_HOME environment (`build_std_isolated`) + // to avoid `[BLOCKING]` messages (from lock contention with other tests) + // from getting in this test's asserts + p.cargo("check").build_std_isolated().target_host().run(); p.cargo("build") - .build_std() + .build_std_isolated() .target_host() // Importantly, this should not say [UPDATING] // There have been multiple bugs where every build triggers and update. @@ -120,7 +147,7 @@ fn basic() { "#]]) .run(); p.cargo("run") - .build_std() + .build_std_isolated() .target_host() .with_stderr_data(str![[r#" [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -129,7 +156,7 @@ fn basic() { "#]]) .run(); p.cargo("test") - .build_std() + .build_std_isolated() .target_host() .with_stderr_data(str![[r#" [COMPILING] rustc-std-workspace-std [..] @@ -379,13 +406,11 @@ fn test_proc_macro() { .file("src/lib.rs", "") .build(); - // Download dependencies first, - // so we can compare `cargo test` output without any wildcard - p.cargo("fetch").build_std().run(); p.cargo("test --lib") .env_remove(cargo_util::paths::dylib_path_envvar()) .build_std() .with_stderr_data(str![[r#" +... [COMPILING] foo v0.0.0 ([ROOT]/foo) [FINISHED] `test` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s [RUNNING] unittests src/lib.rs (target/debug/deps/foo-[HASH]) From cbdbfe4ecd6a83c65aba72800abe2d5549058090 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 16 Dec 2024 23:09:05 -0500 Subject: [PATCH 2/4] refactor(build-std): extract support core only detection This is a preparation of reuse for subsequent fix. --- src/cargo/core/compiler/build_context/target_info.rs | 10 ++++++++++ src/cargo/core/compiler/standard_lib.rs | 8 ++++---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/compiler/build_context/target_info.rs b/src/cargo/core/compiler/build_context/target_info.rs index e03c76d6bbf..1844073ae45 100644 --- a/src/cargo/core/compiler/build_context/target_info.rs +++ b/src/cargo/core/compiler/build_context/target_info.rs @@ -619,6 +619,16 @@ impl TargetInfo { .iter() .any(|sup| sup.as_str() == split.as_str()) } + + /// Checks if a target maybe support std. + /// + /// If no explictly stated in target spec json, we treat it as "maybe support". + /// + /// This is only useful for `-Zbuild-std` to determine the default set of + /// crates it is going to build. + pub fn maybe_support_std(&self) -> bool { + matches!(self.supports_std, Some(true) | None) + } } /// Takes rustc output (using specialized command line args), and calculates the file prefix and diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index 5e9fe6af494..c135a5d6d73 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -115,10 +115,10 @@ pub fn generate_std_roots( ) -> CargoResult>> { // Generate a map of Units for each kind requested. let mut ret = HashMap::new(); - let (core_only, maybe_std): (Vec<&CompileKind>, Vec<_>) = kinds.iter().partition(|kind| - // Only include targets that explicitly don't support std - target_data.info(**kind).supports_std == Some(false)); - for (default_crate, kinds) in [("core", core_only), ("std", maybe_std)] { + let (maybe_std, maybe_core): (Vec<&CompileKind>, Vec<_>) = kinds + .iter() + .partition(|kind| target_data.info(**kind).maybe_support_std()); + for (default_crate, kinds) in [("core", maybe_core), ("std", maybe_std)] { if kinds.is_empty() { continue; } From f004691aa41449fa06b764683385558edee51428 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Sun, 15 Dec 2024 15:23:41 -0500 Subject: [PATCH 3/4] test(build-std): resolve too less deps This failed because since 125e873dffc4b68b263c5decd88750ec10fd441e [`std_resolve`][1] only includes `sysroot` as primary package. When any custom Cargo feature is provided via `-Zbuild-std-feature`, the default feature set `panic-unwind` would be gone, so no `panic_unwind` crate presents in `std_resolve`. When then calling [`std_resolve.query`][2] with the default set of crates from [`std_crates`][3], which automatically includes `panic_unwind` when `std` presents, it'll result in spec not found because `panic_unwind` was not in `std_resolve` anyway. [1]: https://github.com/rust-lang/cargo/blob/addcc8ca715bc7fe20df66afd6efbf3c77ef43f8/src/cargo/core/compiler/standard_lib.rs#L96 [2]: https://github.com/rust-lang/cargo/blob/addcc8ca715bc7fe20df66afd6efbf3c77ef43f8/src/cargo/core/compiler/standard_lib.rs#L158 [3]: https://github.com/rust-lang/cargo/blob/addcc8ca715bc7fe20df66afd6efbf3c77ef43f8/src/cargo/core/compiler/standard_lib.rs#L156 See rust-lang/cargo#14935 --- tests/build-std/main.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/build-std/main.rs b/tests/build-std/main.rs index 32771a01a19..800495da340 100644 --- a/tests/build-std/main.rs +++ b/tests/build-std/main.rs @@ -418,3 +418,30 @@ fn test_proc_macro() { "#]]) .run(); } + +#[cargo_test(build_std_real)] +fn test_panic_abort() { + // See rust-lang/cargo#14935 + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + edition = "2021" + "#, + ) + .file("src/lib.rs", "#![no_std]") + .build(); + + p.cargo("check") + .build_std_arg("std,panic_abort") + .env("RUSTFLAGS", "-C panic=abort") + .arg("-Zbuild-std-features=panic_immediate_abort") + .with_status(101) + .with_stderr_data(str![[r#" +[ERROR] package ID specification `panic_unwind` did not match any packages + +"#]]) + .run(); +} From 0149bca5cc8cbdc98089103971fb6530fd037f95 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 16 Dec 2024 23:23:36 -0500 Subject: [PATCH 4/4] fix(build-std): behavior revert of 125e873dffc4b68b This is kinda a revert of 125e873dffc4b68b263c5decd88750ec10fd441e in terms of the behavior. After this, now `std_resolve` is always resolved by the same set of packages that Cargo will use to generate the unit graph, (technically the same set of crates + `sysroot`), by sharing the same set of primary packages via `std_crates` functions. --- src/cargo/core/compiler/standard_lib.rs | 23 +++++++++++++++++++---- src/cargo/ops/cargo_compile/mod.rs | 11 ++++++++--- src/cargo/ops/cargo_fetch.rs | 10 ++++++++-- tests/build-std/main.rs | 5 ----- 4 files changed, 35 insertions(+), 14 deletions(-) diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index c135a5d6d73..18253b5d465 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/src/cargo/core/compiler/standard_lib.rs @@ -44,10 +44,14 @@ fn std_crates<'a>(crates: &'a [String], default: &'static str, units: &[Unit]) - } /// Resolve the standard library dependencies. +/// +/// * `crates` is the arg value from `-Zbuild-std`. pub fn resolve_std<'gctx>( ws: &Workspace<'gctx>, target_data: &mut RustcTargetData<'gctx>, build_config: &BuildConfig, + crates: &[String], + kinds: &[CompileKind], ) -> CargoResult<(PackageSet<'gctx>, Resolve, ResolvedFeatures)> { if build_config.build_plan { ws.gctx() @@ -65,10 +69,21 @@ pub fn resolve_std<'gctx>( // `[dev-dependencies]`. No need for us to generate a `Resolve` which has // those included because we'll never use them anyway. std_ws.set_require_optional_deps(false); - // `sysroot` + the default feature set below should give us a good default - // Resolve, which includes `libtest` as well. - let specs = Packages::Packages(vec!["sysroot".into()]); - let specs = specs.to_package_id_specs(&std_ws)?; + let specs = { + // If there is anything looks like needing std, resolve with it. + // If not, we assume only `core` maye be needed, as `core the most fundamental crate. + // + // This may need a UI overhaul if `build-std` wants to fully support multi-targets. + let maybe_std = kinds + .iter() + .any(|kind| target_data.info(*kind).maybe_support_std()); + let mut crates = std_crates(crates, if maybe_std { "std" } else { "core" }, &[]); + // `sysroot` is not in the default set because it is optional, but it needs + // to be part of the resolve in case we do need it or `libtest`. + crates.insert("sysroot"); + let specs = Packages::Packages(crates.into_iter().map(Into::into).collect()); + specs.to_package_id_specs(&std_ws)? + }; let features = match &gctx.cli_unstable().build_std_features { Some(list) => list.clone(), None => vec![ diff --git a/src/cargo/ops/cargo_compile/mod.rs b/src/cargo/ops/cargo_compile/mod.rs index c89c064c780..04520d64340 100644 --- a/src/cargo/ops/cargo_compile/mod.rs +++ b/src/cargo/ops/cargo_compile/mod.rs @@ -289,9 +289,14 @@ pub fn create_bcx<'a, 'gctx>( resolved_features, } = resolve; - let std_resolve_features = if gctx.cli_unstable().build_std.is_some() { - let (std_package_set, std_resolve, std_features) = - standard_lib::resolve_std(ws, &mut target_data, &build_config)?; + let std_resolve_features = if let Some(crates) = &gctx.cli_unstable().build_std { + let (std_package_set, std_resolve, std_features) = standard_lib::resolve_std( + ws, + &mut target_data, + &build_config, + crates, + &build_config.requested_kinds, + )?; pkg_set.add_set(std_package_set); Some((std_resolve, std_features)) } else { diff --git a/src/cargo/ops/cargo_fetch.rs b/src/cargo/ops/cargo_fetch.rs index b1e596f6485..4ff89491d86 100644 --- a/src/cargo/ops/cargo_fetch.rs +++ b/src/cargo/ops/cargo_fetch.rs @@ -64,8 +64,14 @@ pub fn fetch<'a>( } // If -Zbuild-std was passed, download dependencies for the standard library. - if gctx.cli_unstable().build_std.is_some() { - let (std_package_set, _, _) = standard_lib::resolve_std(ws, &mut data, &build_config)?; + if let Some(crates) = &gctx.cli_unstable().build_std { + let (std_package_set, _, _) = standard_lib::resolve_std( + ws, + &mut data, + &build_config, + crates, + &build_config.requested_kinds, + )?; packages.add_set(std_package_set); } diff --git a/tests/build-std/main.rs b/tests/build-std/main.rs index 800495da340..976fc7e141b 100644 --- a/tests/build-std/main.rs +++ b/tests/build-std/main.rs @@ -438,10 +438,5 @@ fn test_panic_abort() { .build_std_arg("std,panic_abort") .env("RUSTFLAGS", "-C panic=abort") .arg("-Zbuild-std-features=panic_immediate_abort") - .with_status(101) - .with_stderr_data(str![[r#" -[ERROR] package ID specification `panic_unwind` did not match any packages - -"#]]) .run(); }