bootstrap: Split out `Step::is_really_default` from `Step::should_run` by Zalathar · Pull Request #148965 · rust-lang/rust

Zalathar · 2025-11-15T07:52:36Z

One of the confusing things about bootstrap's Step::should_run is that it combines two loosely-related but non-overlapping responsibilities:

Registering paths/aliases to decide whether a step should be run in response to paths/aliases passed as explicit command-line arguments
- When the user invokes ./x test compiler, this allows bootstrap to know what steps “compiler” should translate into
Deciding whether a step marked DEFAULT = true should actually run or not, when no paths/aliases are explicitly specified
- When the user invokes ./x test, this allows bootstrap to know which steps to run by default

This PR splits out the latter of those responsibilities into a separate is_really_default associated function.

A small number of steps were using ShouldRun::lazy_default_condition to specify a condition that should not be run repeatedly if possible, e.g. because it queries external tools. Those steps now perform memoization via fields in Builder instead.

rustbot · 2025-11-15T07:52:41Z

r? @clubby789

rustbot has assigned @clubby789.
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

jieyouxu

I'm a big fan of the changes, I think this is much less confusing than before. However, I would like to discuss the name of is_really_default with the team, because I still find it confusing to read...

View changes since this review

jieyouxu · 2025-11-15T09:05:59Z

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

+        // named `builder` for IDE autocompletion.
+        let _ = builder;
+        Self::DEFAULT
+    }


Discussion: maybe a name like finalized_default? I kinda hate that name too, it reminds me of the convoluted compiler lint level decoration logic... Anyway, 'finalized' in the sense that there's a hierarchy or precedence ordering:

Step::DEFAULT is the "base" default.

is_really_default or finalized_default can "override" this base default subject to conditions.

(2) then finally determines if a Step is eligible to run.

I say this because is_really_default I find quite confusing to read...

I don't think is_really_default is a great name, but on the other hand I do appreciate that it conveys a suitable amount of “you should probably read the actual docs for this oddly-named function instead of assuming that it does something intuitive”.

... Yeah. Let's land this now as is_really_default, if we can come up with a better name later, we can always do that in a follow-up.

Would it be possible to merge const DEFAULT and fn is_really_default into a single fn default that returns a constant value for most steps?

Hm yeah. I think I like that better.

jieyouxu · 2025-11-15T09:07:01Z

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

+    /// Called to allow steps to register the command-line paths that should
+    /// cause them to run.


Discussion:

command-line paths

I guess this is a path / path filter. Discussing in #t-infra/bootstrap > Path filters in bootstrap @ 💬 on what we actually call these

./x test xxx ^-- what is this called?

jieyouxu · 2025-11-15T10:18:53Z

@bors r+ rollup

bors · 2025-11-15T10:18:56Z

📌 Commit b9b6b58 has been approved by jieyouxu

It is now in the queue for this repository.

jieyouxu · 2025-11-15T13:58:22Z

@bors r-

jieyouxu · 2025-11-15T14:00:11Z

#148965 (comment), but also doesn't necessarily need to be in this PR, at your discretion.

rustbot · 2025-11-16T01:32:17Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Zalathar · 2025-11-16T03:21:01Z

The additional changes for is_default_step were substantial enough that I've opened a separate PR to potentially replace this one:

bootstrap: Replace Step::DEFAULT and default_condition with is_default_step #148987

bors · 2025-11-17T15:36:59Z

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

bootstrap: Replace `Step::DEFAULT` and `default_condition` with `is_default_step` - Revised and expanded version of #148965 --- One of the confusing things about bootstrap's `Step::should_run` is that it combines two loosely-related but non-overlapping responsibilities: - Registering paths/aliases to decide whether a step should be run in response to paths/aliases passed as explicit command-line arguments - When the user invokes `./x test compiler`, this allows bootstrap to know what steps “compiler” should translate into - Deciding whether a step marked `DEFAULT = true` should actually run or not, when no paths/aliases are explicitly specified - When the user invokes `./x test`, this allows bootstrap to know which steps to run by default This PR therefore splits out the latter of those responsibilities into a dedicated `is_default_step` associated function, which also replaces the existing `DEFAULT` associated constant. A small number of steps were using `ShouldRun::lazy_default_condition` to specify a condition that should not be run repeatedly if possible, e.g. because it queries external tools. Those steps now perform memoization via fields in `Builder` instead. r? jieyouxu

Zalathar · 2025-11-29T01:27:46Z

Obsoleted by #148987.

