bootstrap: Store and use an explicit CheckKind in check::Rustc

[Zalathar]

#160417 tried to fix ./x fix compiler by making the check::Rustc step properly use builder.kind instead of hardcoding Kind::Check.

However, that ended up breaking ./x clippy, which was relying on that hardcoded Kind::Check to really invoke cargo check.

This PR therefore adds an explicit CheckKind to individual instances of check::Rustc, allowing command-line invocations to perform a cargo check or cargo fix as appropriate, without breaking other callers that were relying on the hardcoded cargo check.

As a result, ./x fix compiler now works.

r? Kobzol (or bootstrap)

[rustbot]

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

[Kobzol]

Thanks! The last commit looks good.

I'm a bit wondering about 938bf98 though. I spent a lot of time last year ensuring that our usage of the various check/build/etc. steps is correct. It is very easy to pass the wrong thing to steps like check::Rustc. In this case, there is at least a specific type (CompilerForCheck), which makes it a bit harder to pass something completely unrelated. But that compiler still has to be prepared with the right target and mode. I'd like to leave at least the new function, to have a canonical way of constructing that step, if it was used on other places in the future.

That being said, there is only one usage of that function today, and it is close to where ;new` was, so probably it's fine to remove it. I just want to avoid ad-hoc construction of complicated steps (which this one is, because its fields have non-trivial pre-requisites) across bootstrap, and having a dedicated constructor helps with that.

[Zalathar]

In this particular case, I found it weird that the only user of the “real” constructor was the step's own make_run, and that the only other constructor would just bypass everything and forward directly to the fields anyway.

I have pushed a proposed change (diff) that makes PrepareRustcRmetaSysroot call a helper method instead of creating check::Rustc directly, so that all creations are close to Rustc in the source file.

[Kobzol]

In this particular case, I found it weird that the only user of the “real” constructor was the step's own make_run, and that the only other constructor would just bypass everything and forward directly to the fields anyway.

Yeah, the second constructor needs to bypass, because it is essentially "internal" usage of the step. The new function is used for normal usage where you just want to do all the necessary steps to make the step work. And yes, there was only one usage, but I still created a bunch of new constructors to keep a way to create similar steps correctly in the future.

I still think that the new function could be there, for consistency and to have a canonical way of constructing the step, but no strong opinions on it for this step.

Feel free to r=me after CI passes with/without adding the constructor back.

[Zalathar]

@bors r=Kobzol

[rust-bors]

📌 Commit b344260 has been approved by Kobzol

It is now in the queue for this repository.