Fix bootstrap failing to copy rust-llvm-dwp when cross-compiling rustc #91270
Conversation
rust-highfive
added
the
S-waiting-on-review
src/bootstrap/compile.rs
Outdated
| builder.copy(&llvm_bin_dir.join(&src_exe), &libdir_bin.join(&dst_exe)); | ||
| } else { | ||
| // if cross-compiling don't copy the llvm directory for the target instead | ||
| let llvm_bin_dir = format!("build/{}/llvm/build/bin", target_compiler.host); |
There was a problem hiding this comment.
Hmm, would this work for the host too? It would be nice to do this unconditionally instead of making it different, people use download-ci-llvm a lot so this means it would get a lot more testing.
There was a problem hiding this comment.
I think it's better not to, as technically if not cross-compiling it's more correct to use llvm-config to have llvm get the path to itself (which, while better, isn't possible when the host and the target aren't the same)
jyn514
added
the
T-bootstrap
|
This looks good to me - @Mark-Simulacrum do you want to take a look or should I just approve myself? |
| } else { | ||
| // if cross-compiling don't copy from the llvm directory for the target instead | ||
| builder.llvm_out(target_compiler.host).join("build/bin") | ||
| }; |
There was a problem hiding this comment.
So I admit I may just be confused -- but it feels like llvm_bin_dir should ~match with llvm_config_bin returned from native::Llvm invocation? In particular, I would expect llvm-config to be in the same directory as llvm-dwp. So maybe we should skip the --bindir invocation regardless of cross-compilation and just copy llvm_config_bin.with_filename(src_exe)?
If that doesn't work -- presumably for the reason we started invoking bindir here -- it would be good to figure out why. Because it seems like the new code makes the assumption that it should work, which can then presumably fail for whatever reason we started using --bindir.
In some sense I guess this gets at "why does --bindir not work for cross-compilation"?
Mark-Simulacrum
added
S-waiting-on-author
apiraino
added
the
T-infra
|
@jam1garner Ping from triage, any updates on this? |
At this point, my usecase for this fix has shifted off months ago (not long after my original issue) and I'm not confident in my ability to test how any larger changes to the fix would work, so I'd personally be in favor of closing the PR and it just being reference material if anyone hits this issue again. The effort it'd take to recreate the original circumstances just aren't justified by the value of the patch to me personally. Was only able to comply with changes to the patchset this far due to being able to test for equivalent behavior in a vacuum, but I think the proposed rework would exceed that. Not going to close it myself in case others disagree with that being the course of action to take. I appreciate the effort by Mark and Joshua to review my fix, and sorry for not still having access to the build infrastructure needed to test further fixes. If anyone down the road hitting this issue needs anything from me feel free to contact me here, on Zulip, on Twitter, or by email. (In order of preference) |
Fixes #85593
r? @jyn514