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

Zalathar · 2025-11-16T03:19:23Z

Revised and expanded version of bootstrap: Split out Step::is_really_default from Step::should_run #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-16T03:22:05Z

Ignoring the new snapshot tests, the actual git diff --shortstat is more like +511, -307.

Kobzol

Great cleanup! To go further, I think that we could eventually rethink/rename all the important Step functions (not in this PR).

Something like:

should_run => `define_cli_paths`
is_default_step => `should_run_by_default`, `should_run_with_no_path` or just `is_default`
make_run => `run_from_cli`

I would suggest making fn is_default_step { true } the default implementation. IMO everything should be enabled by default, so that we don't miss steps by accident, and only specific steps should return false. This reduces then need to think about the right default impl for the is_default_step function. Most/more of the impls in this PR return true anyway.

View changes since this review

Kobzol · 2025-11-16T20:32:33Z

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

 fn check_if_tidy_is_installed(builder: &Builder<'_>) -> bool {
-    command("tidy").allow_failure().arg("--version").run_capture_stdout(builder).is_success()
+    *builder.html_tidy_is_available_memo.get_or_init(|| {
+        command("tidy").allow_failure().arg("--version").run_capture_stdout(builder).is_success()


Rather than caching this in the builder, you can just make the command cached (.cached()). That's IMO much simpler.

Caching the command output is a bit less powerful, since it doesn't cache the subsequent processing steps.

But if running the command repeatedly is the thing we're worried about, then that's fine.

Ideally I would want to not perform these probes here at all, and instead require opt-in config for these tools. But that's well beyond the scope of this PR.

Tbh I think that even without any caching it would be fine. But if anything, I'd expect the command invocation to take more time than the processing.

Ideally I would want to not perform these probes here at all, and instead require opt-in config for these tools. But that's well beyond the scope of this PR.

This is also my preference (if I were to redesign this setup from scratch), but I can appreciate it being annoying for existing usages.

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

Zalathar · 2025-11-17T02:01:39Z

Great cleanup! To go further, I think that we could eventually rethink/rename all the important Step functions (not in this PR).

Something like:
should_run => `define_cli_paths`
is_default_step => `should_run_by_default`, `should_run_with_no_path` or just `is_default`
make_run => `run_from_cli`
I would suggest making fn is_default_step { true } the default implementation. IMO everything should be enabled by default, so that we don't miss steps by accident, and only specific steps should return false. This reduces then need to think about the right default impl for the is_default_step function. Most/more of the impls in this PR return true anyway.

I've written some of my thoughts about this on Zulip, since it's a bit of a pain to have an actual discussion on GitHub.

jieyouxu · 2025-11-17T07:42:01Z

Holding off on reviewing this to wait for the discussion to settle down first.
@rustbot label: -S-waiting-on-review +S-waiting-on-t-bootstrap

bors · 2025-11-17T15:37:10Z

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

jieyouxu

Looks good to me, handing review back to Jakub in case they had more feedback

View changes since this review

jieyouxu · 2025-11-26T10:23:19Z

r? @Kobzol

rustbot · 2025-11-26T10:23:29Z

Kobzol is not on the review rotation at the moment.
They may take a while to respond.

jieyouxu · 2025-11-26T10:28:26Z

Right. r? me

In that case, I think let's merge this as-is. If we wanted to make run-by-default the default (hah), we can still do that in a follow-up.

@bors r+ rollup=never

bors · 2025-11-26T10:28:30Z

📌 Commit 231c789 has been approved by jieyouxu

It is now in the queue for this repository.

rustbot · 2025-11-26T12:11:29Z

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-26T12:12:36Z

Rebased to fix a trivial soft-conflict with the new step added by #149071.

jieyouxu · 2025-11-26T12:39:30Z

r=me once PR CI is green enough

Kobzol · 2025-11-26T15:27:43Z

Fine by me to this as-is 👍

Zalathar · 2025-11-26T22:43:47Z

@bors r=jieyouxu

bors · 2025-11-26T22:43:49Z

📌 Commit df8a851 has been approved by jieyouxu

It is now in the queue for this repository.

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

bors · 2025-11-27T01:06:31Z

⌛ Testing commit df8a851 with merge c4d75ac...

bors · 2025-11-27T04:15:27Z

💔 Test failed - checks-actions

jieyouxu · 2025-11-27T04:34:29Z

The hosted runner lost communication with the server.

Tragic. @bors retry

bors · 2025-11-27T10:48:15Z

⌛ Testing commit df8a851 with merge cf8a955...

bors · 2025-11-27T14:00:49Z

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing cf8a955 to main...

github-actions · 2025-11-27T14:03:34Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 7b9905e (parent) -> cf8a955 (this PR)

Test differences

Show 13 test diffs

Stage 0

core::builder::cli_paths::tests::x_bench: [missing] -> pass (J0)
core::builder::cli_paths::tests::x_clean: [missing] -> pass (J0)
core::builder::cli_paths::tests::x_clippy: [missing] -> pass (J0)
core::builder::cli_paths::tests::x_doc: [missing] -> pass (J0)
core::builder::cli_paths::tests::x_fix: [missing] -> pass (J0)
core::builder::cli_paths::tests::x_fmt: [missing] -> pass (J0)
core::builder::cli_paths::tests::x_install: [missing] -> pass (J0)
core::builder::cli_paths::tests::x_miri: [missing] -> pass (J0)
core::builder::cli_paths::tests::x_run: [missing] -> pass (J0)
core::builder::cli_paths::tests::x_setup: [missing] -> pass (J0)
core::builder::cli_paths::tests::x_vendor: [missing] -> pass (J0)

Additionally, 2 doctest diffs were found. These are ignored, as they are noisy.

Job group index

J0: aarch64-apple, aarch64-gnu, aarch64-gnu-llvm-20-2, aarch64-msvc-2, i686-gnu-2, i686-gnu-nopt-2, i686-msvc-2, x86_64-gnu, x86_64-gnu-llvm-20-1, x86_64-gnu-llvm-20-3, x86_64-gnu-llvm-21-1, x86_64-gnu-llvm-21-3, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-2, x86_64-msvc-2

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard cf8a95590a1b768b7711f2631d5b5e3ead464de7 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

dist-x86_64-apple: 9564.5s -> 6734.7s (-29.6%)
dist-apple-various: 3384.3s -> 3892.2s (+15.0%)
dist-aarch64-apple: 7384.5s -> 6425.7s (-13.0%)
x86_64-gnu-miri: 4555.3s -> 4091.8s (-10.2%)
aarch64-gnu-llvm-20-1: 3366.3s -> 3687.7s (+9.5%)
dist-i586-gnu-i586-i686-musl: 5366.8s -> 4896.1s (-8.8%)
aarch64-gnu-llvm-20-2: 2181.2s -> 2362.0s (+8.3%)
dist-i686-msvc: 7766.6s -> 8359.5s (+7.6%)
dist-sparcv9-solaris: 5305.7s -> 4910.7s (-7.4%)
x86_64-msvc-1: 9078.6s -> 9740.5s (+7.3%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-11-27T15:20:07Z

Finished benchmarking commit (cf8a955): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (secondary 4.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.2%	[4.2%, 4.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 0.3%, secondary 0.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	4.4%	[4.4%, 4.4%]	1
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	0.3%	[-1.6%, 2.3%]	2

Binary size

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

Bootstrap: 470.06s -> 470.335s (0.06%)
Artifact size: 386.96 MiB -> 386.93 MiB (-0.01%)

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

rustbot assigned jieyouxu Nov 16, 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 16, 2025

Zalathar mentioned this pull request Nov 16, 2025

bootstrap: Split out Step::is_really_default from Step::should_run #148965

Closed

Kobzol reviewed Nov 16, 2025

View reviewed changes

Zalathar force-pushed the is-default branch from 58b9a05 to 15e7855 Compare November 16, 2025 23:59

rustbot added S-waiting-on-t-bootstrap Status: Awaiting decision from T-bootstrap. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2025

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

Zalathar force-pushed the is-default branch from 15e7855 to 231c789 Compare November 18, 2025 00:32

This comment has been minimized.

Sign in to view

jieyouxu approved these changes Nov 26, 2025

View reviewed changes

rustbot assigned Kobzol and unassigned jieyouxu Nov 26, 2025

rustbot assigned jieyouxu Nov 26, 2025

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 26, 2025

rustbot unassigned Kobzol Nov 26, 2025

This comment has been minimized.

Sign in to view

Zalathar added 2 commits November 26, 2025 23:04

Snapshot test the default steps for most bootstrap subcommands

7dff6ce

Replace Step::DEFAULT and default_condition with is_default_step

df8a851

Zalathar force-pushed the is-default branch from 231c789 to df8a851 Compare November 26, 2025 12:11

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 26, 2025

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

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 27, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 27, 2025

bors merged commit cf8a955 into rust-lang:main Nov 27, 2025
12 checks passed

rustbot added this to the 1.93.0 milestone Nov 27, 2025

bors mentioned this pull request Nov 27, 2025

Write .cargo/config.toml in x vendor #144124

Open

Zalathar deleted the is-default branch November 27, 2025 22:45

Uh oh!

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

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

Uh oh!

Conversation

Zalathar commented Nov 16, 2025

Uh oh!

Zalathar commented Nov 16, 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 Nov 16, 2025

Choose a reason for hiding this comment

Uh oh!

Zalathar Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Kobzol Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 17, 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!

Uh oh!

Zalathar commented Nov 17, 2025

Uh oh!

jieyouxu commented Nov 17, 2025

Uh oh!

bors commented Nov 17, 2025

Uh oh!

This comment has been minimized.

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 commented Nov 26, 2025

Uh oh!

rustbot commented Nov 26, 2025

Uh oh!

jieyouxu commented Nov 26, 2025

Uh oh!

bors commented Nov 26, 2025

Uh oh!

This comment has been minimized.

rustbot commented Nov 26, 2025

Uh oh!

Zalathar commented Nov 26, 2025

Uh oh!

jieyouxu commented Nov 26, 2025

Uh oh!

Kobzol commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zalathar commented Nov 26, 2025

Uh oh!

bors commented Nov 26, 2025

Uh oh!

bors commented Nov 27, 2025

Uh oh!

bors commented Nov 27, 2025

Uh oh!

jieyouxu commented Nov 27, 2025

Uh oh!

bors commented Nov 27, 2025

Uh oh!

bors commented Nov 27, 2025

Uh oh!

Uh oh!

github-actions bot commented Nov 27, 2025

Test differences

Stage 0

Job duration changes

Uh oh!

rust-timer commented Nov 27, 2025

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

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

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

Kobzol left a comment •

edited by rustbot

Loading

jieyouxu Nov 17, 2025 •

edited

Loading

jieyouxu left a comment •

edited by rustbot

Loading

Kobzol commented Nov 26, 2025 •

edited

Loading