From a1780a7e6f19ddf9aae04dca3c135644e4065a0e 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 | 85 ++++++++-------------------------- 3 files changed, 27 insertions(+), 67 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index 3d3f991bffaa7..01c7a25d08c91 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 ccf83974b8c2f..348fd3f874dba 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -5,7 +5,6 @@ use std::env; use std::ffi::OsString; -use std::fmt; use std::fs; use std::iter; use std::path::{Path, PathBuf}; @@ -28,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 fmt::Display for TestKind { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(match *self { - TestKind::Test => "Testing", - TestKind::Bench => "Benchmarking", - }) - } -} - fn try_run(builder: &Builder<'_>, cmd: &mut Command) -> bool { if !builder.fail_fast { if !builder.try_run(cmd) { @@ -2105,7 +2066,6 @@ impl Step for RustcGuide { pub struct CrateLibrustc { compiler: Compiler, target: TargetSelection, - test_kind: TestKind, crates: Vec>, } @@ -2127,9 +2087,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<'_>) { @@ -2137,7 +2096,6 @@ impl Step for CrateLibrustc { compiler: self.compiler, target: self.target, mode: Mode::Rustc, - test_kind: self.test_kind, crates: self.crates, }); } @@ -2148,7 +2106,6 @@ pub struct Crate { pub compiler: Compiler, pub target: TargetSelection, pub mode: Mode, - pub test_kind: TestKind, pub crates: Vec>, } @@ -2164,14 +2121,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 @@ -2186,7 +2142,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 }); @@ -2198,7 +2153,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); @@ -2214,7 +2169,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 { @@ -2265,7 +2220,7 @@ impl Step for Crate { builder.info(&format!( "{}{} stage{} ({} -> {})", - test_kind, + builder.kind.test_description(), crate_description(&self.crates), compiler.stage, &compiler.host, @@ -2280,7 +2235,6 @@ impl Step for Crate { #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct CrateRustdoc { host: TargetSelection, - test_kind: TestKind, } impl Step for CrateRustdoc { @@ -2295,13 +2249,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() { @@ -2320,12 +2271,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 { @@ -2388,7 +2339,10 @@ impl Step for CrateRustdoc { builder.info(&format!( "{} rustdoc stage{} ({} -> {})", - test_kind, compiler.stage, &compiler.host, target + builder.kind.test_description(), + compiler.stage, + &compiler.host, + target )); let _time = util::timeit(&builder); @@ -2399,7 +2353,6 @@ impl Step for CrateRustdoc { #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct CrateRustdocJsonTypes { host: TargetSelection, - test_kind: TestKind, } impl Step for CrateRustdocJsonTypes { @@ -2414,13 +2367,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 @@ -2435,12 +2385,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"); } @@ -2455,7 +2405,10 @@ impl Step for CrateRustdocJsonTypes { builder.info(&format!( "{} rustdoc-json-types stage{} ({} -> {})", - test_kind, compiler.stage, &compiler.host, target + builder.kind.test_description(), + compiler.stage, + &compiler.host, + target )); let _time = util::timeit(&builder); From d49c7d666112df5dbf3a1d5d1e8657b20685d991 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 348fd3f874dba..2f486b8b75eff 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -2101,6 +2101,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, @@ -2564,24 +2583,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 6348fac4816bec7e7b887209a576fb28749ba6d3 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 2f486b8b75eff..c947345d98d8d 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -2102,8 +2102,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 { @@ -2116,8 +2121,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)] @@ -2183,60 +2218,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()), - ); - } - builder.info(&format!( "{}{} stage{} ({} -> {})", builder.kind.test_description(), @@ -2245,8 +2226,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); } } @@ -2569,13 +2549,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, @@ -2585,7 +2567,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 84e7253a4ff1fbf12282b34fb4c5d9cc9f3bb3cb 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 | 159 ++++++++++++++++++++---------------------- 1 file changed, 75 insertions(+), 84 deletions(-) diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index c947345d98d8d..36daee6b3ee42 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(&[]); @@ -317,21 +318,15 @@ impl Step for Cargo { &[], ); - if !builder.fail_fast { - cargo.arg("--no-fail-fast"); - } - cargo.arg("--").args(builder.config.cmd.test_args()); - // Don't run cross-compile tests, we may not have cross-compiled libstd libs // available. cargo.env("CFG_DISABLE_CROSS_TESTS", "1"); // 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()); + run_cargo_test(cargo, &[], &[], compiler, self.host, builder); } } @@ -388,9 +383,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 +426,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 +478,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 +603,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 +614,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 +654,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 +694,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 +746,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 +1177,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); } } @@ -2101,8 +2083,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 @@ -2125,6 +2130,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. // @@ -2150,9 +2160,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)] @@ -2275,24 +2283,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'"); } @@ -2332,10 +2322,6 @@ 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"); - } - builder.info(&format!( "{} rustdoc stage{} ({} -> {})", builder.kind.test_description(), @@ -2343,9 +2329,14 @@ impl Step for CrateRustdoc { &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, + ); } } @@ -2379,7 +2370,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, @@ -2389,18 +2380,13 @@ 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 { + &[] + }; builder.info(&format!( "{} rustdoc-json-types stage{} ({} -> {})", @@ -2409,9 +2395,14 @@ impl Step for CrateRustdocJsonTypes { &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 c7ebddb4a3746552c5de63886e3483ea558352ae 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 01c7a25d08c91..608b4c4d245da 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 36daee6b3ee42..e1ba61fca8e57 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); } } @@ -1147,40 +1128,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; @@ -2614,43 +2561,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 79eec6f848c27..04ddde2acde5f 100644 --- a/src/bootstrap/tool.rs +++ b/src/bootstrap/tool.rs @@ -155,7 +155,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 b3ee81a536353e9622ef7fcf246fdb63ea9f4067 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 bfdb029951f29..488e8efe66eb3 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -248,6 +248,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 e1ba61fca8e57..9540adea18973 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(&[]); @@ -307,7 +312,7 @@ impl Step for Cargo { cargo.env("CARGO_TEST_DISABLE_NIGHTLY", "1"); cargo.env("PATH", &path_for_cargo(builder, compiler)); - run_cargo_test(cargo, &[], &[], compiler, self.host, builder); + run_cargo_test(cargo, &[], &[], "cargo", compiler, self.host, builder); } } @@ -364,7 +369,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); } } @@ -413,7 +418,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); } } @@ -461,7 +466,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); } } @@ -598,7 +603,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); @@ -675,7 +680,7 @@ impl Step for CompiletestTest { &[], ); cargo.allow_features("test"); - run_cargo_test(cargo, &[], &[], compiler, host, builder); + run_cargo_test(cargo, &[], &[], "compiletest", compiler, host, builder); } } @@ -718,17 +723,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. @@ -2037,11 +2038,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) } @@ -2051,6 +2054,7 @@ fn prepare_cargo_test( cargo: impl Into, libtest_args: &[&str], crates: &[Interned], + primary_crate: &str, compiler: Compiler, target: TargetSelection, builder: &Builder<'_>, @@ -2068,7 +2072,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 => {} } @@ -2181,7 +2192,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); } } @@ -2280,6 +2291,7 @@ impl Step for CrateRustdoc { cargo, &[], &[INTERNER.intern_str("rustdoc:0.0.0")], + "rustdoc", compiler, target, builder, @@ -2346,6 +2358,7 @@ impl Step for CrateRustdocJsonTypes { cargo, libtest_args, &[INTERNER.intern_str("rustdoc-json-types")], + "rustdoc-json-types", compiler, target, builder, @@ -2505,7 +2518,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<'_> { @@ -2618,7 +2631,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