Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bootstrap: Overhaul and simplify the tool_check_step! macro #134950

Merged
merged 4 commits into from
Jan 1, 2025
Merged

bootstrap: Overhaul and simplify the tool_check_step! macro #134950

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1afeeef
Move most of the `Step::run` impl out of `tool_check_step!`
Zalathar Dec 31, 2024
c59ccae
Infer tool-check-step display name from the last path component
Zalathar Dec 31, 2024
774e83c
Make `tool_check_step!` always assume `SourceType::InTree`
Zalathar Dec 31, 2024
66fd534
Use struct-like syntax in `tool_check_step!`
Zalathar Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 65 additions & 75 deletions src/bootstrap/src/core/build_steps/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,12 +401,13 @@ impl Step for RustAnalyzer {

macro_rules! tool_check_step {
(
$name:ident,
$display_name:literal,
$path:literal,
$($alias:literal, )*
$source_type:path
$(, $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 {
Expand All @@ -416,94 +417,83 @@ 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<'_>) {
run.builder.ensure($name { target: run.target });
}

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), $path);
}
}
};
}
}

/// Used by the implementation of `Step::run` in `tool_check_step!`.
fn run_tool_check_step(
builder: &Builder<'_>,
target: TargetSelection,
step_type_name: &str,
path: &str,
) {
let display_name = path.rsplit('/').next().unwrap();
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,
// 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,
Comment on lines +458 to +462
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rust-lang/bootstrap do you know if there's a specific reason we support out-of-tree tools? Does idk distros or some other consumer rely on this support?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the general concept of SourceType was introduced in #73297, to control whether warnings are denied by default or not.

At that time, none of the uses of tool_check_step! specified a source type other than InTree. So it's plausible that this macro has never actually been used with out-of-tree tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that this PR only removes explicit SourceType support from one particular macro. If an “out of tree” tool hypothetically needed to be added, it shouldn't be much trouble to either adjust this macro, or declare an alternative step or macro.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm in favor of removing this unused code. If we need support for non-InTree tools, we can add it back when the need arises.

&[],
);

// 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);
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, "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 { 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, "compiletest", "src/tools/compiletest", SourceType::InTree, 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.
Expand Down
Loading