Pass all paths to Step::run at once when using ShouldRun::krate

[jyn514]

Helps with #95503. The goal is to run cargo test -p rustc_data_structures -p rustc_lint_defs instead of cargo test -p rustc_data_structures; cargo test -p rustc_lint_defs, which should both recompile less and avoid replaying cached warnings.

This was surprisingly complicated. The main changes are:

1. Invert the order of iteration in StepDescription::run.

   Previously, it did something like:

   for path in paths:
   for (step, should_run) in should_runs:
       if let Some(set) = should_run.pathset_for_path(path):
       step.run(builder, set)

   That worked ok for individual paths, but didn't allow passing more than one path at a time to Step::run
   (since pathset_for_paths only had one path available to it).
   Change it to instead look at the intersection of paths and should_run.paths:

   for (step, should_run) in should_runs:
   if let Some(set) = should_run.pathset_for_paths(paths):
       step.run(builder, set)

2. Change pathset_for_path to take multiple pathsets.

   The goal is to avoid x test library/alloc testing all library crates, instead of just alloc.
   The changes here are similarly subtle, to use the intersection between the paths rather than all
   paths in should_run.paths. I added a test for the behavior to try and make it more clear.

   Note that we use pathsets instead of just paths to allow for sets with multiple aliases (cough all_krates cough).
   See the documentation added in the next commit for more detail.

3. Change StepDescription::run to explicitly handle 0 paths.

   Before this was implicitly handled by the for loop, which just didn't excute when there were no paths.
   Now it needs a check, to avoid trying to run all steps (this is a problem for steps that use default_condition).

4. Change RunDescription to have a list of pathsets, rather than a single path.

5. Remove paths as they're matched

   This allows checking at the end that no invalid paths are left over.
   Note that if two steps matched the same path, this will no longer run both;
   but that's a bug anyway.

6. Handle suite paths separately from regular sets.

   Running multiple suite paths at once instead of in separate make_run invocations is both tricky and not particularly useful.
   The respective test Steps already handle this by introspecting the original paths.

   Avoid having to deal with it by moving suite handling into a seperate loop than PathSet::Set checks.

@rustbot label +A-rustbuild

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[jyn514]

Hmm, it would probably be good to add tests for the commands the second two commits fix; I have a feeling they'll regress otherwise since the logic is more complicated than before :(

[bors]

☔ The latest upstream changes (presumably #96804) made this pull request unmergeable. Please resolve the merge conflicts.

[jyn514]

So, this is failing because since #96660, the process exits on "no paths matched" rather than panicking.

@Mark-Simulacrum how much would you hate me if I wrote a #[should_exit] macro using https://docs.rs/procspawn/ ?

[jyn514]

OH I'm silly, I can just change it to panic only when cfg(test) is set.

@bors author

[jyn514]

@rustbot author

[Mark-Simulacrum]

I'd prefer to avoid extra dependencies, but it sounds like you have an alternative solution regardless.

[jyn514]

@rustbot ready

[rustbot]

Error: The "Ready" shortcut only works on pull requests.

Please let @rust-lang/release know if you're having trouble with this bot.

[jyn514]

@rustbot ready

[jyn514]

Ok, this should be ready for review. Like we discussed on our call, PathSet now means "multiple aliases that execute the same task"; Steps that do different things depending on the path, like test::Crate, are passed multiple PathSets instead of multiple individual PathBufs. I added some documentation as well that should hopefully make this easier to understand.

[Mark-Simulacrum]

r=me with nits resolved

[Mark-Simulacrum]

intersection to me doesn't signal "will remove from the needles" -- I wonder if something like remove_matching here or so might be a better fit? Updating the doc comment to note the removal would also be good, regardless.

[jyn514]

Went with intersection_removing_matches, and added another test for how it modifies needles.

[Mark-Simulacrum]

Similarly to before I'd prefer to avoid an innocuous name like this removing elements from paths. Not sure I have a good suggestion naming wise though. (If we can't come up with anything, let's keep this name for lack of better options).

[jyn514]

I went with pathset_for_paths_removing_matches which seems kinda long, but it's only used in one place so I think it's ok to err on the side of being clear.

[Mark-Simulacrum]

Hm, is this going to be a really long array in the future when we're not testing one by one?

Today it ends up as Testing ["panic_abort"] stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu) (for example) which seems OK though not great. Let's leave it for now though.

[jyn514]

It can, yes:

Testing ["rustc-main", "rustc_data_structures", "rustc_metadata", "rustc_middle", "rustc_trait_selection"] stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

I think I remember you saying we're not going to encourage running all unit tests, though; I think it's ok to have this be kind of long if people do it anyway.

[jyn514]

@bors r=Mark-Simulacrum rollup=iffy (affects nearly every step in bootstrap)

[bors]

📌 Commit fca6dbd has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit fca6dbd with merge 21e9336...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 21e9336 to master...

[rust-timer]

Finished benchmarking commit (21e9336): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 😿 relevant regressions found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.2%	2.2%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

• Primary benchmarks: 😿 relevant regression found
• Secondary benchmarks: 🎉 relevant improvement found

	mean1	max	count2
Regressions 😿 (primary)	2.9%	2.9%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.6%	-3.6%	1
All 😿🎉 (primary)	2.9%	2.9%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²