Skip to content

Commit 9df7680

Browse files
committed
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 <[email protected]>
1 parent 7b18b3e commit 9df7680

File tree

2 files changed

+140
-3
lines changed

2 files changed

+140
-3
lines changed

src/bootstrap/src/core/config/config.rs

Lines changed: 110 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,13 +1216,19 @@ impl Config {
12161216
}
12171217
}
12181218

1219+
pub(crate) fn get_builder_toml(&self, build_name: &str) -> Result<TomlConfig, toml::de::Error> {
1220+
let builder_config_path =
1221+
self.out.join(self.build.triple).join(build_name).join(BUILDER_CONFIG_FILENAME);
1222+
Self::get_toml(&builder_config_path)
1223+
}
1224+
12191225
#[cfg(test)]
1220-
fn get_toml(_: &Path) -> Result<TomlConfig, toml::de::Error> {
1226+
pub(crate) fn get_toml(_: &Path) -> Result<TomlConfig, toml::de::Error> {
12211227
Ok(TomlConfig::default())
12221228
}
12231229

12241230
#[cfg(not(test))]
1225-
fn get_toml(file: &Path) -> Result<TomlConfig, toml::de::Error> {
1231+
pub(crate) fn get_toml(file: &Path) -> Result<TomlConfig, toml::de::Error> {
12261232
let contents =
12271233
t!(fs::read_to_string(file), format!("config file {} not found", file.display()));
12281234
// Deserialize to Value and then TomlConfig to prevent the Deserialize impl of
@@ -2846,6 +2852,108 @@ impl Config {
28462852
}
28472853
}
28482854

2855+
/// Compares the current `Llvm` options against those in the CI LLVM builder and detects any incompatible options.
2856+
/// It does this by destructuring the `Llvm` instance to make sure every `Llvm` field is covered and not missing.
2857+
pub(crate) fn check_incompatible_options_for_ci_llvm(
2858+
current_config_toml: TomlConfig,
2859+
ci_config_toml: TomlConfig,
2860+
) -> Result<(), String> {
2861+
macro_rules! err {
2862+
($current:expr, $expected:expr) => {
2863+
if let Some(current) = &$current {
2864+
if Some(current) != $expected.as_ref() {
2865+
return Err(format!(
2866+
"ERROR: Setting `llvm.{}` is incompatible with `llvm.download-ci-llvm`. \
2867+
Current value: {:?}, Expected value(s): {}{:?}",
2868+
stringify!($expected).replace("_", "-"),
2869+
$current,
2870+
if $expected.is_some() { "None/" } else { "" },
2871+
$expected,
2872+
));
2873+
};
2874+
};
2875+
};
2876+
}
2877+
2878+
macro_rules! warn {
2879+
($current:expr, $expected:expr) => {
2880+
if let Some(current) = &$current {
2881+
if Some(current) != $expected.as_ref() {
2882+
println!(
2883+
"WARNING: `llvm.{}` has no effect with `llvm.download-ci-llvm`. \
2884+
Current value: {:?}, Expected value(s): {}{:?}",
2885+
stringify!($expected).replace("_", "-"),
2886+
$current,
2887+
if $expected.is_some() { "None/" } else { "" },
2888+
$expected,
2889+
);
2890+
};
2891+
};
2892+
};
2893+
}
2894+
2895+
let (Some(current_llvm_config), Some(ci_llvm_config)) =
2896+
(current_config_toml.llvm, ci_config_toml.llvm)
2897+
else {
2898+
return Ok(());
2899+
};
2900+
2901+
let Llvm {
2902+
optimize,
2903+
thin_lto,
2904+
release_debuginfo,
2905+
assertions: _,
2906+
tests: _,
2907+
plugins,
2908+
ccache: _,
2909+
static_libstdcpp: _,
2910+
libzstd,
2911+
ninja: _,
2912+
targets,
2913+
experimental_targets,
2914+
link_jobs: _,
2915+
link_shared: _,
2916+
version_suffix,
2917+
clang_cl,
2918+
cflags,
2919+
cxxflags,
2920+
ldflags,
2921+
use_libcxx,
2922+
use_linker,
2923+
allow_old_toolchain,
2924+
polly,
2925+
clang,
2926+
enable_warnings,
2927+
download_ci_llvm: _,
2928+
build_config,
2929+
enzyme,
2930+
} = ci_llvm_config;
2931+
2932+
err!(current_llvm_config.optimize, optimize);
2933+
err!(current_llvm_config.thin_lto, thin_lto);
2934+
err!(current_llvm_config.release_debuginfo, release_debuginfo);
2935+
err!(current_llvm_config.libzstd, libzstd);
2936+
err!(current_llvm_config.targets, targets);
2937+
err!(current_llvm_config.experimental_targets, experimental_targets);
2938+
err!(current_llvm_config.clang_cl, clang_cl);
2939+
err!(current_llvm_config.version_suffix, version_suffix);
2940+
err!(current_llvm_config.cflags, cflags);
2941+
err!(current_llvm_config.cxxflags, cxxflags);
2942+
err!(current_llvm_config.ldflags, ldflags);
2943+
err!(current_llvm_config.use_libcxx, use_libcxx);
2944+
err!(current_llvm_config.use_linker, use_linker);
2945+
err!(current_llvm_config.allow_old_toolchain, allow_old_toolchain);
2946+
err!(current_llvm_config.polly, polly);
2947+
err!(current_llvm_config.clang, clang);
2948+
err!(current_llvm_config.build_config, build_config);
2949+
err!(current_llvm_config.plugins, plugins);
2950+
err!(current_llvm_config.enzyme, enzyme);
2951+
2952+
warn!(current_llvm_config.enable_warnings, enable_warnings);
2953+
2954+
Ok(())
2955+
}
2956+
28492957
/// Compares the current Rust options against those in the CI rustc builder and detects any incompatible options.
28502958
/// It does this by destructuring the `Rust` instance to make sure every `Rust` field is covered and not missing.
28512959
fn check_incompatible_options_for_ci_rustc(

src/bootstrap/src/core/download.rs

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,7 @@ impl Config {
316316
let mut tar = tar::Archive::new(decompressor);
317317

318318
let is_ci_rustc = dst.ends_with("ci-rustc");
319+
let is_ci_llvm = dst.ends_with("ci-llvm");
319320

320321
// `compile::Sysroot` needs to know the contents of the `rustc-dev` tarball to avoid adding
321322
// it to the sysroot unless it was explicitly requested. But parsing the 100 MB tarball is slow.
@@ -332,7 +333,9 @@ impl Config {
332333
let mut short_path = t!(original_path.strip_prefix(directory_prefix));
333334
let is_builder_config = short_path.to_str() == Some(BUILDER_CONFIG_FILENAME);
334335

335-
if !(short_path.starts_with(pattern) || (is_ci_rustc && is_builder_config)) {
336+
if !(short_path.starts_with(pattern)
337+
|| ((is_ci_rustc || is_ci_llvm) && is_builder_config))
338+
{
336339
continue;
337340
}
338341
short_path = short_path.strip_prefix(pattern).unwrap_or(short_path);
@@ -717,17 +720,43 @@ download-rustc = false
717720

718721
#[cfg(not(feature = "bootstrap-self-test"))]
719722
pub(crate) fn maybe_download_ci_llvm(&self) {
723+
use build_helper::exit;
724+
720725
use crate::core::build_steps::llvm::detect_llvm_sha;
726+
use crate::core::config::check_incompatible_options_for_ci_llvm;
721727

722728
if !self.llvm_from_ci {
723729
return;
724730
}
731+
725732
let llvm_root = self.ci_llvm_root();
726733
let llvm_stamp = llvm_root.join(".llvm-stamp");
727734
let llvm_sha = detect_llvm_sha(self, self.rust_info.is_managed_git_subrepository());
728735
let key = format!("{}{}", llvm_sha, self.llvm_assertions);
729736
if program_out_of_date(&llvm_stamp, &key) && !self.dry_run() {
730737
self.download_ci_llvm(&llvm_sha);
738+
739+
if let Some(config_path) = &self.config {
740+
let current_config_toml = Self::get_toml(config_path).unwrap();
741+
742+
match self.get_builder_toml("ci-llvm") {
743+
Ok(ci_config_toml) => {
744+
check_incompatible_options_for_ci_llvm(current_config_toml, ci_config_toml)
745+
.unwrap();
746+
}
747+
Err(e) if e.to_string().contains("unknown field") => {
748+
println!(
749+
"WARNING: CI rustc has some fields that are no longer supported in bootstrap; download-rustc will be disabled."
750+
);
751+
println!("HELP: Consider rebasing to a newer commit if available.");
752+
}
753+
Err(e) => {
754+
eprintln!("ERROR: Failed to parse CI LLVM config.toml: {e}");
755+
exit!(2);
756+
}
757+
};
758+
};
759+
731760
if self.should_fix_bins_and_dylibs() {
732761
for entry in t!(fs::read_dir(llvm_root.join("bin"))) {
733762
self.fix_bin_or_dylib(&t!(entry).path());

0 commit comments

Comments
 (0)