rust (original) (raw)

FTR, in my investigation, the root problem is potentially the concept of the PathSet itself. It admits several issues (some of these are over-simplifications and may be inaccurate or imprecise, but is sufficient to demonstrate the complexity and the issue at hand):

We conflate aliases with on-disk paths -- it's especially weird if there is an on-disk directory that shares the same name as the alias. (Example: compiler).
Test suites are handled differently versus other test-ish/non-test Steps, where test suites seemingly have some kind of "run-all" semantics but non-test-suite Steps have "run-once-consume-a-path" semantics.
There's some very weird interactions with --skip/--exclude when it comes to test suites versus individual test files under each test suite. This is not bootstrap's fault (alone), though -- it has to do with how libtest (through cargo test or wrapped through compiletest) does test filtering based on a substring match on test names that are usually constructed based on the test path verbatim (unless you path --exact, in which case I think a path prefix doesn't work).
Before bootstrap: allow skipping steps with start of path #133492, PathSet::check matches on suffixes (FTR, while this saves typing, this can also collide real on-disk paths versus aliases, i.e. what if there's a top-level directory named test vs library/test?). After bootstrap: allow skipping steps with start of path #133492, PathSet::check matches on prefixes as well. There are of course some more complicated exceptions, special handling and suite_path logic differences, but this is generally an issue because...

Both CodegenGCC and CodegenCranelift test Steps have should_run conditions:

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.paths(&["compiler/rustc_codegen_cranelift"])
}

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.paths(&["compiler/rustc_codegen_gcc"])
}

These test Steps are registered here via describe!:

test::CodegenCranelift,
test::CodegenGCC,

When the cli receives the filter string compiler, seemingly both test Steps would be eligible. However, there is a problem. These two test Step's should_runs don't use suite_path, they use run.paths (not saying that they should use suite_path, either). This means that they are not eligible for suite_path special handling that will try to exercise all the test suites:

// Handle all test suite paths.
// (This is separate from the loop below to avoid having to handle multiple paths in `is_suite_path` somehow.)
paths.retain(\|path
for (desc, should_run) in v.iter().zip(&should_runs) {
if let Some(suite) = should_run.is_suite_path(path) {
desc.maybe_run(builder, vec![suite.clone()]);
return false;
}
}
true
});

Instead, the Codegen{Cranelift,Gcc} test Steps go through the non-suite-path handling, which has "remove-PathSet-once-matched" semantics, so the first one to match (first Step to be registered in the test describe! that is eligible to match the PathSet will "eat" the PathSet) prevents any other non-suite-path test Steps that are registered later from being matched.

for (desc, should_run) in v.iter().zip(&should_runs) {
let pathsets = should_run.pathset_for_paths_removing_matches(&mut paths, desc.kind);

Indeed, if you inject some printf logging, you can see how the PathSet is consumed:

[07:49] Joe:rust | ./x test compiler --dry-run
Building bootstrap
   Compiling bootstrap v0.0.0 (/home/joe/repos/rust/src/bootstrap)
    Finished `dev` profile [unoptimized] target(s) in 1.76s
StepDescription::run: paths=["compiler"]
StepDescription::run: handle all `PathSet`s: desc=StepDescription { default: true, only_hosts: true, should_run: 0x55e3fe0ece60, make_run: 0x55e3fe0ece80, name: "bootstrap::core::build_steps::test::CodegenCranelift", kind: Test }, pathsets=[Set({test::compiler/rustc_codegen_cranelift})]
StepDescription::maybe_run: pathsets=[Set({test::compiler/rustc_codegen_cranelift})]
CodegenCranelift
Building stage0 library artifacts (x86_64-unknown-linux-gnu)
Building compiler artifacts (stage0 -> stage1, x86_64-unknown-linux-gnu)
Creating a sysroot for stage1 compiler (use `rustup toolchain link 'name' build/host/stage1`)
cranelift not in rust.codegen-backends. skipping
StepDescription::run: handle all `PathSet`s: desc=StepDescription { default: false, only_hosts: false, should_run: 0x55e3fe4b8690, make_run: 0x55e3fe4b86b0, name: "bootstrap::core::build_steps::toolstate::ToolStateCheck", kind: Test }, pathsets=[]
StepDescription::run: handle all `PathSet`s: desc=StepDescription { default: true, only_hosts: true, should_run: 0x55e3fe0dad90, make_run: 0x55e3fe0daed0, name: "bootstrap::core::build_steps::test::Tidy", kind: Test }, pathsets=[]
[... some entries trimmed ...]
"bootstrap::core::build_steps::test::CodegenGCC", kind: Test }, pathsets=[]

Why does moving test::CodegenGCC and test::CodegenCranelift seemingly unblock ./x test compiler? Well, it's because test::CrateLibrustc is now the non-suite-path test Step that first gets to eat the PathSet:

StepDescription::run: handle all `PathSet`s: desc=StepDescription { default: true, only_hosts: true, should_run: 0x5610308a9ef0, make_run: 0x5610308a9f40, name: "bootstrap::core::build_steps::test::CrateLibrustc", kind: Test }, pathsets=[Set({test::compiler})]
[... some entries trimmed ...]
StepDescription::run: handle all `PathSet`s: desc=StepDescription { default: true, only_hosts: true, should_run: 0x5610308afe60, make_run: 0x5610308afe80, name: "bootstrap::core::build_steps::test::CodegenCranelift", kind: Test }, pathsets=[]
StepDescription::run: handle all `PathSet`s: desc=StepDescription { default: true, only_hosts: true, should_run: 0x5610308b0d20, make_run: 0x5610308b0d40, name: "bootstrap::core::build_steps::test::CodegenGCC", kind: Test }, pathsets=[]

So now CodegenCranelift no longer tries to run because the PathSet got eaten by an earlier-registered test Step.

Ok. Now you may ask: how does #133492 regress ./x test compiler? Because before #133492, PathSet-based eligibility check only matches suffixes, but now #133492 also make PathSet-based eligibility also matches prefixes, making #133492 a necessary condition to break ./x test compiler but is not the "root cause", in my honest opinion. The sufficient condition is arguably the PathSet-related handling itself.

I'm also only describing why this is a problem, I don't quite have a good solution in mind just yet.

(Also I'm not sure why there's a specific distinction of suite_path, I suspect that's only used for compiletest-managed tests/.)

bootstrap: Make ./x test compiler actually run the compiler unit tests by Zalathar · Pull Request #134919 · rust-lang/rust (original) (raw)

bootstrap: Make `./x test compiler` actually run the compiler unit tests by Zalathar · Pull Request #134919 · rust-lang/rust (original) (raw)