bootstrap: Make `./x test compiler` actually run the compiler unit tests #134919

Zalathar · 2024-12-30T12:23:15Z

rustbot · 2024-12-30T12:23:23Z

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Zalathar · 2024-12-30T12:25:27Z

src/bootstrap/src/core/builder/tests.rs

+fn test_test_compiler() {
+    let cmd = &["test", "compiler"].map(str::to_owned);
+    let config = configure_with_args(cmd, &[TEST_TRIPLE_1], &[TEST_TRIPLE_1]);
+    let cache = run_build(&config.paths.clone(), config);
+
+    let compiler = cache.contains::<test::CrateLibrustc>();
+    let cranelift = cache.contains::<test::CodegenCranelift>();
+    let gcc = cache.contains::<test::CodegenGCC>();
+
+    assert_eq!((compiler, cranelift, gcc), (true, false, false));
+}


I have manually verified that this unit test fails before the change, and succeeds after the change.

I also tried to write a unit test for ./x test rustc_codegen_cranelift and ./x test compiler/rustc_codegen_cranelift, to make sure this PR doesn't break them. But so far I haven't managed to get those tests to work, so for now I've just verified them by hand.

Zalathar · 2024-12-30T12:51:26Z

Explanation of the fix:

When bootstrap tries to figure out which step is responsible for each command-line argument, it only allows one step to claim any argument. So to force test::CrateLibrustc to take priority over the codegen backend steps, we just need to make sure it appears before them in the list of test steps.

This is the code that allows steps to claim arguments:

rust/src/bootstrap/src/core/builder/mod.rs

Lines 489 to 509 in c156614

    
           for (desc, should_run) in v.iter().zip(&should_runs) { 
        
               let pathsets = should_run.pathset_for_paths_removing_matches(&mut paths, desc.kind); 
        
               // This value is used for sorting the step execution order. 
        
               // By default, `usize::MAX` is used as the index for steps to assign them the lowest priority. 
        
               // 
        
               // If we resolve the step's path from the given CLI input, this value will be updated with 
        
               // the step's actual index. 
        
               let mut closest_index = usize::MAX; 
        
               // Find the closest index from the original list of paths given by the CLI input. 
        
               for (index, (path, is_used)) in path_lookup.iter_mut().enumerate() { 
        
                   if !*is_used && !paths.contains(path) { 
        
                       closest_index = index; 
        
                       *is_used = true; 
        
                       break; 
        
                   } 
        
               } 
        
               steps_to_run.push((closest_index, desc, pathsets)); 
        
           }

src/bootstrap/src/core/builder/mod.rs

jieyouxu · 2024-12-31T00:05:02Z

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:

rust/src/bootstrap/src/core/build_steps/test.rs

Lines 3406 to 3408 in 2061630

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

rust/src/bootstrap/src/core/build_steps/test.rs

Lines 3530 to 3532 in 2061630

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

These test Steps are registered here via describe!:

rust/src/bootstrap/src/core/builder/mod.rs

Lines 925 to 926 in 2061630

    
           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:

rust/src/bootstrap/src/core/builder/mod.rs

Lines 465 to 475 in 2061630

    
           // 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.

rust/src/bootstrap/src/core/builder/mod.rs

Lines 489 to 490 in 2061630

    
           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/.)

jieyouxu

In any case, this is a sufficient fix for the basic workflow of testing the compiler unit crates. Thanks.

jieyouxu · 2024-12-31T00:11:04Z

@bors r+ rollup

bors · 2024-12-31T00:11:07Z

📌 Commit 796835f has been approved by jieyouxu

It is now in the queue for this repository.

bootstrap: Make `./x test compiler` actually run the compiler unit tests Fixes rust-lang#134916.

