fix rustc_llvm spurious rebuilds #100152
fix rustc_llvm spurious rebuilds #100152
Conversation
rustbot
added
T-bootstrap
|
r? @jyn514 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| // If we're just running `check`, there's no need for LLVM to be built. | ||
| // We only track the RUST_CHECK var if it exists to prevent spurious rebuilds. | ||
| println!("cargo:rerun-if-env-changed=RUST_CHECK"); |
There was a problem hiding this comment.
This is pretty suspicious -- this env variable should get set by rustbuild only if LLVM hasn't been built and we don't expect to need it (check/clippy run).
causing it to rerun when checking after building
So if you build, then on a subsequent x.py check, RUST_CHECK shouldn't be set. I think the fault here lies with the logic around here (
Line 1391 in 3830eca
| cargo.env("LLVM_CONFIG", llvm_config.as_os_str().to_ascii_lowercase()); | ||
| } else { | ||
| cargo.env("LLVM_CONFIG", &llvm_config); | ||
| } |
There was a problem hiding this comment.
It feels like we'll be on a pretty long road to fixing this if we have to make this kind of conversion on every point where Cargo is going to be sensitive to this. Is there a chance this is something we can fix with rust-analyzer or otherwise? It feels a little weird that there's different cases floating around, even if the underlying filesystem is insensitive.
| cargo.env("LLVM_CONFIG", &llvm_config); | ||
|
|
||
| // HACK: on windows, paths are case insensitive, so case differences cause spurious rebuilds | ||
| if target.contains("windows") { |
There was a problem hiding this comment.
Why is the target important here? Shouldn't it be the host instead? Because one might cross compile Windows from Linux, which usually happens on a case sensitive file system. Or vice versa. Also, it's an imprecise generalization that windows paths are case insensitive, because you can enable case sensitive directories on Windows. Wouldn't this break in this instance then?
|
This would solve #92369 I presume? |
|
The difference we picked up there seems to be a |
|
Haven't looked at this in detail, but the existing review comments make sense to me - would like to see an answer to those. @rustbot author |
rustbot
added
S-waiting-on-author
|
ping from triage:
@drmeepster ☝️ can you address this? FYI: when a PR is ready for review, send a message containing |
|
Sorry for disappearing for a while! Using a separate build directory for rust-analyzer sidesteps this issue, so I don't really have the motivation to fix the problems with this PR. |
Dylan-DPC
removed
the
S-waiting-on-author
This PR fixes 2 sources of unnecessary rebuilds in
rustc_llvm's build script:LLVM_CONFIGandCFG_LLVM_ROOTenv variables tracked by the build script can have different casing between rust-analyzer and the terminal. This is solved by lowercasing the paths before setting the vars.rustc_llvm's build script checks for theRUST_CHECKenv var to avoid doing any work while just checking. However, it always tracked the var when doing so, causing it to rerun when checking after building. Then, this would cause it to rerun on the next build unnecessarily. This is solved by only trackingRUST_CHECKwhen it is present, causing it to only rebuild when needed.