Always check cg_llvm with ./x.py check

[bjorn3]

Previously it would be skipped if codegen-backends doesn't contain llvm.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

Can you say more about the motivation here? It feels odd to me to override an explicit preference from the user here.

It's worth noting that this sort of kind-sensitivity typically causes us trouble -- e.g., switching between x.py check and x.py build ideally doesn't cause (essentially) spurious rebuilds, but I suspect this change as-is may well cause that as the feature set changes for the rustc crate. Can you confirm whether this happens for you with this change (and llvm disabled in config.toml, of course)?

[bjorn3]

Can you say more about the motivation here? It feels odd to me to override an explicit preference from the user here.

./x.py check already checks cg_clif and I think cg_gcc even if you don't explicitly enable them. I personally disable cg_llvm to avoid having to build llvm. A check run doesn't need to build llvm anyway, so it only cost a couple of seconds while ensuring I don't break cg_llvm.

It's worth noting that this sort of kind-sensitivity typically causes us trouble -- e.g., switching between x.py check and x.py build ideally doesn't cause (essentially) spurious rebuilds, but I suspect this change as-is may well cause that as the feature set changes for the rustc crate.

./x.py check runs with the dev profile and ./x.py build with the release profile, so they shouldn't clobber each others caches.

[Mark-Simulacrum]

Is this patch enough for you locally? For me, if I enable this patch and set codegen-backends = ["cranelift"], then I get a compilation failure during x.py check as part of the rustc_llvm build. I believe this is because of the auto-enabling if LLVM has already been built before here, even in check mode, which presumably falls down due to observing two different things or getting skipped?

In any case, I don't think this should land until this bug is squashed out - it's not the most common configuration locally, but it seems like at least somewhat plausible one, so I'd rather avoid merging this.

If you can't manage to reproduce, let me know, I can try to minimize -- I suspect it's just a matter of building before this PR without any codegen-backends config (and download-ci-llvm = true / a local build of LLVM), and then checking out this PR with LLVM omitted.

Having written that out, this may also be a pre-existing bug, but it seems to me like that code I linked still seems plausibly buggy when used in conjunction with this PR -- I would expect us to need to change it to avoid problems.

[bjorn3]

I can't reproduce this issue.

[Mark-Simulacrum]

Repeated attempts didn't produce failures for me either. Sorry for the run around.

@bors r+

[bors]

📌 Commit 512cc35 has been approved by Mark-Simulacrum