From 9df7680ecf698bf7087616b595774ee1023d3c7b Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 30 Aug 2024 19:48:16 +0300 Subject: [PATCH 1/6] detect incompatible CI LLVM options more precisely Previously, the logic here was simply checking whether the option was set in `config.toml`. This approach was not manageable in our CI runners as we set so many options in config.toml. In reality, those values are not incompatible since they are usually the same value used to generate the CI llvm. Now, the new logic compares the configuration values with the values used to generate the CI llvm, so we get more precise results and make the process more manageable. Signed-off-by: onur-ozkan --- src/bootstrap/src/core/config/config.rs | 112 +++++++++++++++++++++++- src/bootstrap/src/core/download.rs | 31 ++++++- 2 files changed, 140 insertions(+), 3 deletions(-) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 52c1c462788af..57c260a062486 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1216,13 +1216,19 @@ impl Config { } } + pub(crate) fn get_builder_toml(&self, build_name: &str) -> Result { + let builder_config_path = + self.out.join(self.build.triple).join(build_name).join(BUILDER_CONFIG_FILENAME); + Self::get_toml(&builder_config_path) + } + #[cfg(test)] - fn get_toml(_: &Path) -> Result { + pub(crate) fn get_toml(_: &Path) -> Result { Ok(TomlConfig::default()) } #[cfg(not(test))] - fn get_toml(file: &Path) -> Result { + pub(crate) fn get_toml(file: &Path) -> Result { let contents = t!(fs::read_to_string(file), format!("config file {} not found", file.display())); // Deserialize to Value and then TomlConfig to prevent the Deserialize impl of @@ -2846,6 +2852,108 @@ impl Config { } } +/// Compares the current `Llvm` options against those in the CI LLVM builder and detects any incompatible options. +/// It does this by destructuring the `Llvm` instance to make sure every `Llvm` field is covered and not missing. +pub(crate) fn check_incompatible_options_for_ci_llvm( + current_config_toml: TomlConfig, + ci_config_toml: TomlConfig, +) -> Result<(), String> { + macro_rules! err { + ($current:expr, $expected:expr) => { + if let Some(current) = &$current { + if Some(current) != $expected.as_ref() { + return Err(format!( + "ERROR: Setting `llvm.{}` is incompatible with `llvm.download-ci-llvm`. \ + Current value: {:?}, Expected value(s): {}{:?}", + stringify!($expected).replace("_", "-"), + $current, + if $expected.is_some() { "None/" } else { "" }, + $expected, + )); + }; + }; + }; + } + + macro_rules! warn { + ($current:expr, $expected:expr) => { + if let Some(current) = &$current { + if Some(current) != $expected.as_ref() { + println!( + "WARNING: `llvm.{}` has no effect with `llvm.download-ci-llvm`. \ + Current value: {:?}, Expected value(s): {}{:?}", + stringify!($expected).replace("_", "-"), + $current, + if $expected.is_some() { "None/" } else { "" }, + $expected, + ); + }; + }; + }; + } + + let (Some(current_llvm_config), Some(ci_llvm_config)) = + (current_config_toml.llvm, ci_config_toml.llvm) + else { + return Ok(()); + }; + + let Llvm { + optimize, + thin_lto, + release_debuginfo, + assertions: _, + tests: _, + plugins, + ccache: _, + static_libstdcpp: _, + libzstd, + ninja: _, + targets, + experimental_targets, + link_jobs: _, + link_shared: _, + version_suffix, + clang_cl, + cflags, + cxxflags, + ldflags, + use_libcxx, + use_linker, + allow_old_toolchain, + polly, + clang, + enable_warnings, + download_ci_llvm: _, + build_config, + enzyme, + } = ci_llvm_config; + + err!(current_llvm_config.optimize, optimize); + err!(current_llvm_config.thin_lto, thin_lto); + err!(current_llvm_config.release_debuginfo, release_debuginfo); + err!(current_llvm_config.libzstd, libzstd); + err!(current_llvm_config.targets, targets); + err!(current_llvm_config.experimental_targets, experimental_targets); + err!(current_llvm_config.clang_cl, clang_cl); + err!(current_llvm_config.version_suffix, version_suffix); + err!(current_llvm_config.cflags, cflags); + err!(current_llvm_config.cxxflags, cxxflags); + err!(current_llvm_config.ldflags, ldflags); + err!(current_llvm_config.use_libcxx, use_libcxx); + err!(current_llvm_config.use_linker, use_linker); + err!(current_llvm_config.allow_old_toolchain, allow_old_toolchain); + err!(current_llvm_config.polly, polly); + err!(current_llvm_config.clang, clang); + err!(current_llvm_config.build_config, build_config); + err!(current_llvm_config.plugins, plugins); + err!(current_llvm_config.enzyme, enzyme); + + warn!(current_llvm_config.enable_warnings, enable_warnings); + + Ok(()) +} + /// Compares the current Rust options against those in the CI rustc builder and detects any incompatible options. /// It does this by destructuring the `Rust` instance to make sure every `Rust` field is covered and not missing. fn check_incompatible_options_for_ci_rustc( diff --git a/src/bootstrap/src/core/download.rs b/src/bootstrap/src/core/download.rs index 3f5ec70b9abc9..25493c1f55b6c 100644 --- a/src/bootstrap/src/core/download.rs +++ b/src/bootstrap/src/core/download.rs @@ -316,6 +316,7 @@ impl Config { let mut tar = tar::Archive::new(decompressor); let is_ci_rustc = dst.ends_with("ci-rustc"); + let is_ci_llvm = dst.ends_with("ci-llvm"); // `compile::Sysroot` needs to know the contents of the `rustc-dev` tarball to avoid adding // it to the sysroot unless it was explicitly requested. But parsing the 100 MB tarball is slow. @@ -332,7 +333,9 @@ impl Config { let mut short_path = t!(original_path.strip_prefix(directory_prefix)); let is_builder_config = short_path.to_str() == Some(BUILDER_CONFIG_FILENAME); - if !(short_path.starts_with(pattern) || (is_ci_rustc && is_builder_config)) { + if !(short_path.starts_with(pattern) + || ((is_ci_rustc || is_ci_llvm) && is_builder_config)) + { continue; } short_path = short_path.strip_prefix(pattern).unwrap_or(short_path); @@ -717,17 +720,43 @@ download-rustc = false #[cfg(not(feature = "bootstrap-self-test"))] pub(crate) fn maybe_download_ci_llvm(&self) { + use build_helper::exit; + use crate::core::build_steps::llvm::detect_llvm_sha; + use crate::core::config::check_incompatible_options_for_ci_llvm; if !self.llvm_from_ci { return; } + let llvm_root = self.ci_llvm_root(); let llvm_stamp = llvm_root.join(".llvm-stamp"); let llvm_sha = detect_llvm_sha(self, self.rust_info.is_managed_git_subrepository()); let key = format!("{}{}", llvm_sha, self.llvm_assertions); if program_out_of_date(&llvm_stamp, &key) && !self.dry_run() { self.download_ci_llvm(&llvm_sha); + + if let Some(config_path) = &self.config { + let current_config_toml = Self::get_toml(config_path).unwrap(); + + match self.get_builder_toml("ci-llvm") { + Ok(ci_config_toml) => { + check_incompatible_options_for_ci_llvm(current_config_toml, ci_config_toml) + .unwrap(); + } + Err(e) if e.to_string().contains("unknown field") => { + println!( + "WARNING: CI rustc has some fields that are no longer supported in bootstrap; download-rustc will be disabled." + ); + println!("HELP: Consider rebasing to a newer commit if available."); + } + Err(e) => { + eprintln!("ERROR: Failed to parse CI LLVM config.toml: {e}"); + exit!(2); + } + }; + }; + if self.should_fix_bins_and_dylibs() { for entry in t!(fs::read_dir(llvm_root.join("bin"))) { self.fix_bin_or_dylib(&t!(entry).path()); From 23df3a9eeb01d3a50b2ce02db9f69e1d3e37caec Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 30 Aug 2024 19:50:52 +0300 Subject: [PATCH 2/6] remove `check_ci_llvm` usage Signed-off-by: onur-ozkan --- src/bootstrap/src/core/config/config.rs | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 57c260a062486..824f65cb6b8e5 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1914,30 +1914,6 @@ impl Config { "HELP: To use `llvm.libzstd` for LLVM/LLD builds, set `download-ci-llvm` option to false." ); } - - // None of the LLVM options, except assertions, are supported - // when using downloaded LLVM. We could just ignore these but - // that's potentially confusing, so force them to not be - // explicitly set. The defaults and CI defaults don't - // necessarily match but forcing people to match (somewhat - // arbitrary) CI configuration locally seems bad/hard. - check_ci_llvm!(optimize_toml); - check_ci_llvm!(thin_lto); - check_ci_llvm!(release_debuginfo); - check_ci_llvm!(targets); - check_ci_llvm!(experimental_targets); - check_ci_llvm!(clang_cl); - check_ci_llvm!(version_suffix); - check_ci_llvm!(cflags); - check_ci_llvm!(cxxflags); - check_ci_llvm!(ldflags); - check_ci_llvm!(use_libcxx); - check_ci_llvm!(use_linker); - check_ci_llvm!(allow_old_toolchain); - check_ci_llvm!(polly); - check_ci_llvm!(clang); - check_ci_llvm!(build_config); - check_ci_llvm!(plugins); } // NOTE: can never be hit when downloading from CI, since we call `check_ci_llvm!(thin_lto)` above. From ea34bb045264466c6b3bc63caf64264de219ad77 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 30 Aug 2024 19:56:58 +0300 Subject: [PATCH 3/6] print incompatible options even if we don't download Signed-off-by: onur-ozkan --- src/bootstrap/src/core/download.rs | 41 +++++++++++++++--------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/src/bootstrap/src/core/download.rs b/src/bootstrap/src/core/download.rs index 25493c1f55b6c..d8b39ac0b6dac 100644 --- a/src/bootstrap/src/core/download.rs +++ b/src/bootstrap/src/core/download.rs @@ -736,27 +736,6 @@ download-rustc = false if program_out_of_date(&llvm_stamp, &key) && !self.dry_run() { self.download_ci_llvm(&llvm_sha); - if let Some(config_path) = &self.config { - let current_config_toml = Self::get_toml(config_path).unwrap(); - - match self.get_builder_toml("ci-llvm") { - Ok(ci_config_toml) => { - check_incompatible_options_for_ci_llvm(current_config_toml, ci_config_toml) - .unwrap(); - } - Err(e) if e.to_string().contains("unknown field") => { - println!( - "WARNING: CI rustc has some fields that are no longer supported in bootstrap; download-rustc will be disabled." - ); - println!("HELP: Consider rebasing to a newer commit if available."); - } - Err(e) => { - eprintln!("ERROR: Failed to parse CI LLVM config.toml: {e}"); - exit!(2); - } - }; - }; - if self.should_fix_bins_and_dylibs() { for entry in t!(fs::read_dir(llvm_root.join("bin"))) { self.fix_bin_or_dylib(&t!(entry).path()); @@ -789,6 +768,26 @@ download-rustc = false t!(fs::write(llvm_stamp, key)); } + + if let Some(config_path) = &self.config { + let current_config_toml = Self::get_toml(config_path).unwrap(); + + match self.get_builder_toml("ci-llvm") { + Ok(ci_config_toml) => { + t!(check_incompatible_options_for_ci_llvm(current_config_toml, ci_config_toml)); + } + Err(e) if e.to_string().contains("unknown field") => { + println!( + "WARNING: CI LLVM has some fields that are no longer supported in bootstrap; download-ci-llvm will be disabled." + ); + println!("HELP: Consider rebasing to a newer commit if available."); + } + Err(e) => { + eprintln!("ERROR: Failed to parse CI LLVM config.toml: {e}"); + exit!(2); + } + }; + }; } #[cfg(not(feature = "bootstrap-self-test"))] From 018ed9abb92496373da0dcf594767846eaeec4e3 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 30 Aug 2024 20:01:05 +0300 Subject: [PATCH 4/6] fix llvm ThinLTO behaviour Signed-off-by: onur-ozkan --- src/bootstrap/src/core/config/config.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 824f65cb6b8e5..327cf6c575a57 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1916,8 +1916,7 @@ impl Config { } } - // NOTE: can never be hit when downloading from CI, since we call `check_ci_llvm!(thin_lto)` above. - if config.llvm_thin_lto && link_shared.is_none() { + if !config.llvm_from_ci && config.llvm_thin_lto && link_shared.is_none() { // If we're building with ThinLTO on, by default we want to link // to LLVM shared, to avoid re-doing ThinLTO (which happens in // the link step) with each stage. From 13e16a910144492ed3896fb22c9c980bc474b174 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 30 Aug 2024 20:05:55 +0300 Subject: [PATCH 5/6] use `Config::get_builder_toml` for ci-rustc config parsing Signed-off-by: onur-ozkan --- src/bootstrap/src/core/config/config.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 327cf6c575a57..8f7195aba9a8c 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -2355,10 +2355,7 @@ impl Config { self.download_ci_rustc(commit); if let Some(config_path) = &self.config { - let builder_config_path = - self.out.join(self.build.triple).join("ci-rustc").join(BUILDER_CONFIG_FILENAME); - - let ci_config_toml = match Self::get_toml(&builder_config_path) { + let ci_config_toml = match self.get_builder_toml("ci-rustc") { Ok(ci_config_toml) => ci_config_toml, Err(e) if e.to_string().contains("unknown field") => { println!("WARNING: CI rustc has some fields that are no longer supported in bootstrap; download-rustc will be disabled."); @@ -2366,7 +2363,7 @@ impl Config { return None; }, Err(e) => { - eprintln!("ERROR: Failed to parse CI rustc config at '{}': {e}", builder_config_path.display()); + eprintln!("ERROR: Failed to parse CI rustc config.toml: {e}"); exit!(2); }, }; @@ -2829,6 +2826,7 @@ impl Config { /// Compares the current `Llvm` options against those in the CI LLVM builder and detects any incompatible options. /// It does this by destructuring the `Llvm` instance to make sure every `Llvm` field is covered and not missing. +#[cfg(not(feature = "bootstrap-self-test"))] pub(crate) fn check_incompatible_options_for_ci_llvm( current_config_toml: TomlConfig, ci_config_toml: TomlConfig, From 7b8cbe4f1ca1d870b2bb2196d8c3a4bc2a978f55 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Fri, 30 Aug 2024 22:56:11 +0300 Subject: [PATCH 6/6] handle dry-run mode in `Config::get_builder_toml` Signed-off-by: onur-ozkan --- src/bootstrap/src/core/config/config.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 8f7195aba9a8c..68b42305fdcc8 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1217,6 +1217,10 @@ impl Config { } pub(crate) fn get_builder_toml(&self, build_name: &str) -> Result { + if self.dry_run() { + return Ok(TomlConfig::default()); + } + let builder_config_path = self.out.join(self.build.triple).join(build_name).join(BUILDER_CONFIG_FILENAME); Self::get_toml(&builder_config_path)