Skip to content

Commit

Permalink
Add must_use to msg_ functions
Browse files Browse the repository at this point in the history 
This caught several places which weren't waiting until the command finished to drop the Group.

I also took the liberty of calling `msg_sysroot_tool` from `run_cargo_test` to reduce code duplication and make errors like this less likely in the future.
  • Loading branch information
@jyn514
jyn514 committed Jul 14, 2023
1 parent fff8223 commit 26cdf75
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 45 deletions.
8 changes: 4 additions & 4 deletions src/bootstrap/doc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ impl Step for TheBook {
let shared_assets = builder.ensure(SharedAssets { target });

// build the redirect pages
builder.msg_doc(compiler, "book redirect pages", target);
let _guard = builder.msg_doc(compiler, "book redirect pages", target);
for file in t!(fs::read_dir(builder.src.join(&relative_path).join("redirects"))) {
let file = t!(file);
let path = file.path();
Expand Down Expand Up @@ -305,7 +305,7 @@ impl Step for Standalone {
fn run(self, builder: &Builder<'_>) {
let target = self.target;
let compiler = self.compiler;
builder.msg_doc(compiler, "standalone", target);
let _guard = builder.msg_doc(compiler, "standalone", target);
let out = builder.doc_out(target);
t!(fs::create_dir_all(&out));

Expand Down Expand Up @@ -799,8 +799,6 @@ macro_rules! tool_doc {
SourceType::Submodule
};

builder.msg_doc(compiler, stringify!($tool).to_lowercase(), target);

// Symlink compiler docs to the output directory of rustdoc documentation.
let out_dirs = [
builder.stage_out(compiler, Mode::ToolRustc).join(target.triple).join("doc"),
Expand Down Expand Up @@ -839,6 +837,8 @@ macro_rules! tool_doc {
cargo.rustdocflag("--show-type-layout");
cargo.rustdocflag("--generate-link-to-definition");
cargo.rustdocflag("-Zunstable-options");

let _guard = builder.msg_doc(compiler, stringify!($tool).to_lowercase(), target);
builder.run(&mut cargo.into());
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/bootstrap/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,7 @@ impl Build {
}
}

#[must_use = "Groups should not be dropped until the Step finishes running"]
fn msg_check(
&self,
what: impl Display,
Expand All @@ -1007,6 +1008,7 @@ impl Build {
self.msg(Kind::Check, self.config.stage, what, self.config.build, target)
}

#[must_use = "Groups should not be dropped until the Step finishes running"]
fn msg_doc(
&self,
compiler: Compiler,
Expand All @@ -1016,6 +1018,7 @@ impl Build {
self.msg(Kind::Doc, compiler.stage, what, compiler.host, target.into())
}

#[must_use = "Groups should not be dropped until the Step finishes running"]
fn msg_build(
&self,
compiler: Compiler,
Expand All @@ -1028,6 +1031,7 @@ impl Build {
/// Return a `Group` guard for a [`Step`] that is built for each `--stage`.
///
/// [`Step`]: crate::builder::Step
#[must_use = "Groups should not be dropped until the Step finishes running"]
fn msg(
&self,
action: impl Into<Kind>,
Expand All @@ -1054,6 +1058,7 @@ impl Build {
/// Return a `Group` guard for a [`Step`] that is only built once and isn't affected by `--stage`.
///
/// [`Step`]: crate::builder::Step
#[must_use = "Groups should not be dropped until the Step finishes running"]
fn msg_unstaged(
&self,
action: impl Into<Kind>,
Expand All @@ -1065,6 +1070,7 @@ impl Build {
self.group(&msg)
}

#[must_use = "Groups should not be dropped until the Step finishes running"]
fn msg_sysroot_tool(
&self,
action: impl Into<Kind>,
Expand Down
98 changes: 57 additions & 41 deletions src/bootstrap/test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,8 @@ impl Step for CrateBootstrap {
SourceType::InTree,
&[],
);
let _group = builder.msg(Kind::Test, compiler.stage, path, compiler.host, bootstrap_host);
let crate_name = path.rsplit_once('/').unwrap().1;
run_cargo_test(cargo, &[], &[], crate_name, compiler, bootstrap_host, builder);
run_cargo_test(cargo, &[], &[], crate_name, crate_name, compiler, bootstrap_host, builder);
}
}

Expand Down Expand Up @@ -159,14 +158,6 @@ You can skip linkcheck with --exclude src/tools/linkchecker"
let bootstrap_host = builder.config.build;
let compiler = builder.compiler(0, bootstrap_host);

let self_test_group = builder.msg(
Kind::Test,
compiler.stage,
"linkchecker self tests",
bootstrap_host,
bootstrap_host,
);

let cargo = tool::prepare_tool_cargo(
builder,
compiler,
Expand All @@ -177,8 +168,16 @@ You can skip linkcheck with --exclude src/tools/linkchecker"
SourceType::InTree,
&[],
);
run_cargo_test(cargo, &[], &[], "linkchecker", compiler, bootstrap_host, builder);
drop(self_test_group);
run_cargo_test(
cargo,
&[],
&[],
"linkchecker",
"linkchecker self tests",
compiler,
bootstrap_host,
builder,
);

if builder.doc_tests == DocTests::No {
return;
Expand Down Expand Up @@ -413,8 +412,7 @@ impl Step for RustAnalyzer {
cargo.env("SKIP_SLOW_TESTS", "1");

cargo.add_rustc_lib_path(builder, compiler);
builder.msg_sysroot_tool(Kind::Test, compiler.stage, "rust-analyzer", host, host);
run_cargo_test(cargo, &[], &[], "rust-analyzer", compiler, host, builder);
run_cargo_test(cargo, &[], &[], "rust-analyzer", "rust-analyzer", compiler, host, builder);
}
}

Expand Down Expand Up @@ -463,8 +461,7 @@ impl Step for Rustfmt {

cargo.add_rustc_lib_path(builder, compiler);

builder.msg_sysroot_tool(Kind::Test, compiler.stage, "rustfmt", host, host);
run_cargo_test(cargo, &[], &[], "rustfmt", compiler, host, builder);
run_cargo_test(cargo, &[], &[], "rustfmt", "rustfmt", compiler, host, builder);
}
}

Expand Down Expand Up @@ -512,7 +509,16 @@ impl Step for RustDemangler {
cargo.env("RUST_DEMANGLER_DRIVER_PATH", rust_demangler);
cargo.add_rustc_lib_path(builder, compiler);

run_cargo_test(cargo, &[], &[], "rust-demangler", compiler, host, builder);
run_cargo_test(
cargo,
&[],
&[],
"rust-demangler",
"rust-demangler",
compiler,
host,
builder,
);
}
}

Expand Down Expand Up @@ -754,7 +760,16 @@ impl Step for CompiletestTest {
&[],
);
cargo.allow_features("test");
run_cargo_test(cargo, &[], &[], "compiletest", compiler, host, builder);
run_cargo_test(
cargo,
&[],
&[],
"compiletest",
"compiletest self test",
compiler,
host,
builder,
);
}
}

Expand Down Expand Up @@ -810,7 +825,7 @@ impl Step for Clippy {
cargo.env("BLESS", "Gesundheit");
}

builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host);
let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host);

if builder.try_run(&mut cargo).is_ok() {
// The tests succeeded; nothing to do.
Expand Down Expand Up @@ -2201,18 +2216,22 @@ impl Step for CrateLibrustc {
/// Given a `cargo test` subcommand, add the appropriate flags and run it.
///
/// Returns whether the test succeeded.
fn run_cargo_test(
fn run_cargo_test<'a>(
cargo: impl Into<Command>,
libtest_args: &[&str],
crates: &[Interned<String>],
primary_crate: &str,
description: impl Into<Option<&'a str>>,
compiler: Compiler,
target: TargetSelection,
builder: &Builder<'_>,
) -> bool {
let mut cargo =
prepare_cargo_test(cargo, libtest_args, crates, primary_crate, compiler, target, builder);
let _time = util::timeit(&builder);
let _group = description.into().and_then(|what| {
builder.msg_sysroot_tool(Kind::Test, compiler.stage, what, compiler.host, target)
});

#[cfg(feature = "build-metrics")]
builder.metrics.begin_test_suite(
Expand Down Expand Up @@ -2378,14 +2397,16 @@ impl Step for Crate {
_ => panic!("can only test libraries"),
};

let _guard = builder.msg(
builder.kind,
compiler.stage,
crate_description(&self.crates),
compiler.host,
run_cargo_test(
cargo,
&[],
&self.crates,
&self.crates[0],
&*crate_description(&self.crates),
compiler,
target,
builder,
);
run_cargo_test(cargo, &[], &self.crates, &self.crates[0], compiler, target, builder);
}
}

Expand Down Expand Up @@ -2478,18 +2499,12 @@ impl Step for CrateRustdoc {
dylib_path.insert(0, PathBuf::from(&*libdir));
cargo.env(dylib_path_var(), env::join_paths(&dylib_path).unwrap());

let _guard = builder.msg_sysroot_tool(
builder.kind,
compiler.stage,
"rustdoc",
compiler.host,
target,
);
run_cargo_test(
cargo,
&[],
&[INTERNER.intern_str("rustdoc:0.0.0")],
"rustdoc",
"rustdoc",
compiler,
target,
builder,
Expand Down Expand Up @@ -2545,13 +2560,12 @@ impl Step for CrateRustdocJsonTypes {
&[]
};

let _guard =
builder.msg(builder.kind, compiler.stage, "rustdoc-json-types", compiler.host, target);
run_cargo_test(
cargo,
libtest_args,
&[INTERNER.intern_str("rustdoc-json-types")],
"rustdoc-json-types",
"rustdoc-json-types",
compiler,
target,
builder,
Expand Down Expand Up @@ -2722,7 +2736,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"], &[], "bootstrap", compiler, host, builder);
run_cargo_test(cmd, &["--test-threads=1"], &[], "bootstrap", None, compiler, host, builder);
}

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
Expand Down Expand Up @@ -2840,14 +2854,16 @@ impl Step for RustInstaller {
&[],
);

let _guard = builder.msg(
Kind::Test,
compiler.stage,
run_cargo_test(
cargo,
&[],
&[],
"installer",
"rust-installer",
compiler,
bootstrap_host,
bootstrap_host,
builder,
);
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
Expand Down

0 comments on commit 26cdf75

Please sign in to comment.