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.

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

stabilize stage management for rustc tools #137215

Merged
merged 16 commits into from
Feb 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/bootstrap/defaults/config.tools.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ incremental = true
download-rustc = "if-unchanged"

[build]
# cargo and clippy tests don't pass on stage 1
test-stage = 2
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a good default -- it should be considered a bug if the tests do not work in stage 1. Certainly, they do work for Miri, so this can lead to Miri contributors waiting unnecessarily long for their builds to finish.

Copy link
Member Author

Choose a reason for hiding this comment

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

#137522 should be enough for the stage1 problems happen on clippy and cargo tests, so this should be revertable.

Copy link
Member

Choose a reason for hiding this comment

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

So what that does is just bump the stage to 2? I guess that hides the bugs with those tools...

I have #78717 somewhere in my TODO list, so I'll have to remember to revert that then. I don't know why cargo needs stage 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't get how #78717 is being related to test-stage = 2 in the config file.

So what that does is just bump the stage to 2? I guess that hides the bugs with those tools...

Actually I don't know if it's a bug or not. All other tools work fine but cargo and clippy fail on stage 1 tests.

Copy link
Member

Choose a reason for hiding this comment

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

#78717 tracks the bug that you need stage 2 for clippy. Once that is fixed, clippy can be tested on stage 1.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I don't know if it's a bug or not.

I am arguing that we should consider it a bug. We should have the expectation that tools can be tested in stage 1 (except when they really need the bootstrap loop to have completed, which is quite rare).

