From 41d79374930ba9db67467008fc318f5e8fd8b93d Mon Sep 17 00:00:00 2001 From: jyn Date: Wed, 19 Apr 2023 21:05:02 -0500 Subject: [PATCH 1/6] Remove unnecessary `test_kind` field and `TestKind` struct --- src/bootstrap/builder.rs | 8 ++++ src/bootstrap/builder/tests.rs | 1 - src/bootstrap/test.rs | 78 ++++++---------------------------- 3 files changed, 21 insertions(+), 66 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index 0d2d512b4b2ae..5b5e5fe6662f0 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -634,6 +634,14 @@ impl Kind { Kind::Suggest => "suggest", } } + + pub fn test_description(&self) -> &'static str { + match self { + Kind::Test => "Testing", + Kind::Bench => "Benchmarking", + _ => panic!("not a test command: {}!", self.as_str()), + } + } } impl<'a> Builder<'a> { diff --git a/src/bootstrap/builder/tests.rs b/src/bootstrap/builder/tests.rs index 3574f11189ee9..72ac46b6bfddd 100644 --- a/src/bootstrap/builder/tests.rs +++ b/src/bootstrap/builder/tests.rs @@ -578,7 +578,6 @@ mod dist { compiler: Compiler { host, stage: 0 }, target: host, mode: Mode::Std, - test_kind: test::TestKind::Test, crates: vec![INTERNER.intern_str("std")], },] ); diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 601351ea8e3c0..7f843d1c7d2ed 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -27,44 +27,6 @@ use crate::{envify, CLang, DocTests, GitRepo, Mode}; const ADB_TEST_DIR: &str = "/data/local/tmp/work"; -/// The two modes of the test runner; tests or benchmarks. -#[derive(Debug, PartialEq, Eq, Hash, Copy, Clone, PartialOrd, Ord)] -pub enum TestKind { - /// Run `cargo test`. - Test, - /// Run `cargo bench`. - Bench, -} - -impl From for TestKind { - fn from(kind: Kind) -> Self { - match kind { - Kind::Test => TestKind::Test, - Kind::Bench => TestKind::Bench, - _ => panic!("unexpected kind in crate: {:?}", kind), - } - } -} - -impl TestKind { - // Return the cargo subcommand for this test kind - fn subcommand(self) -> &'static str { - match self { - TestKind::Test => "test", - TestKind::Bench => "bench", - } - } -} - -impl Into for TestKind { - fn into(self) -> Kind { - match self { - TestKind::Test => Kind::Test, - TestKind::Bench => Kind::Bench, - } - } -} - fn try_run(builder: &Builder<'_>, cmd: &mut Command) -> bool { if !builder.fail_fast { if !builder.try_run(cmd) { @@ -2111,7 +2073,6 @@ impl Step for RustcGuide { pub struct CrateLibrustc { compiler: Compiler, target: TargetSelection, - test_kind: TestKind, crates: Vec>, } @@ -2133,9 +2094,8 @@ impl Step for CrateLibrustc { .iter() .map(|p| builder.crate_paths[&p.assert_single_path().path].clone()) .collect(); - let test_kind = builder.kind.into(); - builder.ensure(CrateLibrustc { compiler, target: run.target, test_kind, crates }); + builder.ensure(CrateLibrustc { compiler, target: run.target, crates }); } fn run(self, builder: &Builder<'_>) { @@ -2143,7 +2103,6 @@ impl Step for CrateLibrustc { compiler: self.compiler, target: self.target, mode: Mode::Rustc, - test_kind: self.test_kind, crates: self.crates, }); } @@ -2154,7 +2113,6 @@ pub struct Crate { pub compiler: Compiler, pub target: TargetSelection, pub mode: Mode, - pub test_kind: TestKind, pub crates: Vec>, } @@ -2170,14 +2128,13 @@ impl Step for Crate { let builder = run.builder; let host = run.build_triple(); let compiler = builder.compiler_for(builder.top_stage, host, host); - let test_kind = builder.kind.into(); let crates = run .paths .iter() .map(|p| builder.crate_paths[&p.assert_single_path().path].clone()) .collect(); - builder.ensure(Crate { compiler, target: run.target, mode: Mode::Std, test_kind, crates }); + builder.ensure(Crate { compiler, target: run.target, mode: Mode::Std, crates }); } /// Runs all unit tests plus documentation tests for a given crate defined @@ -2192,7 +2149,6 @@ impl Step for Crate { let compiler = self.compiler; let target = self.target; let mode = self.mode; - let test_kind = self.test_kind; builder.ensure(compile::Std::new(compiler, target)); builder.ensure(RemoteCopyLibs { compiler, target }); @@ -2204,7 +2160,7 @@ impl Step for Crate { let compiler = builder.compiler_for(compiler.stage, compiler.host, target); let mut cargo = - builder.cargo(compiler, mode, SourceType::InTree, target, test_kind.subcommand()); + builder.cargo(compiler, mode, SourceType::InTree, target, builder.kind.as_str()); match mode { Mode::Std => { compile::std_cargo(builder, target, compiler.stage, &mut cargo); @@ -2220,7 +2176,7 @@ impl Step for Crate { // Pass in some standard flags then iterate over the graph we've discovered // in `cargo metadata` with the maps above and figure out what `-p` // arguments need to get passed. - if test_kind.subcommand() == "test" && !builder.fail_fast { + if builder.kind == Kind::Test && !builder.fail_fast { cargo.arg("--no-fail-fast"); } match builder.doc_tests { @@ -2270,7 +2226,7 @@ impl Step for Crate { } let _guard = builder.msg( - test_kind, + builder.kind, compiler.stage, crate_description(&self.crates), compiler.host, @@ -2285,7 +2241,6 @@ impl Step for Crate { #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct CrateRustdoc { host: TargetSelection, - test_kind: TestKind, } impl Step for CrateRustdoc { @@ -2300,13 +2255,10 @@ impl Step for CrateRustdoc { fn make_run(run: RunConfig<'_>) { let builder = run.builder; - let test_kind = builder.kind.into(); - - builder.ensure(CrateRustdoc { host: run.target, test_kind }); + builder.ensure(CrateRustdoc { host: run.target }); } fn run(self, builder: &Builder<'_>) { - let test_kind = self.test_kind; let target = self.host; let compiler = if builder.download_rustc() { @@ -2325,12 +2277,12 @@ impl Step for CrateRustdoc { compiler, Mode::ToolRustc, target, - test_kind.subcommand(), + builder.kind.as_str(), "src/tools/rustdoc", SourceType::InTree, &[], ); - if test_kind.subcommand() == "test" && !builder.fail_fast { + if builder.kind == Kind::Test && !builder.fail_fast { cargo.arg("--no-fail-fast"); } match builder.doc_tests { @@ -2391,7 +2343,7 @@ impl Step for CrateRustdoc { cargo.arg("--quiet"); } - let _guard = builder.msg(test_kind, compiler.stage, "rustdoc", compiler.host, target); + let _guard = builder.msg(builder.kind, compiler.stage, "rustdoc", compiler.host, target); let _time = util::timeit(&builder); @@ -2402,7 +2354,6 @@ impl Step for CrateRustdoc { #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct CrateRustdocJsonTypes { host: TargetSelection, - test_kind: TestKind, } impl Step for CrateRustdocJsonTypes { @@ -2417,13 +2368,10 @@ impl Step for CrateRustdocJsonTypes { fn make_run(run: RunConfig<'_>) { let builder = run.builder; - let test_kind = builder.kind.into(); - - builder.ensure(CrateRustdocJsonTypes { host: run.target, test_kind }); + builder.ensure(CrateRustdocJsonTypes { host: run.target }); } fn run(self, builder: &Builder<'_>) { - let test_kind = self.test_kind; let target = self.host; // Use the previous stage compiler to reuse the artifacts that are @@ -2438,12 +2386,12 @@ impl Step for CrateRustdocJsonTypes { compiler, Mode::ToolRustc, target, - test_kind.subcommand(), + builder.kind.as_str(), "src/rustdoc-json-types", SourceType::InTree, &[], ); - if test_kind.subcommand() == "test" && !builder.fail_fast { + if builder.kind == Kind::Test && !builder.fail_fast { cargo.arg("--no-fail-fast"); } @@ -2457,7 +2405,7 @@ impl Step for CrateRustdocJsonTypes { } let _guard = - builder.msg(test_kind, compiler.stage, "rustdoc-json-types", compiler.host, target); + builder.msg(builder.kind, compiler.stage, "rustdoc-json-types", compiler.host, target); let _time = util::timeit(&builder); add_flags_and_try_run_tests(builder, &mut cargo.into()); From fc5a742b24539f4e3e62dffa005c6a927adb66b5 Mon Sep 17 00:00:00 2001 From: jyn Date: Wed, 19 Apr 2023 20:58:45 -0500 Subject: [PATCH 2/6] [wip] separate out a test_crate_args fn --- src/bootstrap/test.rs | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 7f843d1c7d2ed..e8804eb1d609c 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -2108,6 +2108,25 @@ impl Step for CrateLibrustc { } } +// Given a `cargo test` subcommand, pass it the appropriate test flags given a `builder`. +fn cargo_test_args(cargo: &mut Command, libtest_args: &[&str], _crates: &[&str], builder: &Builder<'_>) { + if !builder.fail_fast { + cargo.arg("--no-fail-fast"); + } + match builder.doc_tests { + DocTests::Only => { + cargo.arg("--doc"); + } + DocTests::No => { + cargo.args(&["--lib", "--bins", "--examples", "--tests", "--benches"]); + } + DocTests::Yes => {} + } + + cargo.arg("--").args(&builder.config.cmd.test_args()).args(libtest_args); + add_flags_and_try_run_tests(builder, cargo); +} + #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct Crate { pub compiler: Compiler, @@ -2560,24 +2579,9 @@ impl Step for Bootstrap { // https://github.com/rust-lang/rust/issues/49215 cmd.env("RUSTFLAGS", flags); } - if !builder.fail_fast { - cmd.arg("--no-fail-fast"); - } - match builder.doc_tests { - DocTests::Only => { - cmd.arg("--doc"); - } - DocTests::No => { - cmd.args(&["--lib", "--bins", "--examples", "--tests", "--benches"]); - } - DocTests::Yes => {} - } - - cmd.arg("--").args(&builder.config.cmd.test_args()); // rustbuild tests are racy on directory creation so just run them one at a time. // Since there's not many this shouldn't be a problem. - cmd.arg("--test-threads=1"); - add_flags_and_try_run_tests(builder, &mut cmd); + cargo_test_args(&mut cmd, &["--test-threads=1"], &[], builder); } fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { From 107257eedc3d351b7c1a02cddee89e52db5808e9 Mon Sep 17 00:00:00 2001 From: jyn Date: Wed, 19 Apr 2023 21:21:13 -0500 Subject: [PATCH 3/6] switch Crate to run_cargo_test --- src/bootstrap/test.rs | 104 +++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 61 deletions(-) diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index e8804eb1d609c..b026c449a3838 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -2109,8 +2109,13 @@ impl Step for CrateLibrustc { } // Given a `cargo test` subcommand, pass it the appropriate test flags given a `builder`. -fn cargo_test_args(cargo: &mut Command, libtest_args: &[&str], _crates: &[&str], builder: &Builder<'_>) { - if !builder.fail_fast { +fn run_cargo_test(cargo: impl Into, libtest_args: &[&str], crates: &[Interned], compiler: Compiler, target: TargetSelection, builder: &Builder<'_>) { + let mut cargo = cargo.into(); + + // Pass in some standard flags then iterate over the graph we've discovered + // in `cargo metadata` with the maps above and figure out what `-p` + // arguments need to get passed. + if builder.kind == Kind::Test && !builder.fail_fast { cargo.arg("--no-fail-fast"); } match builder.doc_tests { @@ -2123,8 +2128,38 @@ fn cargo_test_args(cargo: &mut Command, libtest_args: &[&str], _crates: &[&str], DocTests::Yes => {} } + for &krate in crates { + cargo.arg("-p").arg(krate); + } + + // The tests are going to run with the *target* libraries, so we need to + // ensure that those libraries show up in the LD_LIBRARY_PATH equivalent. + // + // Note that to run the compiler we need to run with the *host* libraries, + // but our wrapper scripts arrange for that to be the case anyway. + let mut dylib_path = dylib_path(); + dylib_path.insert(0, PathBuf::from(&*builder.sysroot_libdir(compiler, target))); + cargo.env(dylib_path_var(), env::join_paths(&dylib_path).unwrap()); + + if target.contains("emscripten") { + cargo.env( + format!("CARGO_TARGET_{}_RUNNER", envify(&target.triple)), + builder.config.nodejs.as_ref().expect("nodejs not configured"), + ); + } else if target.starts_with("wasm32") { + let node = builder.config.nodejs.as_ref().expect("nodejs not configured"); + let runner = format!("{} {}/src/etc/wasm32-shim.js", node.display(), builder.src.display()); + cargo.env(format!("CARGO_TARGET_{}_RUNNER", envify(&target.triple)), &runner); + } else if builder.remote_tested(target) { + cargo.env( + format!("CARGO_TARGET_{}_RUNNER", envify(&target.triple)), + format!("{} run 0", builder.tool_exe(Tool::RemoteTestClient).display()), + ); + } + cargo.arg("--").args(&builder.config.cmd.test_args()).args(libtest_args); - add_flags_and_try_run_tests(builder, cargo); + let _time = util::timeit(&builder); + add_flags_and_try_run_tests(builder, &mut cargo); } #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] @@ -2190,60 +2225,6 @@ impl Step for Crate { _ => panic!("can only test libraries"), }; - // Build up the base `cargo test` command. - // - // Pass in some standard flags then iterate over the graph we've discovered - // in `cargo metadata` with the maps above and figure out what `-p` - // arguments need to get passed. - if builder.kind == Kind::Test && !builder.fail_fast { - cargo.arg("--no-fail-fast"); - } - match builder.doc_tests { - DocTests::Only => { - cargo.arg("--doc"); - } - DocTests::No => { - cargo.args(&["--lib", "--bins", "--examples", "--tests", "--benches"]); - } - DocTests::Yes => {} - } - - for krate in &self.crates { - cargo.arg("-p").arg(krate); - } - - // The tests are going to run with the *target* libraries, so we need to - // ensure that those libraries show up in the LD_LIBRARY_PATH equivalent. - // - // Note that to run the compiler we need to run with the *host* libraries, - // but our wrapper scripts arrange for that to be the case anyway. - let mut dylib_path = dylib_path(); - dylib_path.insert(0, PathBuf::from(&*builder.sysroot_libdir(compiler, target))); - cargo.env(dylib_path_var(), env::join_paths(&dylib_path).unwrap()); - - cargo.arg("--"); - cargo.args(&builder.config.cmd.test_args()); - - cargo.arg("-Z").arg("unstable-options"); - cargo.arg("--format").arg("json"); - - if target.contains("emscripten") { - cargo.env( - format!("CARGO_TARGET_{}_RUNNER", envify(&target.triple)), - builder.config.nodejs.as_ref().expect("nodejs not configured"), - ); - } else if target.starts_with("wasm32") { - let node = builder.config.nodejs.as_ref().expect("nodejs not configured"); - let runner = - format!("{} {}/src/etc/wasm32-shim.js", node.display(), builder.src.display()); - cargo.env(format!("CARGO_TARGET_{}_RUNNER", envify(&target.triple)), &runner); - } else if builder.remote_tested(target) { - cargo.env( - format!("CARGO_TARGET_{}_RUNNER", envify(&target.triple)), - format!("{} run 0", builder.tool_exe(Tool::RemoteTestClient).display()), - ); - } - let _guard = builder.msg( builder.kind, compiler.stage, @@ -2251,8 +2232,7 @@ impl Step for Crate { compiler.host, target, ); - let _time = util::timeit(&builder); - crate::render_tests::try_run_tests(builder, &mut cargo.into()); + run_cargo_test(cargo, &[], &self.crates, compiler, target, builder); } } @@ -2565,13 +2545,15 @@ impl Step for Bootstrap { check_bootstrap.arg("bootstrap_test.py").current_dir(builder.src.join("src/bootstrap/")); try_run(builder, &mut check_bootstrap); + let host = builder.config.build; + let compiler = builder.compiler(0, host); let mut cmd = Command::new(&builder.initial_cargo); cmd.arg("test") .current_dir(builder.src.join("src/bootstrap")) .env("RUSTFLAGS", "-Cdebuginfo=2") .env("CARGO_TARGET_DIR", builder.out.join("bootstrap")) .env("RUSTC_BOOTSTRAP", "1") - .env("RUSTDOC", builder.rustdoc(builder.compiler(0, builder.build.build))) + .env("RUSTDOC", builder.rustdoc(compiler)) .env("RUSTC", &builder.initial_rustc); if let Some(flags) = option_env!("RUSTFLAGS") { // Use the same rustc flags for testing as for "normal" compilation, @@ -2581,7 +2563,7 @@ impl Step for Bootstrap { } // rustbuild tests are racy on directory creation so just run them one at a time. // Since there's not many this shouldn't be a problem. - cargo_test_args(&mut cmd, &["--test-threads=1"], &[], builder); + run_cargo_test(cmd, &["--test-threads=1"], &[], compiler, host, builder); } fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { From ff674c1664330fc838a295f115972f8068959f3b Mon Sep 17 00:00:00 2001 From: jyn Date: Wed, 19 Apr 2023 21:34:30 -0500 Subject: [PATCH 4/6] Convert the rest of the `test` Steps to run_cargo_test --- src/bootstrap/test.rs | 164 ++++++++++++++++++++---------------------- 1 file changed, 79 insertions(+), 85 deletions(-) diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index b026c449a3838..a1d46f4e33d93 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -13,6 +13,7 @@ use std::process::{Command, Stdio}; use crate::builder::crate_description; use crate::builder::{Builder, Compiler, Kind, RunConfig, ShouldRun, Step}; use crate::cache::Interned; +use crate::cache::INTERNER; use crate::compile; use crate::config::TargetSelection; use crate::dist; @@ -85,7 +86,7 @@ impl Step for CrateJsonDocLint { SourceType::InTree, &[], ); - add_flags_and_try_run_tests(builder, &mut cargo.into()); + run_cargo_test(cargo, &[], &[], compiler, bootstrap_host, builder); } } @@ -111,7 +112,7 @@ impl Step for SuggestTestsCrate { let bootstrap_host = builder.config.build; let compiler = builder.compiler(0, bootstrap_host); - let suggest_tests = tool::prepare_tool_cargo( + let cargo = tool::prepare_tool_cargo( builder, compiler, Mode::ToolBootstrap, @@ -121,7 +122,7 @@ impl Step for SuggestTestsCrate { SourceType::InTree, &[], ); - add_flags_and_try_run_tests(builder, &mut suggest_tests.into()); + run_cargo_test(cargo, &[], &[], compiler, bootstrap_host, builder); } } @@ -170,7 +171,7 @@ You can skip linkcheck with --exclude src/tools/linkchecker" SourceType::InTree, &[], ); - add_flags_and_try_run_tests(builder, &mut cargo.into()); + run_cargo_test(cargo, &[], &[], compiler, bootstrap_host, builder); // Build all the default documentation. builder.default_doc(&[]); @@ -306,7 +307,7 @@ impl Step for Cargo { let compiler = builder.compiler(self.stage, self.host); builder.ensure(tool::Cargo { compiler, target: self.host }); - let mut cargo = tool::prepare_tool_cargo( + let cargo = tool::prepare_tool_cargo( builder, compiler, Mode::ToolRustc, @@ -317,10 +318,8 @@ impl Step for Cargo { &[], ); - if !builder.fail_fast { - cargo.arg("--no-fail-fast"); - } - cargo.arg("--").args(builder.config.cmd.test_args()); + // NOTE: can't use `run_cargo_test` because we need to overwrite `PATH` + let mut cargo = prepare_cargo_test(cargo, &[], &[], compiler, self.host, builder); // Don't run cross-compile tests, we may not have cross-compiled libstd libs // available. @@ -328,10 +327,10 @@ impl Step for Cargo { // Forcibly disable tests using nightly features since any changes to // those features won't be able to land. cargo.env("CARGO_TEST_DISABLE_NIGHTLY", "1"); - cargo.env("PATH", &path_for_cargo(builder, compiler)); - add_flags_and_try_run_tests(builder, &mut cargo.into()); + let _time = util::timeit(&builder); + add_flags_and_try_run_tests(builder, &mut cargo); } } @@ -388,9 +387,7 @@ impl Step for RustAnalyzer { cargo.env("SKIP_SLOW_TESTS", "1"); cargo.add_rustc_lib_path(builder, compiler); - cargo.arg("--").args(builder.config.cmd.test_args()); - - add_flags_and_try_run_tests(builder, &mut cargo.into()); + run_cargo_test(cargo, &[], &[], compiler, host, builder); } } @@ -433,17 +430,13 @@ impl Step for Rustfmt { &[], ); - if !builder.fail_fast { - cargo.arg("--no-fail-fast"); - } - let dir = testdir(builder, compiler.host); t!(fs::create_dir_all(&dir)); cargo.env("RUSTFMT_TEST_DIR", dir); cargo.add_rustc_lib_path(builder, compiler); - add_flags_and_try_run_tests(builder, &mut cargo.into()); + run_cargo_test(cargo, &[], &[], compiler, host, builder); } } @@ -489,12 +482,9 @@ impl Step for RustDemangler { t!(fs::create_dir_all(&dir)); cargo.env("RUST_DEMANGLER_DRIVER_PATH", rust_demangler); - - cargo.arg("--").args(builder.config.cmd.test_args()); - cargo.add_rustc_lib_path(builder, compiler); - add_flags_and_try_run_tests(builder, &mut cargo.into()); + run_cargo_test(cargo, &[], &[], compiler, host, builder); } } @@ -617,10 +607,6 @@ impl Step for Miri { ); cargo.add_rustc_lib_path(builder, compiler); - if !builder.fail_fast { - cargo.arg("--no-fail-fast"); - } - // miri tests need to know about the stage sysroot cargo.env("MIRI_SYSROOT", &miri_sysroot); cargo.env("MIRI_HOST_SYSROOT", sysroot); @@ -632,13 +618,14 @@ impl Step for Miri { // Set the target. cargo.env("MIRI_TEST_TARGET", target.rustc_target_arg()); - // Forward test filters. - cargo.arg("--").args(builder.config.cmd.test_args()); - // This can NOT be `add_flags_and_try_run_tests` since the Miri test runner - // does not understand those flags! - let mut cargo = Command::from(cargo); - builder.run(&mut cargo); + // This can NOT be `run_cargo_test` since the Miri test runner + // does not understand the flags added by `add_flags_and_try_run_test`. + let mut cargo = prepare_cargo_test(cargo, &[], &[], compiler, target, builder); + { + let _time = util::timeit(&builder); + builder.run(&mut cargo); + } // # Run `cargo miri test`. // This is just a smoke test (Miri's own CI invokes this in a bunch of different ways and ensures @@ -671,6 +658,7 @@ impl Step for Miri { cargo.env("RUST_BACKTRACE", "1"); let mut cargo = Command::from(cargo); + let _time = util::timeit(&builder); builder.run(&mut cargo); } } @@ -710,8 +698,7 @@ impl Step for CompiletestTest { &[], ); cargo.allow_features("test"); - - add_flags_and_try_run_tests(builder, &mut cargo.into()); + run_cargo_test(cargo, &[], &[], compiler, host, builder); } } @@ -763,11 +750,10 @@ impl Step for Clippy { let host_libs = builder.stage_out(compiler, Mode::ToolRustc).join(builder.cargo_dir()); cargo.env("HOST_LIBS", host_libs); - cargo.arg("--").args(builder.config.cmd.test_args()); - cargo.add_rustc_lib_path(builder, compiler); + let mut cargo = prepare_cargo_test(cargo, &[], &[], compiler, host, builder); - if builder.try_run(&mut cargo.into()) { + if builder.try_run(&mut cargo) { // The tests succeeded; nothing to do. return; } @@ -1195,7 +1181,7 @@ impl Step for TidySelfTest { SourceType::InTree, &[], ); - add_flags_and_try_run_tests(builder, &mut cargo.into()); + run_cargo_test(cargo, &[], &[], compiler, bootstrap_host, builder); } } @@ -2108,8 +2094,31 @@ impl Step for CrateLibrustc { } } -// Given a `cargo test` subcommand, pass it the appropriate test flags given a `builder`. -fn run_cargo_test(cargo: impl Into, libtest_args: &[&str], crates: &[Interned], compiler: Compiler, target: TargetSelection, builder: &Builder<'_>) { +/// Given a `cargo test` subcommand, add the appropriate flags and run it. +/// +/// Returns whether the test succeeded. +fn run_cargo_test( + cargo: impl Into, + libtest_args: &[&str], + crates: &[Interned], + compiler: Compiler, + target: TargetSelection, + builder: &Builder<'_>, +) -> bool { + let mut cargo = prepare_cargo_test(cargo, libtest_args, crates, compiler, target, builder); + let _time = util::timeit(&builder); + add_flags_and_try_run_tests(builder, &mut cargo) +} + +/// Given a `cargo test` subcommand, pass it the appropriate test flags given a `builder`. +fn prepare_cargo_test( + cargo: impl Into, + libtest_args: &[&str], + crates: &[Interned], + compiler: Compiler, + target: TargetSelection, + builder: &Builder<'_>, +) -> Command { let mut cargo = cargo.into(); // Pass in some standard flags then iterate over the graph we've discovered @@ -2132,6 +2141,11 @@ fn run_cargo_test(cargo: impl Into, libtest_args: &[&str], crates: &[In cargo.arg("-p").arg(krate); } + cargo.arg("--").args(&builder.config.cmd.test_args()).args(libtest_args); + if !builder.config.verbose_tests { + cargo.arg("--quiet"); + } + // The tests are going to run with the *target* libraries, so we need to // ensure that those libraries show up in the LD_LIBRARY_PATH equivalent. // @@ -2157,9 +2171,7 @@ fn run_cargo_test(cargo: impl Into, libtest_args: &[&str], crates: &[In ); } - cargo.arg("--").args(&builder.config.cmd.test_args()).args(libtest_args); - let _time = util::timeit(&builder); - add_flags_and_try_run_tests(builder, &mut cargo); + cargo } #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] @@ -2281,24 +2293,6 @@ impl Step for CrateRustdoc { SourceType::InTree, &[], ); - if builder.kind == Kind::Test && !builder.fail_fast { - cargo.arg("--no-fail-fast"); - } - match builder.doc_tests { - DocTests::Only => { - cargo.arg("--doc"); - } - DocTests::No => { - cargo.args(&["--lib", "--bins", "--examples", "--tests", "--benches"]); - } - DocTests::Yes => {} - } - - cargo.arg("-p").arg("rustdoc:0.0.0"); - - cargo.arg("--"); - cargo.args(&builder.config.cmd.test_args()); - if self.host.contains("musl") { cargo.arg("'-Ctarget-feature=-crt-static'"); } @@ -2338,15 +2332,15 @@ impl Step for CrateRustdoc { dylib_path.insert(0, PathBuf::from(&*libdir)); cargo.env(dylib_path_var(), env::join_paths(&dylib_path).unwrap()); - if !builder.config.verbose_tests { - cargo.arg("--quiet"); - } - let _guard = builder.msg(builder.kind, compiler.stage, "rustdoc", compiler.host, target); - - let _time = util::timeit(&builder); - - add_flags_and_try_run_tests(builder, &mut cargo.into()); + run_cargo_test( + cargo, + &[], + &[INTERNER.intern_str("rustdoc:0.0.0")], + compiler, + target, + builder, + ); } } @@ -2380,7 +2374,7 @@ impl Step for CrateRustdocJsonTypes { let compiler = builder.compiler_for(builder.top_stage, target, target); builder.ensure(compile::Rustc::new(compiler, target)); - let mut cargo = tool::prepare_tool_cargo( + let cargo = tool::prepare_tool_cargo( builder, compiler, Mode::ToolRustc, @@ -2390,24 +2384,24 @@ impl Step for CrateRustdocJsonTypes { SourceType::InTree, &[], ); - if builder.kind == Kind::Test && !builder.fail_fast { - cargo.arg("--no-fail-fast"); - } - - cargo.arg("-p").arg("rustdoc-json-types"); - cargo.arg("--"); - cargo.args(&builder.config.cmd.test_args()); - - if self.host.contains("musl") { - cargo.arg("'-Ctarget-feature=-crt-static'"); - } + // FIXME: this looks very wrong, libtest doesn't accept `-C` arguments and the quotes are fishy. + let libtest_args = if self.host.contains("musl") { + ["'-Ctarget-feature=-crt-static'"].as_slice() + } else { + &[] + }; let _guard = builder.msg(builder.kind, compiler.stage, "rustdoc-json-types", compiler.host, target); - let _time = util::timeit(&builder); - - add_flags_and_try_run_tests(builder, &mut cargo.into()); + run_cargo_test( + cargo, + libtest_args, + &[INTERNER.intern_str("rustdoc-json-types")], + compiler, + target, + builder, + ); } } From 2a75607baba3e42814e4cc7fa35358f1be7f75ed Mon Sep 17 00:00:00 2001 From: jyn Date: Wed, 19 Apr 2023 22:35:53 -0500 Subject: [PATCH 5/6] Combine several `Step`s into a single step with multiple paths --- src/bootstrap/builder.rs | 5 +- src/bootstrap/test.rs | 130 ++++++--------------------------------- src/bootstrap/tool.rs | 2 +- 3 files changed, 22 insertions(+), 115 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index 5b5e5fe6662f0..d9d4685dfc790 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -703,7 +703,6 @@ impl<'a> Builder<'a> { crate::toolstate::ToolStateCheck, test::ExpandYamlAnchors, test::Tidy, - test::TidySelfTest, test::Ui, test::RunPassValgrind, test::MirOpt, @@ -719,11 +718,9 @@ impl<'a> Builder<'a> { test::CrateLibrustc, test::CrateRustdoc, test::CrateRustdocJsonTypes, - test::CrateJsonDocLint, - test::SuggestTestsCrate, + test::CrateBootstrap, test::Linkcheck, test::TierCheck, - test::ReplacePlaceholderTest, test::Cargotest, test::Cargo, test::RustAnalyzer, diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index a1d46f4e33d93..5b70494b30b8c 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -55,26 +55,37 @@ fn try_run_quiet(builder: &Builder<'_>, cmd: &mut Command) -> bool { } #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] -pub struct CrateJsonDocLint { +pub struct CrateBootstrap { + path: Interned, host: TargetSelection, } -impl Step for CrateJsonDocLint { +impl Step for CrateBootstrap { type Output = (); const ONLY_HOSTS: bool = true; const DEFAULT: bool = true; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { run.path("src/tools/jsondoclint") + .path("src/tools/suggest-tests") + .path("src/tools/replace-version-placeholder") + .alias("tidyselftest") } fn make_run(run: RunConfig<'_>) { - run.builder.ensure(CrateJsonDocLint { host: run.target }); + for path in run.paths { + let path = INTERNER.intern_path(path.assert_single_path().path.clone()); + run.builder.ensure(CrateBootstrap { host: run.target, path }); + } } fn run(self, builder: &Builder<'_>) { let bootstrap_host = builder.config.build; let compiler = builder.compiler(0, bootstrap_host); + let mut path = self.path.to_str().unwrap(); + if path == "tidyselftest" { + path = "src/tools/tidy"; + } let cargo = tool::prepare_tool_cargo( builder, @@ -82,46 +93,16 @@ impl Step for CrateJsonDocLint { Mode::ToolBootstrap, bootstrap_host, "test", - "src/tools/jsondoclint", + path, SourceType::InTree, &[], ); - run_cargo_test(cargo, &[], &[], compiler, bootstrap_host, builder); - } -} - -#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] -pub struct SuggestTestsCrate { - host: TargetSelection, -} - -impl Step for SuggestTestsCrate { - type Output = (); - const ONLY_HOSTS: bool = true; - const DEFAULT: bool = true; - - fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - run.path("src/tools/suggest-tests") - } - - fn make_run(run: RunConfig<'_>) { - run.builder.ensure(SuggestTestsCrate { host: run.target }); - } - - fn run(self, builder: &Builder<'_>) { - let bootstrap_host = builder.config.build; - let compiler = builder.compiler(0, bootstrap_host); - - let cargo = tool::prepare_tool_cargo( - builder, - compiler, - Mode::ToolBootstrap, + builder.info(&format!( + "{} {} stage0 ({})", + builder.kind.test_description(), + path, bootstrap_host, - "test", - "src/tools/suggest-tests", - SourceType::InTree, - &[], - ); + )); run_cargo_test(cargo, &[], &[], compiler, bootstrap_host, builder); } } @@ -1151,40 +1132,6 @@ help: to skip test's attempt to check tidiness, pass `--exclude src/tools/tidy` } } -/// Runs tidy's own tests. -#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] -pub struct TidySelfTest; - -impl Step for TidySelfTest { - type Output = (); - const DEFAULT: bool = true; - const ONLY_HOSTS: bool = true; - - fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - run.alias("tidyselftest") - } - - fn make_run(run: RunConfig<'_>) { - run.builder.ensure(TidySelfTest); - } - - fn run(self, builder: &Builder<'_>) { - let bootstrap_host = builder.config.build; - let compiler = builder.compiler(0, bootstrap_host); - let cargo = tool::prepare_tool_cargo( - builder, - compiler, - Mode::ToolBootstrap, - bootstrap_host, - "test", - "src/tools/tidy", - SourceType::InTree, - &[], - ); - run_cargo_test(cargo, &[], &[], compiler, bootstrap_host, builder); - } -} - #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct ExpandYamlAnchors; @@ -2613,43 +2560,6 @@ impl Step for TierCheck { } } -#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] -pub struct ReplacePlaceholderTest; - -impl Step for ReplacePlaceholderTest { - type Output = (); - const ONLY_HOSTS: bool = true; - const DEFAULT: bool = true; - - /// Ensure the version placeholder replacement tool builds - fn run(self, builder: &Builder<'_>) { - builder.info("build check for version replacement placeholder"); - - // Test the version placeholder replacement tool itself. - let bootstrap_host = builder.config.build; - let compiler = builder.compiler(0, bootstrap_host); - let cargo = tool::prepare_tool_cargo( - builder, - compiler, - Mode::ToolBootstrap, - bootstrap_host, - "test", - "src/tools/replace-version-placeholder", - SourceType::InTree, - &[], - ); - add_flags_and_try_run_tests(builder, &mut cargo.into()); - } - - fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - run.path("src/tools/replace-version-placeholder") - } - - fn make_run(run: RunConfig<'_>) { - run.builder.ensure(Self); - } -} - #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct LintDocs { pub compiler: Compiler, diff --git a/src/bootstrap/tool.rs b/src/bootstrap/tool.rs index 9418cf0a7ed1e..39f6369b4d3f5 100644 --- a/src/bootstrap/tool.rs +++ b/src/bootstrap/tool.rs @@ -141,7 +141,7 @@ pub fn prepare_tool_cargo( mode: Mode, target: TargetSelection, command: &'static str, - path: &'static str, + path: &str, source_type: SourceType, extra_features: &[String], ) -> CargoCommand { From 78a709348dca33414f76986ae72c651d489092f4 Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 20 Apr 2023 21:30:48 -0500 Subject: [PATCH 6/6] Fix `x test --no-deps` - Use `cargo metadata` to determine whether a crate has a library package or not - Collect metadata for all workspaces, not just the root workspace and cargo - Don't pass `--lib` for crates without a library - Use `run_cargo_test` for rust-installer - Don't build documentation in `lint-docs` if `--no-doc` is passed --- src/bootstrap/lib.rs | 1 + src/bootstrap/metadata.rs | 42 ++++++++++++++++++++------------- src/bootstrap/test.rs | 49 +++++++++++++++++++++++++-------------- 3 files changed, 58 insertions(+), 34 deletions(-) diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index 14e1328171b9a..59d2e9cc69e79 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -246,6 +246,7 @@ struct Crate { name: Interned, deps: HashSet>, path: PathBuf, + has_lib: bool, } impl Crate { diff --git a/src/bootstrap/metadata.rs b/src/bootstrap/metadata.rs index 597aefadcfe4c..8f2c3faca3a48 100644 --- a/src/bootstrap/metadata.rs +++ b/src/bootstrap/metadata.rs @@ -5,7 +5,7 @@ use serde_derive::Deserialize; use crate::cache::INTERNER; use crate::util::output; -use crate::{Build, Crate}; +use crate::{t, Build, Crate}; /// For more information, see the output of /// @@ -22,6 +22,7 @@ struct Package { source: Option, manifest_path: String, dependencies: Vec, + targets: Vec, } /// For more information, see the output of @@ -32,6 +33,11 @@ struct Dependency { source: Option, } +#[derive(Debug, Deserialize)] +struct Target { + kind: Vec, +} + /// Collects and stores package metadata of each workspace members into `build`, /// by executing `cargo metadata` commands. pub fn build(build: &mut Build) { @@ -46,11 +52,16 @@ pub fn build(build: &mut Build) { .filter(|dep| dep.source.is_none()) .map(|dep| INTERNER.intern_string(dep.name)) .collect(); - let krate = Crate { name, deps, path }; + let has_lib = package.targets.iter().any(|t| t.kind.iter().any(|k| k == "lib")); + let krate = Crate { name, deps, path, has_lib }; let relative_path = krate.local_path(build); build.crates.insert(name, krate); let existing_path = build.crate_paths.insert(relative_path, name); - assert!(existing_path.is_none(), "multiple crates with the same path"); + assert!( + existing_path.is_none(), + "multiple crates with the same path: {}", + existing_path.unwrap() + ); } } } @@ -60,7 +71,7 @@ pub fn build(build: &mut Build) { /// Note that `src/tools/cargo` is no longer a workspace member but we still /// treat it as one here, by invoking an additional `cargo metadata` command. fn workspace_members(build: &Build) -> impl Iterator { - let cmd_metadata = |manifest_path| { + let collect_metadata = |manifest_path| { let mut cargo = Command::new(&build.initial_cargo); cargo .arg("metadata") @@ -68,21 +79,20 @@ fn workspace_members(build: &Build) -> impl Iterator { .arg("1") .arg("--no-deps") .arg("--manifest-path") - .arg(manifest_path); - cargo + .arg(build.src.join(manifest_path)); + let metadata_output = output(&mut cargo); + let Output { packages, .. } = t!(serde_json::from_str(&metadata_output)); + packages }; - // Collects `metadata.packages` from the root workspace. - let root_manifest_path = build.src.join("Cargo.toml"); - let root_output = output(&mut cmd_metadata(&root_manifest_path)); - let Output { packages, .. } = serde_json::from_str(&root_output).unwrap(); - - // Collects `metadata.packages` from src/tools/cargo separately. - let cargo_manifest_path = build.src.join("src/tools/cargo/Cargo.toml"); - let cargo_output = output(&mut cmd_metadata(&cargo_manifest_path)); - let Output { packages: cargo_packages, .. } = serde_json::from_str(&cargo_output).unwrap(); + // Collects `metadata.packages` from all workspaces. + let packages = collect_metadata("Cargo.toml"); + let cargo_packages = collect_metadata("src/tools/cargo/Cargo.toml"); + let ra_packages = collect_metadata("src/tools/rust-analyzer/Cargo.toml"); + let bootstrap_packages = collect_metadata("src/bootstrap/Cargo.toml"); // We only care about the root package from `src/tool/cargo` workspace. let cargo_package = cargo_packages.into_iter().find(|pkg| pkg.name == "cargo").into_iter(); - packages.into_iter().chain(cargo_package) + + packages.into_iter().chain(cargo_package).chain(ra_packages).chain(bootstrap_packages) } diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 5b70494b30b8c..aee84e9c9beb9 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -103,7 +103,8 @@ impl Step for CrateBootstrap { path, bootstrap_host, )); - run_cargo_test(cargo, &[], &[], compiler, bootstrap_host, builder); + let crate_name = path.rsplit_once('/').unwrap().1; + run_cargo_test(cargo, &[], &[], crate_name, compiler, bootstrap_host, builder); } } @@ -152,7 +153,11 @@ You can skip linkcheck with --exclude src/tools/linkchecker" SourceType::InTree, &[], ); - run_cargo_test(cargo, &[], &[], compiler, bootstrap_host, builder); + run_cargo_test(cargo, &[], &[], "linkchecker", compiler, bootstrap_host, builder); + + if builder.doc_tests == DocTests::No { + return; + } // Build all the default documentation. builder.default_doc(&[]); @@ -300,7 +305,7 @@ impl Step for Cargo { ); // NOTE: can't use `run_cargo_test` because we need to overwrite `PATH` - let mut cargo = prepare_cargo_test(cargo, &[], &[], compiler, self.host, builder); + let mut cargo = prepare_cargo_test(cargo, &[], &[], "cargo", compiler, self.host, builder); // Don't run cross-compile tests, we may not have cross-compiled libstd libs // available. @@ -368,7 +373,7 @@ impl Step for RustAnalyzer { cargo.env("SKIP_SLOW_TESTS", "1"); cargo.add_rustc_lib_path(builder, compiler); - run_cargo_test(cargo, &[], &[], compiler, host, builder); + run_cargo_test(cargo, &[], &[], "rust-analyzer", compiler, host, builder); } } @@ -417,7 +422,7 @@ impl Step for Rustfmt { cargo.add_rustc_lib_path(builder, compiler); - run_cargo_test(cargo, &[], &[], compiler, host, builder); + run_cargo_test(cargo, &[], &[], "rustfmt", compiler, host, builder); } } @@ -465,7 +470,7 @@ impl Step for RustDemangler { cargo.env("RUST_DEMANGLER_DRIVER_PATH", rust_demangler); cargo.add_rustc_lib_path(builder, compiler); - run_cargo_test(cargo, &[], &[], compiler, host, builder); + run_cargo_test(cargo, &[], &[], "rust-demangler", compiler, host, builder); } } @@ -602,7 +607,7 @@ impl Step for Miri { // This can NOT be `run_cargo_test` since the Miri test runner // does not understand the flags added by `add_flags_and_try_run_test`. - let mut cargo = prepare_cargo_test(cargo, &[], &[], compiler, target, builder); + let mut cargo = prepare_cargo_test(cargo, &[], &[], "miri", compiler, target, builder); { let _time = util::timeit(&builder); builder.run(&mut cargo); @@ -679,7 +684,7 @@ impl Step for CompiletestTest { &[], ); cargo.allow_features("test"); - run_cargo_test(cargo, &[], &[], compiler, host, builder); + run_cargo_test(cargo, &[], &[], "compiletest", compiler, host, builder); } } @@ -722,17 +727,13 @@ impl Step for Clippy { &[], ); - if !builder.fail_fast { - cargo.arg("--no-fail-fast"); - } - cargo.env("RUSTC_TEST_SUITE", builder.rustc(compiler)); cargo.env("RUSTC_LIB_PATH", builder.rustc_libdir(compiler)); let host_libs = builder.stage_out(compiler, Mode::ToolRustc).join(builder.cargo_dir()); cargo.env("HOST_LIBS", host_libs); cargo.add_rustc_lib_path(builder, compiler); - let mut cargo = prepare_cargo_test(cargo, &[], &[], compiler, host, builder); + let mut cargo = prepare_cargo_test(cargo, &[], &[], "clippy", compiler, host, builder); if builder.try_run(&mut cargo) { // The tests succeeded; nothing to do. @@ -2048,11 +2049,13 @@ fn run_cargo_test( cargo: impl Into, libtest_args: &[&str], crates: &[Interned], + primary_crate: &str, compiler: Compiler, target: TargetSelection, builder: &Builder<'_>, ) -> bool { - let mut cargo = prepare_cargo_test(cargo, libtest_args, crates, compiler, target, builder); + let mut cargo = + prepare_cargo_test(cargo, libtest_args, crates, primary_crate, compiler, target, builder); let _time = util::timeit(&builder); add_flags_and_try_run_tests(builder, &mut cargo) } @@ -2062,6 +2065,7 @@ fn prepare_cargo_test( cargo: impl Into, libtest_args: &[&str], crates: &[Interned], + primary_crate: &str, compiler: Compiler, target: TargetSelection, builder: &Builder<'_>, @@ -2079,7 +2083,14 @@ fn prepare_cargo_test( cargo.arg("--doc"); } DocTests::No => { - cargo.args(&["--lib", "--bins", "--examples", "--tests", "--benches"]); + let krate = &builder + .crates + .get(&INTERNER.intern_str(primary_crate)) + .unwrap_or_else(|| panic!("missing crate {primary_crate}")); + if krate.has_lib { + cargo.arg("--lib"); + } + cargo.args(&["--bins", "--examples", "--tests", "--benches"]); } DocTests::Yes => {} } @@ -2191,7 +2202,7 @@ impl Step for Crate { compiler.host, target, ); - run_cargo_test(cargo, &[], &self.crates, compiler, target, builder); + run_cargo_test(cargo, &[], &self.crates, &self.crates[0], compiler, target, builder); } } @@ -2284,6 +2295,7 @@ impl Step for CrateRustdoc { cargo, &[], &[INTERNER.intern_str("rustdoc:0.0.0")], + "rustdoc", compiler, target, builder, @@ -2345,6 +2357,7 @@ impl Step for CrateRustdocJsonTypes { cargo, libtest_args, &[INTERNER.intern_str("rustdoc-json-types")], + "rustdoc-json-types", compiler, target, builder, @@ -2504,7 +2517,7 @@ impl Step for Bootstrap { } // rustbuild tests are racy on directory creation so just run them one at a time. // Since there's not many this shouldn't be a problem. - run_cargo_test(cmd, &["--test-threads=1"], &[], compiler, host, builder); + run_cargo_test(cmd, &["--test-threads=1"], &[], "bootstrap", compiler, host, builder); } fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { @@ -2617,7 +2630,7 @@ impl Step for RustInstaller { SourceType::InTree, &[], ); - try_run(builder, &mut cargo.into()); + run_cargo_test(cargo, &[], &[], "installer", compiler, bootstrap_host, builder); // We currently don't support running the test.sh script outside linux(?) environments. // Eventually this should likely migrate to #[test]s in rust-installer proper rather than a