From 1afeeef77d7cc55d37bb00642716b78453d2d476 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 31 Dec 2024 12:14:31 +1100 Subject: [PATCH 1/4] Move most of the `Step::run` impl out of `tool_check_step!` Ordinary code is much easier to work with than macro-generated code. --- src/bootstrap/src/core/build_steps/check.rs | 86 ++++++++++----------- 1 file changed, 41 insertions(+), 45 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index b4d37b25a6c70..81c69cbd5dbdf 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -428,53 +428,49 @@ macro_rules! tool_check_step { } fn run(self, builder: &Builder<'_>) { - let compiler = builder.compiler(builder.top_stage, builder.config.build); - let target = self.target; - - builder.ensure(Rustc::new(target, builder)); - - let mut cargo = prepare_tool_cargo( - builder, - compiler, - Mode::ToolRustc, - target, - builder.kind, - $path, - $source_type, - &[], - ); - - // For ./x.py clippy, don't run with --all-targets because - // linting tests and benchmarks can produce very noisy results - if builder.kind != Kind::Clippy { - cargo.arg("--all-targets"); - } - - let _guard = builder.msg_check(&format!("{} artifacts", $display_name), target); - run_cargo( - builder, - cargo, - builder.config.free_args.clone(), - &stamp(builder, compiler, target), - vec![], - true, - false, - ); - - /// Cargo's output path in a given stage, compiled by a particular - /// compiler for the specified target. - fn stamp( - builder: &Builder<'_>, - compiler: Compiler, - target: TargetSelection, - ) -> PathBuf { - builder - .cargo_out(compiler, Mode::ToolRustc, target) - .join(format!(".{}-check.stamp", stringify!($name).to_lowercase())) - } + let Self { target } = self; + run_tool_check_step(builder, target, stringify!($name), $display_name, $path, $source_type); } } - }; + } +} + +/// Used by the implementation of `Step::run` in `tool_check_step!`. +fn run_tool_check_step( + builder: &Builder<'_>, + target: TargetSelection, + step_type_name: &str, + display_name: &str, + path: &str, + source_type: SourceType, +) { + let compiler = builder.compiler(builder.top_stage, builder.config.build); + + builder.ensure(Rustc::new(target, builder)); + + let mut cargo = prepare_tool_cargo( + builder, + compiler, + Mode::ToolRustc, + target, + builder.kind, + path, + source_type, + &[], + ); + + // For ./x.py clippy, don't run with --all-targets because + // linting tests and benchmarks can produce very noisy results + if builder.kind != Kind::Clippy { + cargo.arg("--all-targets"); + } + + let stamp = builder + .cargo_out(compiler, Mode::ToolRustc, target) + .join(format!(".{}-check.stamp", step_type_name.to_lowercase())); + + let _guard = builder.msg_check(format!("{display_name} artifacts"), target); + run_cargo(builder, cargo, builder.config.free_args.clone(), &stamp, vec![], true, false); } tool_check_step!(Rustdoc, "rustdoc", "src/tools/rustdoc", "src/librustdoc", SourceType::InTree); From c59ccae7393e62735d529c1d7b7e3d6ff90e1049 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 31 Dec 2024 12:33:24 +1100 Subject: [PATCH 2/4] Infer tool-check-step display name from the last path component --- src/bootstrap/src/core/build_steps/check.rs | 37 ++++++++------------- 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index 81c69cbd5dbdf..15d7aea52a645 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -402,7 +402,6 @@ impl Step for RustAnalyzer { macro_rules! tool_check_step { ( $name:ident, - $display_name:literal, $path:literal, $($alias:literal, )* $source_type:path @@ -429,7 +428,7 @@ macro_rules! tool_check_step { fn run(self, builder: &Builder<'_>) { let Self { target } = self; - run_tool_check_step(builder, target, stringify!($name), $display_name, $path, $source_type); + run_tool_check_step(builder, target, stringify!($name), $path, $source_type); } } } @@ -440,10 +439,10 @@ fn run_tool_check_step( builder: &Builder<'_>, target: TargetSelection, step_type_name: &str, - display_name: &str, path: &str, source_type: SourceType, ) { + let display_name = path.rsplit('/').next().unwrap(); let compiler = builder.compiler(builder.top_stage, builder.config.build); builder.ensure(Rustc::new(target, builder)); @@ -473,33 +472,23 @@ fn run_tool_check_step( run_cargo(builder, cargo, builder.config.free_args.clone(), &stamp, vec![], true, false); } -tool_check_step!(Rustdoc, "rustdoc", "src/tools/rustdoc", "src/librustdoc", SourceType::InTree); +tool_check_step!(Rustdoc, "src/tools/rustdoc", "src/librustdoc", SourceType::InTree); // Clippy, miri and Rustfmt are hybrids. They are external tools, but use a git subtree instead // of a submodule. Since the SourceType only drives the deny-warnings // behavior, treat it as in-tree so that any new warnings in clippy will be // rejected. -tool_check_step!(Clippy, "clippy", "src/tools/clippy", SourceType::InTree); -tool_check_step!(Miri, "miri", "src/tools/miri", SourceType::InTree); -tool_check_step!(CargoMiri, "cargo-miri", "src/tools/miri/cargo-miri", SourceType::InTree); -tool_check_step!(Rls, "rls", "src/tools/rls", SourceType::InTree); -tool_check_step!(Rustfmt, "rustfmt", "src/tools/rustfmt", SourceType::InTree); -tool_check_step!( - MiroptTestTools, - "miropt-test-tools", - "src/tools/miropt-test-tools", - SourceType::InTree -); -tool_check_step!( - TestFloatParse, - "test-float-parse", - "src/etc/test-float-parse", - SourceType::InTree -); - -tool_check_step!(Bootstrap, "bootstrap", "src/bootstrap", SourceType::InTree, false); +tool_check_step!(Clippy, "src/tools/clippy", SourceType::InTree); +tool_check_step!(Miri, "src/tools/miri", SourceType::InTree); +tool_check_step!(CargoMiri, "src/tools/miri/cargo-miri", SourceType::InTree); +tool_check_step!(Rls, "src/tools/rls", SourceType::InTree); +tool_check_step!(Rustfmt, "src/tools/rustfmt", SourceType::InTree); +tool_check_step!(MiroptTestTools, "src/tools/miropt-test-tools", SourceType::InTree); +tool_check_step!(TestFloatParse, "src/etc/test-float-parse", SourceType::InTree); + +tool_check_step!(Bootstrap, "src/bootstrap", SourceType::InTree, false); // Compiletest is implicitly "checked" when it gets built in order to run tests, // so this is mainly for people working on compiletest to run locally. -tool_check_step!(Compiletest, "compiletest", "src/tools/compiletest", SourceType::InTree, false); +tool_check_step!(Compiletest, "src/tools/compiletest", SourceType::InTree, false); /// Cargo's output path for the standard library in a given stage, compiled /// by a particular compiler for the specified target. From 774e83cea1fb3d6ddf15f50405206ae7c9cb0761 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 31 Dec 2024 13:01:58 +1100 Subject: [PATCH 3/4] Make `tool_check_step!` always assume `SourceType::InTree` All of the tools that use this macro are currently in-tree, so support for specifying a `SourceType` was not meaningfully used. It can potentially be re-added in the future if needed. --- src/bootstrap/src/core/build_steps/check.rs | 44 +++++++++++---------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index 15d7aea52a645..1ea28901363e2 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -402,10 +402,9 @@ impl Step for RustAnalyzer { macro_rules! tool_check_step { ( $name:ident, - $path:literal, - $($alias:literal, )* - $source_type:path - $(, $default:literal )? + $path:literal + $(, alt_path: $alt_path:literal )* + $(, default: $default:literal )? ) => { #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct $name { @@ -415,11 +414,11 @@ macro_rules! tool_check_step { impl Step for $name { type Output = (); const ONLY_HOSTS: bool = true; - /// don't ever check out-of-tree tools by default, they'll fail when toolstate is broken - const DEFAULT: bool = matches!($source_type, SourceType::InTree) $( && $default )?; + /// Most of the tool-checks using this macro are run by default. + const DEFAULT: bool = true $( && $default )?; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - run.paths(&[ $path, $($alias),* ]) + run.paths(&[ $path, $( $alt_path ),* ]) } fn make_run(run: RunConfig<'_>) { @@ -428,7 +427,7 @@ macro_rules! tool_check_step { fn run(self, builder: &Builder<'_>) { let Self { target } = self; - run_tool_check_step(builder, target, stringify!($name), $path, $source_type); + run_tool_check_step(builder, target, stringify!($name), $path); } } } @@ -440,7 +439,6 @@ fn run_tool_check_step( target: TargetSelection, step_type_name: &str, path: &str, - source_type: SourceType, ) { let display_name = path.rsplit('/').next().unwrap(); let compiler = builder.compiler(builder.top_stage, builder.config.build); @@ -454,7 +452,11 @@ fn run_tool_check_step( target, builder.kind, path, - source_type, + // Currently, all of the tools that use this macro/function are in-tree. + // If support for out-of-tree tools is re-added in the future, those + // steps should probably be marked non-default so that the default + // checks aren't affected by toolstate being broken. + SourceType::InTree, &[], ); @@ -472,23 +474,23 @@ fn run_tool_check_step( run_cargo(builder, cargo, builder.config.free_args.clone(), &stamp, vec![], true, false); } -tool_check_step!(Rustdoc, "src/tools/rustdoc", "src/librustdoc", SourceType::InTree); +tool_check_step!(Rustdoc, "src/tools/rustdoc", alt_path: "src/librustdoc"); // Clippy, miri and Rustfmt are hybrids. They are external tools, but use a git subtree instead // of a submodule. Since the SourceType only drives the deny-warnings // behavior, treat it as in-tree so that any new warnings in clippy will be // rejected. -tool_check_step!(Clippy, "src/tools/clippy", SourceType::InTree); -tool_check_step!(Miri, "src/tools/miri", SourceType::InTree); -tool_check_step!(CargoMiri, "src/tools/miri/cargo-miri", SourceType::InTree); -tool_check_step!(Rls, "src/tools/rls", SourceType::InTree); -tool_check_step!(Rustfmt, "src/tools/rustfmt", SourceType::InTree); -tool_check_step!(MiroptTestTools, "src/tools/miropt-test-tools", SourceType::InTree); -tool_check_step!(TestFloatParse, "src/etc/test-float-parse", SourceType::InTree); - -tool_check_step!(Bootstrap, "src/bootstrap", SourceType::InTree, false); +tool_check_step!(Clippy, "src/tools/clippy"); +tool_check_step!(Miri, "src/tools/miri"); +tool_check_step!(CargoMiri, "src/tools/miri/cargo-miri"); +tool_check_step!(Rls, "src/tools/rls"); +tool_check_step!(Rustfmt, "src/tools/rustfmt"); +tool_check_step!(MiroptTestTools, "src/tools/miropt-test-tools"); +tool_check_step!(TestFloatParse, "src/etc/test-float-parse"); + +tool_check_step!(Bootstrap, "src/bootstrap", default: false); // Compiletest is implicitly "checked" when it gets built in order to run tests, // so this is mainly for people working on compiletest to run locally. -tool_check_step!(Compiletest, "src/tools/compiletest", SourceType::InTree, false); +tool_check_step!(Compiletest, "src/tools/compiletest", default: false); /// Cargo's output path for the standard library in a given stage, compiled /// by a particular compiler for the specified target. From 66fd5340eaf1792f845e6dbfda7063c9b8d856c4 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 31 Dec 2024 13:04:42 +1100 Subject: [PATCH 4/4] Use struct-like syntax in `tool_check_step!` This tricks rustfmt into formatting the macro arguments as expressions, instead of giving up and ignoring them. --- src/bootstrap/src/core/build_steps/check.rs | 33 +++++++++++---------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index 1ea28901363e2..9434d876df8e8 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -401,10 +401,13 @@ impl Step for RustAnalyzer { macro_rules! tool_check_step { ( - $name:ident, - $path:literal - $(, alt_path: $alt_path:literal )* - $(, default: $default:literal )? + $name:ident { + // The part of this path after the final '/' is also used as a display name. + path: $path:literal + $(, alt_path: $alt_path:literal )* + $(, default: $default:literal )? + $( , )? + } ) => { #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct $name { @@ -474,23 +477,23 @@ fn run_tool_check_step( run_cargo(builder, cargo, builder.config.free_args.clone(), &stamp, vec![], true, false); } -tool_check_step!(Rustdoc, "src/tools/rustdoc", alt_path: "src/librustdoc"); +tool_check_step!(Rustdoc { path: "src/tools/rustdoc", alt_path: "src/librustdoc" }); // Clippy, miri and Rustfmt are hybrids. They are external tools, but use a git subtree instead // of a submodule. Since the SourceType only drives the deny-warnings // behavior, treat it as in-tree so that any new warnings in clippy will be // rejected. -tool_check_step!(Clippy, "src/tools/clippy"); -tool_check_step!(Miri, "src/tools/miri"); -tool_check_step!(CargoMiri, "src/tools/miri/cargo-miri"); -tool_check_step!(Rls, "src/tools/rls"); -tool_check_step!(Rustfmt, "src/tools/rustfmt"); -tool_check_step!(MiroptTestTools, "src/tools/miropt-test-tools"); -tool_check_step!(TestFloatParse, "src/etc/test-float-parse"); - -tool_check_step!(Bootstrap, "src/bootstrap", default: false); +tool_check_step!(Clippy { path: "src/tools/clippy" }); +tool_check_step!(Miri { path: "src/tools/miri" }); +tool_check_step!(CargoMiri { path: "src/tools/miri/cargo-miri" }); +tool_check_step!(Rls { path: "src/tools/rls" }); +tool_check_step!(Rustfmt { path: "src/tools/rustfmt" }); +tool_check_step!(MiroptTestTools { path: "src/tools/miropt-test-tools" }); +tool_check_step!(TestFloatParse { path: "src/etc/test-float-parse" }); + +tool_check_step!(Bootstrap { path: "src/bootstrap", default: false }); // Compiletest is implicitly "checked" when it gets built in order to run tests, // so this is mainly for people working on compiletest to run locally. -tool_check_step!(Compiletest, "src/tools/compiletest", default: false); +tool_check_step!(Compiletest { path: "src/tools/compiletest", default: false }); /// Cargo's output path for the standard library in a given stage, compiled /// by a particular compiler for the specified target.