bootstrap: Replace Step::DEFAULT and default_condition with is_default_step

[Zalathar]

• 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]

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]

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

[Zalathar]

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.

[Kobzol]

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.

[jieyouxu]

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.

[Zalathar]

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]

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]

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