Skip to content

fix rustc_llvm spurious rebuilds #100152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix rustc_llvm spurious rebuilds #100152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion compiler/rustc_llvm/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,10 @@ fn main() {
println!("cargo:rustc-check-cfg=values(llvm_component,\"{}\")", component);
}

if tracked_env_var_os("RUST_CHECK").is_some() {
if env::var_os("RUST_CHECK").is_some() {
// 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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (

rust/src/bootstrap/builder.rs

Line 1391 in 3830eca

if crate::native::prebuilt_llvm_config(self, target).is_err() {
), not this build script.

return;
}

Expand Down
16 changes: 14 additions & 2 deletions src/bootstrap/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,9 +780,21 @@ pub fn rustc_cargo_env(builder: &Builder<'_>, cargo: &mut Cargo, target: TargetS
cargo.env("LLVM_RUSTLLVM", "1");
}
let llvm_config = builder.ensure(native::Llvm { target });
cargo.env("LLVM_CONFIG", &llvm_config);

// HACK: on windows, paths are case insensitive, so case differences cause spurious rebuilds
if target.contains("windows") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

cargo.env("LLVM_CONFIG", llvm_config.as_os_str().to_ascii_lowercase());
} else {
cargo.env("LLVM_CONFIG", &llvm_config);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if let Some(s) = target_config.and_then(|c| c.llvm_config.as_ref()) {
cargo.env("CFG_LLVM_ROOT", s);
// HACK: on windows, paths are case insensitive, so case differences cause spurious rebuilds
if target.contains("windows") {
cargo.env("CFG_LLVM_ROOT", s.as_os_str().to_ascii_lowercase());
} else {
cargo.env("CFG_LLVM_ROOT", s);
}
}

// Some LLVM linker flags (-L and -l) may be needed to link `rustc_llvm`. Its build script
Expand Down