# Document with the in-tree rustdoc by default, since `download-rustc` makes it quick to compile.
doc-stage = 2
# Contributors working on tools will probably expect compiler docs to be generated, so they can figure out how to use the API.
Expand Down
26 changes: 13 additions & 13 deletions src/bootstrap/src/core/build_steps/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1977,13 +1977,14 @@ impl Step for Assemble {
let maybe_install_llvm_bitcode_linker = |compiler| {
if builder.config.llvm_bitcode_linker_enabled {
trace!("llvm-bitcode-linker enabled, installing");
let src_path = builder.ensure(crate::core::build_steps::tool::LlvmBitcodeLinker {
compiler,
target: target_compiler.host,
extra_features: vec![],
});
let llvm_bitcode_linker =
builder.ensure(crate::core::build_steps::tool::LlvmBitcodeLinker {
compiler,
target: target_compiler.host,
extra_features: vec![],
});
let tool_exe = exe("llvm-bitcode-linker", target_compiler.host);
builder.copy_link(&src_path, &libdir_bin.join(tool_exe));
builder.copy_link(&llvm_bitcode_linker.tool_path, &libdir_bin.join(tool_exe));
}
};

Expand Down Expand Up @@ -2171,14 +2172,13 @@ impl Step for Assemble {
// logic to create the final binary. This is used by the
// `wasm32-wasip2` target of Rust.
if builder.tool_enabled("wasm-component-ld") {
let wasm_component_ld_exe =
builder.ensure(crate::core::build_steps::tool::WasmComponentLd {
compiler: build_compiler,
target: target_compiler.host,
});
let wasm_component = builder.ensure(crate::core::build_steps::tool::WasmComponentLd {
compiler: build_compiler,
target: target_compiler.host,
});
builder.copy_link(
&wasm_component_ld_exe,
&libdir_bin.join(wasm_component_ld_exe.file_name().unwrap()),
&wasm_component.tool_path,
&libdir_bin.join(wasm_component.tool_path.file_name().unwrap()),
);
}

Expand Down
22 changes: 11 additions & 11 deletions src/bootstrap/src/core/build_steps/dist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ impl Step for Rustc {
},
builder.kind,
) {
builder.install(&ra_proc_macro_srv, &image.join("libexec"), 0o755);
builder.install(&ra_proc_macro_srv.tool_path, &image.join("libexec"), 0o755);
}

let libdir_relative = builder.libdir_relative(compiler);
Expand Down Expand Up @@ -1134,7 +1134,7 @@ impl Step for Cargo {
let mut tarball = Tarball::new(builder, "cargo", &target.triple);
tarball.set_overlay(OverlayKind::Cargo);

tarball.add_file(cargo, "bin", 0o755);
tarball.add_file(cargo.tool_path, "bin", 0o755);
tarball.add_file(etc.join("_cargo"), "share/zsh/site-functions", 0o644);
tarball.add_renamed_file(etc.join("cargo.bashcomp.sh"), "etc/bash_completion.d", "cargo");
tarball.add_dir(etc.join("man"), "share/man/man1");
Expand Down Expand Up @@ -1180,7 +1180,7 @@ impl Step for Rls {
let mut tarball = Tarball::new(builder, "rls", &target.triple);
tarball.set_overlay(OverlayKind::Rls);
tarball.is_preview(true);
tarball.add_file(rls, "bin", 0o755);
tarball.add_file(rls.tool_path, "bin", 0o755);
tarball.add_legal_and_readme_to("share/doc/rls");
Some(tarball.generate())
}
Expand Down Expand Up @@ -1222,7 +1222,7 @@ impl Step for RustAnalyzer {
let mut tarball = Tarball::new(builder, "rust-analyzer", &target.triple);
tarball.set_overlay(OverlayKind::RustAnalyzer);
tarball.is_preview(true);
tarball.add_file(rust_analyzer, "bin", 0o755);
tarball.add_file(rust_analyzer.tool_path, "bin", 0o755);
tarball.add_legal_and_readme_to("share/doc/rust-analyzer");
Some(tarball.generate())
}
Expand Down Expand Up @@ -1268,8 +1268,8 @@ impl Step for Clippy {
let mut tarball = Tarball::new(builder, "clippy", &target.triple);
tarball.set_overlay(OverlayKind::Clippy);
tarball.is_preview(true);
tarball.add_file(clippy, "bin", 0o755);
tarball.add_file(cargoclippy, "bin", 0o755);
tarball.add_file(clippy.tool_path, "bin", 0o755);
tarball.add_file(cargoclippy.tool_path, "bin", 0o755);
tarball.add_legal_and_readme_to("share/doc/clippy");
Some(tarball.generate())
}
Expand Down Expand Up @@ -1318,8 +1318,8 @@ impl Step for Miri {
let mut tarball = Tarball::new(builder, "miri", &target.triple);
tarball.set_overlay(OverlayKind::Miri);
tarball.is_preview(true);
tarball.add_file(miri, "bin", 0o755);
tarball.add_file(cargomiri, "bin", 0o755);
tarball.add_file(miri.tool_path, "bin", 0o755);
tarball.add_file(cargomiri.tool_path, "bin", 0o755);
tarball.add_legal_and_readme_to("share/doc/miri");
Some(tarball.generate())
}
Expand Down Expand Up @@ -1449,8 +1449,8 @@ impl Step for Rustfmt {
let mut tarball = Tarball::new(builder, "rustfmt", &target.triple);
tarball.set_overlay(OverlayKind::Rustfmt);
tarball.is_preview(true);
tarball.add_file(rustfmt, "bin", 0o755);
tarball.add_file(cargofmt, "bin", 0o755);
tarball.add_file(rustfmt.tool_path, "bin", 0o755);
tarball.add_file(cargofmt.tool_path, "bin", 0o755);
tarball.add_legal_and_readme_to("share/doc/rustfmt");
Some(tarball.generate())
}
Expand Down Expand Up @@ -2272,7 +2272,7 @@ impl Step for LlvmBitcodeLinker {
tarball.set_overlay(OverlayKind::LlvmBitcodeLinker);
tarball.is_preview(true);

tarball.add_file(llbc_linker, self_contained_bin_dir, 0o755);
tarball.add_file(llbc_linker.tool_path, self_contained_bin_dir, 0o755);

Some(tarball.generate())
}
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/src/core/build_steps/perf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ Consider setting `rust.debuginfo-level = 1` in `config.toml`."#);
let results_dir = rustc_perf_dir.join("results");
builder.create_dir(&results_dir);

let mut cmd = command(collector);
let mut cmd = command(collector.tool_path);

// We need to set the working directory to `src/tools/rustc-perf`, so that it can find the directory
// with compile-time benchmarks.
Expand Down
6 changes: 1 addition & 5 deletions src/bootstrap/src/core/build_steps/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,7 @@ impl Step for Miri {

// This compiler runs on the host, we'll just use it for the target.
let target_compiler = builder.compiler(stage, host);
// Similar to `compile::Assemble`, build with the previous stage's compiler. Otherwise
// we'd have stageN/bin/rustc and stageN/bin/rustdoc be effectively different stage
// compilers, which isn't what we want. Rustdoc should be linked in the same way as the
// rustc compiler it's paired with, so it must be built with the previous stage compiler.
let host_compiler = builder.compiler(stage - 1, host);
let host_compiler = tool::get_tool_rustc_compiler(builder, target_compiler);

// Get a target sysroot for Miri.
let miri_sysroot = test::Miri::build_miri_sysroot(builder, target_compiler, target);
Expand Down
66 changes: 35 additions & 31 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ impl Step for Cargotest {

let _time = helpers::timeit(builder);
let mut cmd = builder.tool_cmd(Tool::CargoTest);
cmd.arg(&cargo)
cmd.arg(&cargo.tool_path)
.arg(&out_dir)
.args(builder.config.test_args())
.env("RUSTC", builder.rustc(compiler))
Expand Down Expand Up @@ -298,9 +298,16 @@ impl Step for Cargo {

/// Runs `cargo test` for `cargo` packaged with Rust.
fn run(self, builder: &Builder<'_>) {
if self.stage < 2 {
eprintln!("WARNING: cargo tests on stage {} may not behave well.", self.stage);
eprintln!("HELP: consider using stage 2");
}

let compiler = builder.compiler(self.stage, self.host);

builder.ensure(tool::Cargo { compiler, target: self.host });
let cargo = builder.ensure(tool::Cargo { compiler, target: self.host });
let compiler = cargo.build_compiler;

let cargo = tool::prepare_tool_cargo(
builder,
compiler,
Expand Down Expand Up @@ -367,6 +374,7 @@ impl Step for RustAnalyzer {
let stage = self.stage;
let host = self.host;
let compiler = builder.compiler(stage, host);
let compiler = tool::get_tool_rustc_compiler(builder, compiler);

// We don't need to build the whole Rust Analyzer for the proc-macro-srv test suite,
// but we do need the standard library to be present.
Expand Down Expand Up @@ -427,7 +435,8 @@ impl Step for Rustfmt {
let host = self.host;
let compiler = builder.compiler(stage, host);

builder.ensure(tool::Rustfmt { compiler, target: self.host });
let tool_result = builder.ensure(tool::Rustfmt { compiler, target: self.host });
let compiler = tool_result.build_compiler;

let mut cargo = tool::prepare_tool_cargo(
builder,
Expand Down Expand Up @@ -522,16 +531,11 @@ impl Step for Miri {

// This compiler runs on the host, we'll just use it for the target.
let target_compiler = builder.compiler(stage, host);
// Similar to `compile::Assemble`, build with the previous stage's compiler. Otherwise
// we'd have stageN/bin/rustc and stageN/bin/rustdoc be effectively different stage
// compilers, which isn't what we want. Rustdoc should be linked in the same way as the
// rustc compiler it's paired with, so it must be built with the previous stage compiler.
let host_compiler = builder.compiler(stage - 1, host);

// Build our tools.
let miri = builder.ensure(tool::Miri { compiler: host_compiler, target: host });
let miri = builder.ensure(tool::Miri { compiler: target_compiler, target: host });
// the ui tests also assume cargo-miri has been built
builder.ensure(tool::CargoMiri { compiler: host_compiler, target: host });
builder.ensure(tool::CargoMiri { compiler: target_compiler, target: host });

// We also need sysroots, for Miri and for the host (the latter for build scripts).
// This is for the tests so everything is done with the target compiler.
Expand All @@ -542,7 +546,8 @@ impl Step for Miri {
// Miri has its own "target dir" for ui test dependencies. Make sure it gets cleared when
// the sysroot gets rebuilt, to avoid "found possibly newer version of crate `std`" errors.
if !builder.config.dry_run() {
let ui_test_dep_dir = builder.stage_out(host_compiler, Mode::ToolStd).join("miri_ui");
let ui_test_dep_dir =
builder.stage_out(miri.build_compiler, Mode::ToolStd).join("miri_ui");
// The mtime of `miri_sysroot` changes when the sysroot gets rebuilt (also see
// <https://github.com/RalfJung/rustc-build-sysroot/commit/10ebcf60b80fe2c3dc765af0ff19fdc0da4b7466>).
// We can hence use that directly as a signal to clear the ui test dir.
Expand All @@ -553,7 +558,7 @@ impl Step for Miri {
// This is with the Miri crate, so it uses the host compiler.
let mut cargo = tool::prepare_tool_cargo(
builder,
host_compiler,
miri.build_compiler,
Mode::ToolRustc,
host,
Kind::Test,
Expand All @@ -571,7 +576,7 @@ impl Step for Miri {
// miri tests need to know about the stage sysroot
cargo.env("MIRI_SYSROOT", &miri_sysroot);
cargo.env("MIRI_HOST_SYSROOT", &host_sysroot);
cargo.env("MIRI", &miri);
cargo.env("MIRI", &miri.tool_path);

// Set the target.
cargo.env("MIRI_TEST_TARGET", target.rustc_target_arg());
Expand Down Expand Up @@ -743,7 +748,13 @@ impl Step for Clippy {
let host = self.host;
let compiler = builder.compiler(stage, host);

builder.ensure(tool::Clippy { compiler, target: self.host });
if stage < 2 {
eprintln!("WARNING: clippy tests on stage {stage} may not behave well.");
eprintln!("HELP: consider using stage 2");
}

let tool_result = builder.ensure(tool::Clippy { compiler, target: self.host });
let compiler = tool_result.build_compiler;
let mut cargo = tool::prepare_tool_cargo(
builder,
compiler,
Expand Down Expand Up @@ -1720,18 +1731,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
// If we're using `--stage 0`, we should provide the bootstrap cargo.
builder.initial_cargo.clone()
} else {
// We need to properly build cargo using the suitable stage compiler.

let compiler = builder.download_rustc().then_some(compiler).unwrap_or_else(||
// HACK: currently tool stages are off-by-one compared to compiler stages, i.e. if
// you give `tool::Cargo` a stage 1 rustc, it will cause stage 2 rustc to be built
// and produce a cargo built with stage 2 rustc. To fix this, we need to chop off
// the compiler stage by 1 to align with expected `./x test run-make --stage N`
// behavior, i.e. we need to pass `N - 1` compiler stage to cargo. See also Miri
// which does a similar hack.
builder.compiler(builder.top_stage - 1, compiler.host));

builder.ensure(tool::Cargo { compiler, target: compiler.host })
builder.ensure(tool::Cargo { compiler, target: compiler.host }).tool_path
};

cmd.arg("--cargo-path").arg(cargo_path);
Expand All @@ -1752,9 +1752,10 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
// Use the beta compiler for jsondocck
let json_compiler = compiler.with_stage(0);
cmd.arg("--jsondocck-path")
.arg(builder.ensure(tool::JsonDocCk { compiler: json_compiler, target }));
cmd.arg("--jsondoclint-path")
.arg(builder.ensure(tool::JsonDocLint { compiler: json_compiler, target }));
.arg(builder.ensure(tool::JsonDocCk { compiler: json_compiler, target }).tool_path);
cmd.arg("--jsondoclint-path").arg(
builder.ensure(tool::JsonDocLint { compiler: json_compiler, target }).tool_path,
);
}

if matches!(mode, "coverage-map" | "coverage-run") {
Expand Down Expand Up @@ -2989,12 +2990,15 @@ impl Step for RemoteCopyLibs {

builder.info(&format!("REMOTE copy libs to emulator ({target})"));

let server = builder.ensure(tool::RemoteTestServer { compiler, target });
let remote_test_server = builder.ensure(tool::RemoteTestServer { compiler, target });

// Spawn the emulator and wait for it to come online
let tool = builder.tool_exe(Tool::RemoteTestClient);
let mut cmd = command(&tool);
cmd.arg("spawn-emulator").arg(target.triple).arg(&server).arg(builder.tempdir());
cmd.arg("spawn-emulator")
.arg(target.triple)
.arg(&remote_test_server.tool_path)
.arg(builder.tempdir());
if let Some(rootfs) = builder.qemu_rootfs(target) {
cmd.arg(rootfs);
}
Expand Down
Loading
Loading