Add if-identical mode for download-ci-llvm
#113761
Conversation
To make it easier to compare two LLVM configurations.
…mpile LLVM dist archive
It downloads llvm-opts from CI and checks if it is equal to the LLVM compiled on CI.
rustbot
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
src/bootstrap/config.rs
Outdated
| #[derive(Default, Clone)] | ||
| pub struct LLVMConfig { |
There was a problem hiding this comment.
this is essentially the same as config::Llvm struct, right? can we reuse that instead of adding a new struct?
(btw ty for putting this in a separate commit, it made things a lot easier to review ❤️)
There was a problem hiding this comment.
The goal of this struct is to have a "ground truth" representation of all the options that can affect the build of LLVM, so that we can compare the CI version and the local version in an easy way - by just comparing the value of a struct, without manually comparing a subset of its keys.
However, there are some options under the [llvm] key in config.toml which should not be compared - notably download-ci-llvm. We definitely do not want to include this key when comparing the CI and local configs. So this is not 1:1 with the LLVM struct.
Note: I only removed from_ci from this struct in a later commit, to uphold what I wrote above, which is probably confusing.
| /* run only if llvm-config isn't used */ | ||
| if let Some(config) = builder.config.target_config.get(&target) { | ||
| if let Some(ref _s) = config.llvm_config { | ||
| builder.info(&format!("Skipping RustDevConfig ({}): external LLVM", target)); | ||
| return None; | ||
| } | ||
| } |
There was a problem hiding this comment.
can you make this a default instead of a skip, so that we can give a hard error if someone runs x dist rust-dev-config when external llvm is set? see #113640 for an example.
There was a problem hiding this comment.
Like this?
fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let config = run.builder.config.target_config.get(&<how to get target?>).map(|c| c.llvm_config);
run.alias("rust-dev-config").default_condition(config.is_none())
}I'm not sure how to get the target, because run.builder contains a list of targets.
| /* run only if llvm-config isn't used */ | ||
| if let Some(config) = builder.config.target_config.get(&target) { | ||
| if let Some(ref _s) = config.llvm_config { | ||
| builder.info(&format!("Skipping RustDevConfig ({}): external LLVM", target)); |
There was a problem hiding this comment.
this is true but confusing when llvm_from_ci is set. maybe we can improve it a bit?
| builder.info(&format!("Skipping RustDevConfig ({}): external LLVM", target)); | |
| let reason = if builder.config.llvm_from_ci { "downloaded LLVM from CI instead of building it" } else { "external llvm" }; | |
| builder.info(&format!("Skipping RustDevConfig ({target}): {reason}"); |
There was a problem hiding this comment.
Good idea. However, I realized that this shouldn't fail when llvm_from_ci is true? Because this has to succeed when download-ci-llvm is if-identical. We should probably make llvm_from_ci an enum to distinguish these situations?
| let config = t!(serde_json::to_string_pretty(&builder.build.config.llvm)); | ||
| t!(std::fs::write(tarball.image_dir().join("llvm-opts.json"), config)); | ||
|
|
||
| Some(tarball.generate()) |
There was a problem hiding this comment.
this panics when running x dist rust-dev-config before the llvm submodule is checked out. could you add builder.update_submodule somewhere around
Lines 305 to 307 in 0a1b983
There was a problem hiding this comment.
Should I really update LLVM submodule in a function that creates a tarball? Shouldn't this be in RustDevConfig? It sounds quite LLVM specific to do this in the Tarball struct.
There was a problem hiding this comment.
hmm, i suppose doing it in this Step is fine. i was imaging you'd check overlay_kind in Tarball so each calling Step doesn't have to worry about it but in practice they're probably all doing the right thing.
| pub(crate) fn download_ci_llvm_opts(&self, llvm_sha: &str) { | ||
| let cache_prefix = format!("llvm-opts-{llvm_sha}"); | ||
| let cache_dst = self.out.join("cache"); | ||
| let rustc_cache = cache_dst.join(cache_prefix); | ||
| t!(fs::create_dir_all(&rustc_cache)); | ||
| let base = if self.llvm.assertions { | ||
| &self.stage0_metadata.config.artifacts_with_llvm_assertions_server | ||
| } else { | ||
| &self.stage0_metadata.config.artifacts_server | ||
| }; | ||
| let version = self.artifact_version_part(llvm_sha); | ||
| let filename = format!("rust-dev-config-{}-{}.tar.xz", version, self.build.triple); | ||
| let tarball = rustc_cache.join(&filename); | ||
| if !tarball.exists() { | ||
| let help_on_error = "error: failed to download llvm config from ci | ||
|
|
||
| help: old builds get deleted after a certain time | ||
| help: if trying to compile an old commit of rustc, disable `download-ci-llvm` in config.toml: | ||
|
|
||
| [llvm] | ||
| download-ci-llvm = false | ||
| "; | ||
| self.download_file(&format!("{base}/{llvm_sha}/{filename}"), &tarball, help_on_error); | ||
| } | ||
|
|
||
| let llvm_root = self.ci_llvm_root_opts(); | ||
| self.unpack(&tarball, &llvm_root, "rust-dev-config"); |
There was a problem hiding this comment.
i have a vague "this is duplicating a lot of code" feeling but nothing concrete - if you find some easy way to reduce the boilerplate here that would be nice, but no worries if it's tricky
There was a problem hiding this comment.
I still wonder if it wouldn't be simpler to just always download the LLVM archive (when using the if-identical mode) and embed the LLVM config directly inside of it. I think that the "hitrate" of this cache should be high enough (src/llvm-project doesn't change often, nor do build flags in dist builders), so downloading the archive everytime should be fine.
| // The LLVM config has changed its format or is corrupted in some way | ||
| eprintln!("Cannot deserialize LLVM config from llvm-opts.json"); | ||
| return None; |
There was a problem hiding this comment.
hmm, when would this happen? continuing the build when we can't parse the format seems unfortunate, i worry it'll silence a real bug.
There was a problem hiding this comment.
For example, if the struct is modified in a backwards incompatible way (e.g. new option added/removed), then it may fail to parse. This situation necessarily means that the config has changed somehow, and therefore we shouldn't reuse the LLVM from CI.
In theory, we could lose all caching if there was some bug and it was never deserialized properly, but the alternative is basically failing the build if the format changes, which we probably shouldn't do?
There was a problem hiding this comment.
Or we could devise some versioning scheme for the config (Protobuf? 😆 ), but that seems like overkill.
|
@bors try |
|
⌛ Trying commit de8e02c with merge d67dca47d47cdaa9066289e11501efdd553e56c0... |
the way download-rustc handles this is by making |
jyn514
added
A-LLVM
|
☀️ Try build successful - checks-actions |
1 similar comment
|
☀️ Try build successful - checks-actions |
|
Would it make sense to disable download-ci-llvm outside of the dev and nightly channels as safety guard for reproducibility if something ever goes wrong with this logic causing incorrect reuse of a precompiled LLVM. No artifacts in the dev and nightly channels are part of the bootstrap chain and stable and beta builds are much less common. |
|
to start i would actually like to only enable this in |
|
i won't have time to look at this for a while. r? infra-ci |
Dylan-DPC
added
S-waiting-on-author
|
@Kobzol any updates on this? thanks |
|
Hi, I'm no longer sure if this is a good idea, since we would not exercise the LLVM dist code paths. In any case, I don't have time to work on this currently, so I'll close it for now, and revisit it later if I come back to it. |
This PR tries to modify the
download-ci-llvmlogic so that it can support more use-cases, primarily downloading of LLVM from CI indistCI builders. Currently, LLVM can be downloaded from CI only if you do not set any LLVM flags onconfig.toml(apart from assertions) - because it is not possible to find out the options that were used to compile LLVM from the dist archive stored on S3.This means that
distbuilders have to build LLVM on every merge, even though the LLVM source code (nor its compilation options/parameters) have changed. It usually does not take too long to build LLVM thanks tosccache(usually about 10 - 20 minutes), but it's still a nontrivial cost when multiplied by the number of dist builders.This PR proposes a new workflow:
rust-dev, but then we would have to download this rather large archive whenif-identicalis used. I'm not sure if it would be that big of a deal, this mode is designed for CI, and most of the time it should be a "cache hit", when the LLVM options will be the same, and we will thus want to download LLVM anyway.if-identicalmode fordownload-ci-llvmis added. When used, it will try to download the LLVM opts from CI, and compare them to the local options. If they are identical, it will download LLVM from CI. This means that adistbuilder can use this option to download a previous LLVM build without rebuilding it from scratch if nothing related to LLVM has changed.The logic of
if-identicalis now a bit weird in that it always unconditionally tries to download the configuration from CI, which means that it does this e.g. even if I just runpython3 x.py fmt, which is definitely not something that we should do. I'm not sure how to resolve this though - maybe the logic of this mode should be "lazy", similarly as to how the value ofllvm_link_sharedis resolved lazily, based on data downloaded from CI?I'm opening this PR mostly for discussion, it's not clear to me if this is the right direction. One counter-argument to this could be that we should actually run the dist LLVM builds on every commit, just to check more often that it still works. If this is the case, I would still like to do something like this at least for try builds, which are latency-bounds, possibly with some ad-hoc way and without using
download-ci-llvm.Related issue: #112011
Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Downloading.20LLVM.20from.20CI.20in.20dist.20builders
r? jyn514