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

[Zalathar]

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]

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]

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:

1. Step::DEFAULT is the "base" default.
2. 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...

[Zalathar]

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

[jieyouxu]

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

[bjorn3]

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?

[jieyouxu]

Hm yeah. I think I like that better.

[jieyouxu]

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]

@bors r+ rollup

[bors]

📌 Commit b9b6b58 has been approved by jieyouxu

It is now in the queue for this repository.

[jieyouxu]

@bors r-

[jieyouxu]

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

[rustbot]

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]

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]

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