bootstrap: Extract parts of `bootstrap::core::builder` into a `cli_paths` module #148317

Zalathar · 2025-10-31T04:53:26Z

One of the things that makes bootstrap's CLI path handling hard to work with is the fact that it's in the middle of a ~2000 line file full of all sorts of other things. And the primary code sequence is in an unhelpfully-named StepDescription::run function.

This PR therefore pulls some key chunks of code out into a cli_paths submodule.

This should be a purely non-functional change.

rustbot · 2025-10-31T04:53:29Z

r? @Kobzol

rustbot has assigned @Kobzol.
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 · 2025-10-31T05:00:16Z

I also want to clean up path_lookup: Vec<(CLIStepPath, bool)> to use a more sensible data structure, but that will be a non-trivial change so I left it out of this particular PR.

What I would like to work towards is being able to extract the parts of match_paths_to_steps_and_run that perform path-to-step matching, without immediately running the selected steps, so that that part can be unit tested.

Zalathar · 2025-10-31T05:13:07Z

(The error check arguably doesn't belong in match_paths_to_steps_and_run, but I didn't want to have to figure out where it can safely be moved to, so I left it as-is for now.)

Kobzol · 2025-10-31T09:51:56Z

What I would like to work towards is being able to extract the parts of match_paths_to_steps_and_run that perform path-to-step matching, without immediately running the selected steps, so that that part can be unit tested.

Do you think we could already test the current path matching with the snapshot tests? We have code that creates a Config based on CLI arguments, so we could drive the logic a few steps further, take the Builder/Build and assert that it has certain properties.
Or, to make the test more end-to-end, just try a bunch of different input paths and assert which steps get executed.

Unit tests are nice and all, but in the grand scheme of bootstrap, things can still break, so I'd like to also have tests that check the path handling logic end-to-end.

Kobzol

Thank you for cleaning this up! This code really deserves a proper refactoring. Left some comments, but that's not really related to this PR, so if you're happy with this, you can r=me.

View changes since this review

Kobzol · 2025-10-31T09:53:55Z

src/bootstrap/src/core/builder/cli_paths.rs

+
+    // FIXME(Zalathar): This particular check isn't related to path-to-step
+    // matching, and should probably be hoisted to somewhere much earlier.
+    if builder.download_rustc() && (builder.kind == Kind::Dist || builder.kind == Kind::Install) {


Yes, in particular, I think that this should be moved to the part of config parsing that checks if we can run build with stage 0 etc.

Kobzol · 2025-10-31T09:55:06Z

src/bootstrap/src/core/builder/cli_paths.rs

+    // Handle all PathSets.
+    for (_index, desc, pathsets) in steps_to_run {
+        if !pathsets.is_empty() {
+            desc.maybe_run(builder, pathsets);


(for a later PR): this innocent line actually runs bootstrap's CLI steps, which is not obvious. It would be nice if we could just return a set of paths to run from this function/module, and then actually run them in the builder code.

Yep, that's more or less the plan, and I hope to make the resulting helper function snapshot-testable.

I was playing around with changes to StepDescription::run, and found that I kept wanting to pull everything out into a separate file first, which became this PR.

In general I'm a big fan of “the code that does the important thing should be obvious and easy to find”, and the current code certainly doesn't live up to that principle. 😅

Zalathar · 2025-10-31T10:02:43Z

@bors r=Kobzol rollup

bors · 2025-10-31T10:02:53Z

📌 Commit 250ee47 has been approved by Kobzol

It is now in the queue for this repository.

Zalathar · 2025-10-31T10:08:03Z

What I would like to work towards is being able to extract the parts of match_paths_to_steps_and_run that perform path-to-step matching, without immediately running the selected steps, so that that part can be unit tested.

Do you think we could already test the current path matching with the snapshot tests? We have code that creates a Config based on CLI arguments, so we could drive the logic a few steps further, take the Builder/Build and assert that it has certain properties. Or, to make the test more end-to-end, just try a bunch of different input paths and assert which steps get executed.

Unit tests are nice and all, but in the grand scheme of bootstrap, things can still break, so I'd like to also have tests that check the path handling logic end-to-end.

For the purposes of testing path-to-step handling, I would like to have some kind of snapshot-test suite that takes a command-line as input, and produces a list of non-transitive steps only. So basically just the immediate result of translating command-line paths into top-level steps.

The existing snapshot-test harness is not set up to do that (since from what I can tell it includes transitive steps, and omits all but some specially-marked steps), but if you have an idea of how to pull that off before making any functional changes to match_paths_to_steps_and_run, that would be very helpful.

Kobzol · 2025-10-31T10:29:09Z

So the current snapshot tests work by having a test-only code in Builder::ensure that stores all requested steps. We could do the same in Builder::maybe_run, where we would store all the top-level make_run invocations to test on which paths they were invoked.

Or, as a perhaps simpler alternative, we could use the current step recording machinery, but filter only steps that were executed when the "step stack" was empty, which should be the top-level, non-transitive steps executed from make_run.

Rollup of 3 pull requests Successful merges: - #148165 (Use `mut` less in dataflow analysis) - #148287 (Fix deferred cast checks using the wrong body for determining constness) - #148317 (bootstrap: Extract parts of `bootstrap::core::builder` into a `cli_paths` module) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #148317 - Zalathar:cli-paths-mod, r=Kobzol bootstrap: Extract parts of `bootstrap::core::builder` into a `cli_paths` module One of the things that makes bootstrap's CLI path handling hard to work with is the fact that it's in the middle of a ~2000 line file full of all sorts of other things. And the primary code sequence is in an unhelpfully-named `StepDescription::run` function. This PR therefore pulls some key chunks of code out into a `cli_paths` submodule. This should be a purely non-functional change.

rustbot assigned Kobzol Oct 31, 2025

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 Oct 31, 2025

Extract parts of bootstrap::core::builder into a cli_paths module

49205a1

Zalathar force-pushed the cli-paths-mod branch from 8bf985b to 28e6c7a Compare October 31, 2025 05:15

Zalathar added 2 commits October 31, 2025 17:27

Replace repeated zips with a dedicated StepExtra struct

70a9883

Replace bare tuple with a StepToRun struct

250ee47

Zalathar force-pushed the cli-paths-mod branch from 28e6c7a to 250ee47 Compare October 31, 2025 06:27

Kobzol reviewed Oct 31, 2025

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 Oct 31, 2025

Zalathar mentioned this pull request Oct 31, 2025

Rollup of 3 pull requests #148324

Merged

bors merged commit 472c7cd into rust-lang:master Oct 31, 2025
11 checks passed

rustbot added this to the 1.93.0 milestone Oct 31, 2025

Zalathar deleted the cli-paths-mod branch October 31, 2025 22:37

bootstrap: Extract parts of bootstrap::core::builder into a cli_paths module #148317

bootstrap: Extract parts of bootstrap::core::builder into a cli_paths module #148317

Uh oh!

Conversation

Zalathar commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 31, 2025

Uh oh!

Zalathar commented Oct 31, 2025

Uh oh!

Zalathar commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kobzol commented Oct 31, 2025

Uh oh!

Kobzol left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kobzol Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Kobzol Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Zalathar Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zalathar commented Oct 31, 2025

Uh oh!

bors commented Oct 31, 2025

Uh oh!

Zalathar commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kobzol commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bootstrap: Extract parts of `bootstrap::core::builder` into a `cli_paths` module #148317

bootstrap: Extract parts of `bootstrap::core::builder` into a `cli_paths` module #148317

Zalathar commented Oct 31, 2025 •

edited

Loading

Zalathar commented Oct 31, 2025 •

edited

Loading

Kobzol left a comment •

edited by rustbot

Loading

Zalathar Oct 31, 2025 •

edited

Loading

Zalathar commented Oct 31, 2025 •

edited

Loading