bootstrap: Replace `Step::DEFAULT` and `default_condition` with `is_default_step` - Revised and expanded version of rust-lang/rust#148965 --- One of the confusing things about bootstrap's `Step::should_run` is that it combines two loosely-related but non-overlapping responsibilities: - Registering paths/aliases to decide whether a step should be run in response to paths/aliases passed as explicit command-line arguments - When the user invokes `./x test compiler`, this allows bootstrap to know what steps “compiler” should translate into - Deciding whether a step marked `DEFAULT = true` should actually run or not, when no paths/aliases are explicitly specified - When the user invokes `./x test`, this allows bootstrap to know which steps to run by default This PR therefore splits out the latter of those responsibilities into a dedicated `is_default_step` associated function, which also replaces the existing `DEFAULT` associated constant. A small number of steps were using `ShouldRun::lazy_default_condition` to specify a condition that should not be run repeatedly if possible, e.g. because it queries external tools. Those steps now perform memoization via fields in `Builder` instead. r? jieyouxu

bootstrap: Replace `Step::DEFAULT` and `default_condition` with `is_default_step` - Revised and expanded version of rust-lang#148965 --- One of the confusing things about bootstrap's `Step::should_run` is that it combines two loosely-related but non-overlapping responsibilities: - Registering paths/aliases to decide whether a step should be run in response to paths/aliases passed as explicit command-line arguments - When the user invokes `./x test compiler`, this allows bootstrap to know what steps “compiler” should translate into - Deciding whether a step marked `DEFAULT = true` should actually run or not, when no paths/aliases are explicitly specified - When the user invokes `./x test`, this allows bootstrap to know which steps to run by default This PR therefore splits out the latter of those responsibilities into a dedicated `is_default_step` associated function, which also replaces the existing `DEFAULT` associated constant. A small number of steps were using `ShouldRun::lazy_default_condition` to specify a condition that should not be run repeatedly if possible, e.g. because it queries external tools. Those steps now perform memoization via fields in `Builder` instead. r? jieyouxu

bootstrap: Replace `Step::DEFAULT` and `default_condition` with `is_default_step` - Revised and expanded version of rust-lang/rust#148965 --- One of the confusing things about bootstrap's `Step::should_run` is that it combines two loosely-related but non-overlapping responsibilities: - Registering paths/aliases to decide whether a step should be run in response to paths/aliases passed as explicit command-line arguments - When the user invokes `./x test compiler`, this allows bootstrap to know what steps “compiler” should translate into - Deciding whether a step marked `DEFAULT = true` should actually run or not, when no paths/aliases are explicitly specified - When the user invokes `./x test`, this allows bootstrap to know which steps to run by default This PR therefore splits out the latter of those responsibilities into a dedicated `is_default_step` associated function, which also replaces the existing `DEFAULT` associated constant. A small number of steps were using `ShouldRun::lazy_default_condition` to specify a condition that should not be run repeatedly if possible, e.g. because it queries external tools. Those steps now perform memoization via fields in `Builder` instead. r? jieyouxu

rustbot assigned clubby789 Nov 15, 2025

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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 Nov 15, 2025

jieyouxu self-assigned this Nov 15, 2025

jieyouxu approved these changes Nov 15, 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 Nov 15, 2025

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 15, 2025

Split out Step::is_really_default from Step::should_run

7b863bb

Zalathar force-pushed the really-default branch from b9b6b58 to 7b863bb Compare November 16, 2025 01:32

Zalathar mentioned this pull request Nov 16, 2025

bootstrap: Replace Step::DEFAULT and default_condition with is_default_step #148987

Merged

Zalathar marked this pull request as draft November 16, 2025 03:22

Zalathar closed this Nov 29, 2025

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 29, 2025

Zalathar deleted the really-default branch November 29, 2025 01:27

		/// Called to allow steps to register the command-line paths that should
		/// cause them to run.

Uh oh!

Conversation

Zalathar commented Nov 15, 2025

Uh oh!

rustbot commented Nov 15, 2025

Uh oh!

jieyouxu 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!

jieyouxu Nov 15, 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 Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

bjorn3 Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Nov 15, 2025

Uh oh!

bors commented Nov 15, 2025

Uh oh!

jieyouxu commented Nov 15, 2025

Uh oh!

jieyouxu commented Nov 15, 2025

Uh oh!

rustbot commented Nov 16, 2025

Uh oh!

Zalathar commented Nov 16, 2025

Uh oh!

bors commented Nov 17, 2025

Uh oh!

Zalathar commented Nov 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jieyouxu left a comment •

edited by rustbot

Loading

jieyouxu Nov 15, 2025 •

edited

Loading