rustbuild: cargo keeps being rebuilt

[matthiaskrgr]

When running ./x.py build two times in a row, the cargo(bin) target as well as rls will be built again for the second x.py build even if no sources were touched.

[ehuss]

Hm, this is an unexpected consequence of #73297. Clippy gets built with different RUSTFLAGS, which busts the cache. I'm going to contemplate finding some way to independently cache artifacts with different RUSTFLAGS, it has come up multiple times recently.

In the meantime, we may want to consider changing Clippy back to "Submodule" type so that it doesn't use different RUSTFLAGS.

[Mark-Simulacrum]

I would've expected x.py build without arguments to be relatively rare as something folks are running. However, if this is an active blocker/pain point (and not just "mild annoyance"), I'm okay with adding an exception for clippy (I'd rather not change its type as that may have effects elsewhere and I wouldn't want to have to think about it every time until this is fixed).

Separate caches seems reasonable, though would only contribute to perceived "bloat" of target directories I imagine.

[ehuss]

It increases CI time because some dependencies are getting built multiple times. I compared the build just before #73297, and it looks like it may have added 18 minutes to the build time. 😦

I think it is relatively safe to change clippy to Submodule mode. The only difference is "deny warnings". The risk is that a rust-lang/rust PR introduces a new warning in the clippy code, which will have to be resolved whenever clippy is synced back to its repo. Of course it would be better to deny warnings, just tossing it out as an option.

(Just to be clear, I'm not certain that's the only issue here, it will take some more investigation and testing to verify.)

[Mark-Simulacrum]

Ah, so perhaps I misinterpreted -- I believe most of our CI platforms only run x.py once? (At least the longer ones, where a bump in CI times really matters).

Yeah, I agree that there's not much difference to changing to submodule mode vs. explicitly gating on clippy in the warnings check, but I would prefer the later I think. I would loosely be prepared to r+ such a PR.

[ehuss]

The cache thrashing is due to the order the tools are built. It goes:

1. cargo
2. rustfmt
3. clippy-driver (busts cache, lots of rebuilds)
4. rls (lots of rebuilds due to 3)
5. cargo-clippy (busts cache, lots of rebuilds)
6. miri (some rebuilds due to 5)

I dunno how bootstrap decides which order things get built. Is it random? I'm surprised the two clippy binaries are not done consecutively.

[mati865]

@ehuss they are built in this order:

rust/src/bootstrap/builder.rs

Line 347 in 0cd7ff7

fn get_step_descriptions(kind: Kind) -> Vec<StepDescription> {

In the past when Clippy was not always buildable I had added

rust/src/bootstrap/tool.rs

Line 653 in 0cd7ff7

builder.ensure(Clippy {

since RLS's clippy feature works only when Clippy can build.
That's why Clippy driver is built before RLS.

Reordering tools in builder.rs would remove 1 rebuild, right?