x.py test --stage 0 src/tools/clippy does not work #78717
Comments
jyn514
added
T-bootstrap
|
The really strange thing here is that clippy is built twice: |
|
This does work fine with |
|
Ok, I dug quite a bit into this. The main issue is that clippy is trying to run tests against the sysroot of the host compiler, not the target compiler. In particular, test --stage 0 is failing because it's trying to use the .rlibs in stage0-sysroot, instead of stage1. I fixed that. The problem now is that clippy wants to link the test files to its own dependencies. That doesn't work while bootstrapping, because its dependencies were built by a different toolchain than the currently running clippy (stage0, not stage1). @rust-lang/clippy why does clippy need to use its own dependencies in tests? Is it possible to write the tests without those? I can fix this, but it will make If it's not possible to do that, can we instead add some dummy package that only depends on |
Because clippy contains lints for working on clippy itself, and they need to be tested too |
|
Also tbh my experience with rustc is that stage0 is basically always useless and should never be used, so it doesn't seem too off for stage 0 to be useless for working on clippy. Maybe we can just disallow this for clippy.
worth noting, this error is kinda also because compiletest doesn't quite know what crates to link to, this happens pretty often in many different scenarios. |
|
@Manishearth the numbering for clippy is inconsistent with the numbering for the rest of the compiler. That error up there is outdated, it was fixed by rust-lang/rust-clippy#7631. The new error is or something similar - I don't have the exact output on hand, but it's related to stage0 vs stage1 sysroot. |
This is hard to do, because cargo only allows one library per package, and doesn't allow building binaries without first building the library :( I guess we could hard-code the dependencies clippy uses in another package somewhere? nested workspaces are still a pipe dream unfortunately ... |
FWIW Miri tests work fine in stage 0, and generally there are very few things that require building the compiler twice these days -- testing clippy is one of them. This makes clippy failures more painful to deal with than most other issues one runs into when working on rustc. |
|
Unfortunately this issue is still present, which means that having to bless clippy tests takes about 2x to 3x as long (and >2x the disk space) as it takes to bless any other part of rustc I've had to bless in the last years -- clippy is literally the only thing that forces me to do a 2nd-stage build.
I assume those tests fail fairly rarely due to rustc changes, so it'd be good if it were possible to at least bless all the other tests without having to wait for rustc bootstrap to finish. |
RalfJung
added
A-contributor-roadblock
@RalfJung |
|
(unless things have radically changed while i'm gone i suspect there will still be other things using stage 2, like |
|
@jyn514 I meant all the clippy tests that don't need to depend on clippy themselves. For the majority of clippy tests, I don't see why they would need a stage 2, so maybe we can require the higher stage only for the tests that actually need it.
Yeah there probably are, but at least with the kind of compiler changes I am doing, you need to re-bless them even less often than clippy. |
|
I have a patch that almost makes this work, by using the same staging logic in clippy that we also already use in Miri. diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 30fdea7e19e..6076763a7d2 100644
--- a/src/bootstrap/src/core/build_steps/test.rs
+++ b/src/bootstrap/src/core/build_steps/test.rs
@@ -505,9 +505,8 @@ fn run(self, builder: &Builder<'_>) {
// 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.
+ // we'd have stageN/bin/rustc and stageN/bin/miri be effectively different stage
+ // compilers, which isn't what we want.
let host_compiler = builder.compiler(stage - 1, host);
// Build our tools.
@@ -738,12 +737,27 @@ fn make_run(run: RunConfig<'_>) {
fn run(self, builder: &Builder<'_>) {
let stage = self.stage;
let host = self.host;
- let compiler = builder.compiler(stage, host);
+ if stage == 0 {
+ eprintln!("clippy cannot be tested at stage 0");
+ std::process::exit(1);
+ }
+ 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/clippy be effectively different stage
+ // compilers, which isn't what we want.
+ let host_compiler = builder.compiler(stage - 1, host);
+
+ // We need clippy itself, and a sysroot for the tests to use.
+ builder.ensure(tool::Clippy {
+ compiler: host_compiler,
+ target: self.host,
+ extra_features: Vec::new(),
+ });
+ builder.ensure(compile::Std::new(target_compiler, host));
- builder.ensure(tool::Clippy { compiler, target: self.host, extra_features: Vec::new() });
let mut cargo = tool::prepare_tool_cargo(
builder,
- compiler,
+ host_compiler,
Mode::ToolRustc,
host,
Kind::Test,
@@ -752,15 +766,16 @@ fn run(self, builder: &Builder<'_>) {
&[],
);
- 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("SYSROOT", builder.sysroot(target_compiler));
+ cargo.env("RUSTC_TEST_SUITE", builder.rustc(host_compiler));
+ cargo.env("RUSTC_LIB_PATH", builder.rustc_libdir(host_compiler));
+ let host_libs = builder.stage_out(host_compiler, Mode::ToolRustc).join(builder.cargo_dir());
cargo.env("HOST_LIBS", host_libs);
cargo.add_rustc_lib_path(builder);
- let cargo = prepare_cargo_test(cargo, &[], &[], "clippy", compiler, host, builder);
+ let cargo = prepare_cargo_test(cargo, &[], &[], "clippy", host_compiler, host, builder);
- let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host);
+ let _guard = builder.msg_sysroot_tool(Kind::Test, stage, "clippy", host, host);
// Clippy reports errors if it blessed the outputs
if cargo.allow_failure().run(builder) {This makes almost all tests pass in @oli-obk are there any plans to switch clippy to also use ui_test's dependency builder, rather than its own home-grown system? That would help a lot with integrating clippy better into the bootstrap staging process. There are two tests that import clippy itself, which will not work in stage 1. However, in all the times I had to bless clippy tests during rustc work, I never had to bless those two tests, so it seems completely reasonable to just exclude those in stage 1 and only run them in a "full-deps" stage 2 build. |
I initially had that, but it increased the clippy test time for a clean build so it was reverted. |
|
Hm, do the dependencies take so long to build? |
|
Context can be found in rust-lang/rust-clippy#11045 |
|
Looking at the diff, seems like that approach involved also building clippy_utils and clippy_lints via the ui_test system. So yeah that would be quite slow as those crates are now built twice. But as I mentioned only 2 tests need those crates. So maybe we can handle all the other dependencies via ui_test, and exclude those 2 tests from stage 1 testing (or maybe entirely exclude them from |
Meta
3478d7c
The text was updated successfully, but these errors were encountered: