Include a copy of compiler-rt source in the download-ci-llvm tarball
#129116
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
S-waiting-on-review
|
Can't we just precompile it and include the built artifacts in the archive? It seems a bit silly to download a subset of LLVM source code from CI just so that you don't have to checkout the submodule. |
|
For what it's worth, the tarball already contains thousands of LLVM header files, for building And it seems like it would be a lot of extra work to include |
|
We already build the profiler artifacts on the most common I like distributing the artifacts more, because it provides a better value to contributors. If we already go through the trouble of storing something extra there, and modifying bootstrap, we might as well provide the artifacts directly. Now, this of course assumes that the implementation of using the CI artifacts is not much more difficult than using the CI sources. That's probably something that we'd have to try and see. But I don't think that using the sources will be much easier, you'll also need to check whether the submodule has been initialized and whether Not sure if there is ever a need to compile the profiler builtins in some special way though. |
I'm a bit confused by this, because the contributor is still building the rest of the compiler and library anyway, and building
As far as I'm aware, using a prebuilt artifact would require an entirely new mechanism for using library artifacts from the unpacked Building from a different source directory would mostly come down to a few extra lines in |
I was thinking that we could reduce the need to use a C++ compiler. Even though it's currently also needed to build
You're right. For some reason, I thought that Nevermind then, I agree now that it's easier to distribute the sources than the artifacts. We should ideally include some detection that local sources will be used if they are checked out and modified, but I think that we already have that logic for |
|
Isn't it nicer security wise to ship as few binaries as possible? Also that can create platform incompatibilities, e.g. on NixOS LD is not at the standard path etc |
|
Well, I don't understand why downloading a 90 MiB archive (which needs to be redownloaded each time you change to a fresh commit) is better than checking out a git submodule that changes maybe once a month though. Just out of curiosity, what's the reason you want to avoid checking out the LLVM submodule @Zalathar? |
|
Then disregard my comment I don't know if this specific case would be a problem then |
|
So, my underlying motivation looks something like this:
|
|
Thank you for the context, now it makes more sense to me (especially given the Zulip thread). It's indeed not ideal that we don't run with profiler enabled in PR CI, that makes it more valuable to enable the profiler by default. |
|
@bors r+ |
bors
added
S-waiting-on-bors
Rollup of 7 pull requests Successful merges: - rust-lang#127679 (Stabilize `raw_ref_op` (RFC 2582)) - rust-lang#128084 (Suggest adding Result return type for associated method in E0277.) - rust-lang#128628 (When deduplicating unreachable blocks, erase the source information.) - rust-lang#128902 (doc: std::env::var: Returns None for names with '=' or NUL byte) - rust-lang#129048 (Update `crosstool-ng` for loongarch64) - rust-lang#129116 (Include a copy of `compiler-rt` source in the `download-ci-llvm` tarball) - rust-lang#129208 (Fix order of normalization and recursion in const folding.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#129116 - Zalathar:compiler-rt, r=Mark-Simulacrum Include a copy of `compiler-rt` source in the `download-ci-llvm` tarball This will make it possible to experiment with allowing `download-ci-llvm` builds to build `library/profiler_builtins`, without needing to check out the `src/llvm-project` submodule. By itself, this PR just adds the files to the tarball, but doesn't actually do anything with them. The idea is that once this is merged, it will then be much easier to proceed with work on the necessary bootstrap changes (using the real downloaded tarball), without having to rig up weird hacks to simulate downloading a modified tarball. --- Adding these files to the compressed tarballs appears to increase its size by a negligible amount (<1 MB out of 400/800+ MB). The uncompressed size is about 14 MB (out of 2+ GB for the whole tarball). (The excluded test files would have been another 35 MB.)
Build `library/profiler_builtins` from `ci-llvm` if appropriate Running all of `tests/coverage` requires the LLVM profiler runtime, which requires setting `build.profiler = true`. Historically, doing that has required checking out the entire `src/llvm-project` submodule. For compiler contributors who otherwise don't need that submodule (thanks to `download-ci-vm`), that's quite inconvenient. However, thanks to rust-lang#129116, the downloaded CI LLVM tarball now contains a copy of LLVM's `compiler-rt` directory, which includes all the files needed to build the profiler runtime. So with a little bit of extra logic in bootstrap, we can have `library/profiler_builtins` look for the `compiler-rt` files in `ci-llvm` instead of the `src/llvm-project` submodule.
Build `library/profiler_builtins` from `ci-llvm` if appropriate Running all of `tests/coverage` requires the LLVM profiler runtime, which requires setting `build.profiler = true`. Historically, doing that has required checking out the entire `src/llvm-project` submodule. For compiler contributors who otherwise don't need that submodule (thanks to `download-ci-vm`), that's quite inconvenient. However, thanks to rust-lang#129116, the downloaded CI LLVM tarball now contains a copy of LLVM's `compiler-rt` directory, which includes all the files needed to build the profiler runtime. So with a little bit of extra logic in bootstrap, we can have `library/profiler_builtins` look for the `compiler-rt` files in `ci-llvm` instead of the `src/llvm-project` submodule.
Build `library/profiler_builtins` from `ci-llvm` if appropriate Running all of `tests/coverage` requires the LLVM profiler runtime, which requires setting `build.profiler = true`. Historically, doing that has required checking out the entire `src/llvm-project` submodule. For compiler contributors who otherwise don't need that submodule (thanks to `download-ci-vm`), that's quite inconvenient. However, thanks to rust-lang#129116, the downloaded CI LLVM tarball now contains a copy of LLVM's `compiler-rt` directory, which includes all the files needed to build the profiler runtime. So with a little bit of extra logic in bootstrap, we can have `library/profiler_builtins` look for the `compiler-rt` files in `ci-llvm` instead of the `src/llvm-project` submodule.
Build `library/profiler_builtins` from `ci-llvm` if appropriate Running all of `tests/coverage` requires the LLVM profiler runtime, which requires setting `build.profiler = true`. Historically, doing that has required checking out the entire `src/llvm-project` submodule. For compiler contributors who otherwise don't need that submodule (thanks to `download-ci-vm`), that's quite inconvenient. However, thanks to rust-lang#129116, the downloaded CI LLVM tarball now contains a copy of LLVM's `compiler-rt` directory, which includes all the files needed to build the profiler runtime. So with a little bit of extra logic in bootstrap, we can have `library/profiler_builtins` look for the `compiler-rt` files in `ci-llvm` instead of the `src/llvm-project` submodule.
This will make it possible to experiment with allowing
download-ci-llvmbuilds to buildlibrary/profiler_builtins, without needing to check out thesrc/llvm-projectsubmodule.By itself, this PR just adds the files to the tarball, but doesn't actually do anything with them. The idea is that once this is merged, it will then be much easier to proceed with work on the necessary bootstrap changes (using the real downloaded tarball), without having to rig up weird hacks to simulate downloading a modified tarball.
Adding these files to the compressed tarballs appears to increase its size by a negligible amount (<1 MB out of 400/800+ MB).
The uncompressed size is about 14 MB (out of 2+ GB for the whole tarball).
(The excluded test files would have been another 35 MB.)