Rollup of 7 pull requests Successful merges: - rust-lang#133461 (Add COPYRIGHT-*.html files to distribution and update `COPYRIGHT`) - rust-lang#134919 (bootstrap: Make `./x test compiler` actually run the compiler unit tests) - rust-lang#134927 (Make slice::as_flattened_mut unstably const) - rust-lang#134930 (ptr docs: make it clear that we are talking only about memory accesses) - rust-lang#134932 (explicitly set float ABI for all ARM targets) - rust-lang#134934 (Fix typos) - rust-lang#134941 (compiler: Add a statement-of-intent to `rustc_abi`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 8 pull requests Successful merges: - rust-lang#134919 (bootstrap: Make `./x test compiler` actually run the compiler unit tests) - rust-lang#134927 (Make slice::as_flattened_mut unstably const) - rust-lang#134930 (ptr docs: make it clear that we are talking only about memory accesses) - rust-lang#134932 (explicitly set float ABI for all ARM targets) - rust-lang#134933 (Make sure we check the future type is `Sized` in `AsyncFn*`) - rust-lang#134934 (Fix typos) - rust-lang#134941 (compiler: Add a statement-of-intent to `rustc_abi`) - rust-lang#134949 (Convert some `Into` impls into `From` impls) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#134919 - Zalathar:x-test-compiler, r=jieyouxu bootstrap: Make `./x test compiler` actually run the compiler unit tests Fixes rust-lang#134916.

Signed-off-by: onur-ozkan <[email protected]>

rustbot assigned jieyouxu Dec 30, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 30, 2024

Zalathar commented Dec 30, 2024

View reviewed changes

onur-ozkan reviewed Dec 30, 2024

View reviewed changes

src/bootstrap/src/core/builder/mod.rs Outdated Show resolved Hide resolved

Make ./x test compiler actually run the compiler unit tests

796835f

Zalathar force-pushed the x-test-compiler branch from be32f13 to 796835f Compare December 31, 2024 00:02

jieyouxu approved these changes Dec 31, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 31, 2024

jieyouxu mentioned this pull request Dec 31, 2024

Running ./x test compiler only runs unit tests for rustc_codegen_cranelift #134916

Closed

Zalathar added a commit to Zalathar/rust that referenced this pull request Dec 31, 2024

Rollup merge of rust-lang#134919 - Zalathar:x-test-compiler, r=jieyouxu

2bb90b2

bootstrap: Make `./x test compiler` actually run the compiler unit tests Fixes rust-lang#134916.

Zalathar mentioned this pull request Dec 31, 2024

Rollup of 7 pull requests #134944

Closed

Zalathar mentioned this pull request Dec 31, 2024

Rollup of 8 pull requests #134952

Merged

bors merged commit a777f4a into rust-lang:master Dec 31, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 31, 2024

Zalathar deleted the x-test-compiler branch December 31, 2024 09:08

onur-ozkan added a commit to onur-ozkan/rust that referenced this pull request Jan 2, 2025

revert step order from rust-lang#134919

2547834

Signed-off-by: onur-ozkan <[email protected]>

onur-ozkan added a commit to onur-ozkan/rust that referenced this pull request Jan 2, 2025

revert step order from rust-lang#134919

4e17685

Signed-off-by: onur-ozkan <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootstrap: Make `./x test compiler` actually run the compiler unit tests #134919

bootstrap: Make `./x test compiler` actually run the compiler unit tests #134919

Zalathar commented Dec 30, 2024

rustbot commented Dec 30, 2024

Zalathar Dec 30, 2024

Zalathar Dec 31, 2024

Zalathar commented Dec 30, 2024

jieyouxu commented Dec 31, 2024 •

edited

Loading

jieyouxu left a comment

jieyouxu commented Dec 31, 2024

bors commented Dec 31, 2024

bootstrap: Make ./x test compiler actually run the compiler unit tests #134919

bootstrap: Make ./x test compiler actually run the compiler unit tests #134919

Conversation

Zalathar commented Dec 30, 2024

rustbot commented Dec 30, 2024

Zalathar Dec 30, 2024

Choose a reason for hiding this comment

Zalathar Dec 31, 2024

Choose a reason for hiding this comment

Zalathar commented Dec 30, 2024

jieyouxu commented Dec 31, 2024 • edited Loading

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu commented Dec 31, 2024

bors commented Dec 31, 2024

bootstrap: Make `./x test compiler` actually run the compiler unit tests #134919

bootstrap: Make `./x test compiler` actually run the compiler unit tests #134919

jieyouxu commented Dec 31, 2024 •

edited

Loading