Skip to content

Commit

Permalink
detect incompatible CI LLVM options more precisely
Browse files Browse the repository at this point in the history
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 <work@onurozkan.dev>
  • Loading branch information
onur-ozkan committed Sep 8, 2024
1 parent 7b18b3e commit 9df7680
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 3 deletions.
112 changes: 110 additions & 2 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1216,13 +1216,19 @@ impl Config {
}
}

pub(crate) fn get_builder_toml(&self, build_name: &str) -> Result<TomlConfig, toml::de::Error> {
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<TomlConfig, toml::de::Error> {
pub(crate) fn get_toml(_: &Path) -> Result<TomlConfig, toml::de::Error> {
Ok(TomlConfig::default())
}

#[cfg(not(test))]
fn get_toml(file: &Path) -> Result<TomlConfig, toml::de::Error> {
pub(crate) fn get_toml(file: &Path) -> Result<TomlConfig, toml::de::Error> {
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
Expand Down Expand Up @@ -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(
Expand Down
31 changes: 30 additions & 1 deletion src/bootstrap/src/core/download.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
Expand Down Expand Up @@ -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());
Expand Down

0 comments on commit 9df7680

Please sign in to comment.