Offload cmake cleanups - #162196
Conversation
|
This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. |
rustbot
added
A-rustc-dev-guide
|
|
This comment has been minimized.
This comment has been minimized.
Originally introduced in the Enzyme build and coppied around since, but without effect for a few years by now.
9d399d1 to
341d294
Compare
| /// | ||
| /// Deliberately not under `llvm_output_dir`, since running cmake twice in the same folder is | ||
| /// known to cause issues, like deleting existing binaries. | ||
| pub fn offload_out(&self, target: TargetSelection) -> PathBuf { |
There was a problem hiding this comment.
I deliberately removed these functions recently, because they are an antipattern 😅 Steps should be ensuring other steps to get the build directory, rather than implicitly depending on paths from a shared function.
| @@ -2899,7 +2898,7 @@ impl CommandLineStep for Offload { | |||
| tarball.set_overlay(OverlayKind::Offload); | |||
| tarball.is_preview(true); | |||
|
|
|||
| let omp_offload_libdir = builder.out.join(target).join("offload").join("lib"); | |||
| let omp_offload_libdir = builder.offload_out(target).join("lib"); | |||
There was a problem hiding this comment.
This should be using the path from the omp_offload step output.
| // binaries. We therefore write our offload artifacts into it's own folder, instead of | ||
| // using the llvm build dir. | ||
| let out_dir = builder.out.join(self.target.triple).join("offload"); | ||
| let out_dir = builder.offload_out(self.target); |
There was a problem hiding this comment.
This should hardcode the path here, this is the canonical place where that path is decided.
4 participants
r? kobzol
Split out of #161565, which requires (minor) LLVM changes.
These cleanups are valuable on their own already, and shouldn't be blocked. So let's land it, before it get's stale with all the refactoring going on ^^
Copied the disclaimer: I used an llm to implement the fixes, since I still don't like cmake.
I confirm that on a high-level these are the right fixes as far as I can tell, and I reviewed/refactored it.