mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #129052 - onur-ozkan:better-incompatibility-check, r=Kobzol

Browse Source 
detect incompatible CI rustc 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 rustc. Now, the new logic compares the configuration values with the values used to generate the CI rustc, so we get more precise results and make the process more manageable.

r? Kobzol

Blocker for https://github.com/rust-lang/rust/pull/122709
This commit is contained in:
bors 2024-08-16 11:52:38 +00:00
commit 27b93da8de
4 changed files with 166 additions and 100 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

240
src/bootstrap/src/core/config/config.rs
View File

@ -36,6 +36,14 @@ macro_rules! check_ci_llvm {
};
}
/// This file is embedded in the overlay directory of the tarball sources. It is
/// useful in scenarios where developers want to see how the tarball sources were
/// generated.
///
/// We also use this file to compare the host's config.toml against the CI rustc builder
/// configuration to detect any incompatible options.
pub(crate) const BUILDER_CONFIG_FILENAME: &str = "builder-config";
#[derive(Clone, Default)]
pub enum DryRun {
/// This isn't a dry run.
@ -47,7 +55,7 @@ pub enum DryRun {
UserSelected,
}
#[derive(Copy, Clone, Default, PartialEq, Eq)]
#[derive(Copy, Clone, Default, Debug, Eq, PartialEq)]
pub enum DebuginfoLevel {
#[default]
None,
@ -117,7 +125,7 @@ impl Display for DebuginfoLevel {
/// 2) MSVC
/// - Self-contained: `-Clinker=<path to rust-lld>`
/// - External: `-Clinker=lld`
#[derive(Default, Copy, Clone)]
#[derive(Copy, Clone, Default, Debug, PartialEq)]
pub enum LldMode {
/// Do not use LLD
#[default]
@ -1203,40 +1211,42 @@ impl Config {
}
}
pub fn parse(flags: Flags) -> Config {
#[cfg(test)]
fn get_toml(_: &Path) -> TomlConfig {
TomlConfig::default()
}
#[cfg(not(test))]
fn get_toml(file: &Path) -> TomlConfig {
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
// TomlConfig and sub types to be monomorphized 5x by toml.
toml::from_str(&contents)
.and_then(|table: toml::Value| TomlConfig::deserialize(table))
.unwrap_or_else(|err| {
if let Ok(Some(changes)) = toml::from_str(&contents)
.and_then(|table: toml::Value| ChangeIdWrapper::deserialize(table)).map(|change_id| change_id.inner.map(crate::find_recent_config_change_ids))
{
if !changes.is_empty() {
println!(
"WARNING: There have been changes to x.py since you last updated:\n{}",
crate::human_readable_changes(&changes)
);
}
}
eprintln!("failed to parse TOML configuration '{}': {err}", file.display());
exit!(2);
})
}
Self::parse_inner(flags, get_toml)
#[cfg(test)]
fn get_toml(_: &Path) -> Result<TomlConfig, toml::de::Error> {
Ok(TomlConfig::default())
}
pub(crate) fn parse_inner(mut flags: Flags, get_toml: impl Fn(&Path) -> TomlConfig) -> Config {
#[cfg(not(test))]
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
// TomlConfig and sub types to be monomorphized 5x by toml.
toml::from_str(&contents)
.and_then(|table: toml::Value| TomlConfig::deserialize(table))
.inspect_err(|_| {
if let Ok(Some(changes)) = toml::from_str(&contents)
.and_then(|table: toml::Value| ChangeIdWrapper::deserialize(table))
.map(|change_id| change_id.inner.map(crate::find_recent_config_change_ids))
{
if !changes.is_empty() {
println!(
"WARNING: There have been changes to x.py since you last updated:\n{}",
crate::human_readable_changes(&changes)
);
}
}
})
}
pub fn parse(flags: Flags) -> Config {
Self::parse_inner(flags, Self::get_toml)
}
pub(crate) fn parse_inner(
mut flags: Flags,
get_toml: impl Fn(&Path) -> Result<TomlConfig, toml::de::Error>,
) -> Config {
let mut config = Config::default_opts();
// Set flags.
@ -1344,7 +1354,10 @@ impl Config {
} else {
toml_path.clone()
});
get_toml(&toml_path)
get_toml(&toml_path).unwrap_or_else(|e| {
eprintln!("ERROR: Failed to parse '{}': {e}", toml_path.display());
exit!(2);
})
} else {
config.config = None;
TomlConfig::default()
@ -1375,7 +1388,13 @@ impl Config {
include_path.push("bootstrap");
include_path.push("defaults");
include_path.push(format!("config.{include}.toml"));
let included_toml = get_toml(&include_path);
let included_toml = get_toml(&include_path).unwrap_or_else(|e| {
eprintln!(
"ERROR: Failed to parse default config profile at '{}': {e}",
include_path.display()
);
exit!(2);
});
toml.merge(included_toml, ReplaceOpt::IgnoreDuplicate);
}
@ -1591,24 +1610,6 @@ impl Config {
let mut is_user_configured_rust_channel = false;
if let Some(rust) = toml.rust {
if let Some(commit) = config.download_ci_rustc_commit(rust.download_rustc.clone()) {
// Primarily used by CI runners to avoid handling download-rustc incompatible
// options one by one on shell scripts.
let disable_ci_rustc_if_incompatible =
env::var_os("DISABLE_CI_RUSTC_IF_INCOMPATIBLE")
.is_some_and(|s| s == "1" || s == "true");
if let Err(e) = check_incompatible_options_for_ci_rustc(&rust) {
if disable_ci_rustc_if_incompatible {
config.download_rustc_commit = None;
} else {
panic!("{}", e);
}
} else {
config.download_rustc_commit = Some(commit);
}
}
let Rust {
optimize: optimize_toml,
debug: debug_toml,
@ -1656,7 +1657,7 @@ impl Config {
new_symbol_mangling,
profile_generate,
profile_use,
download_rustc: _,
download_rustc,
lto,
validate_mir_opts,
frame_pointers,
@ -1668,6 +1669,8 @@ impl Config {
is_user_configured_rust_channel = channel.is_some();
set(&mut config.channel, channel.clone());
config.download_rustc_commit = config.download_ci_rustc_commit(download_rustc);
debug = debug_toml;
debug_assertions = debug_assertions_toml;
debug_assertions_std = debug_assertions_std_toml;
@ -2345,6 +2348,45 @@ impl Config {
None => None,
Some(commit) => {
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) {
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.");
println!("HELP: Consider rebasing to a newer commit if available.");
return None;
},
Err(e) => {
eprintln!("ERROR: Failed to parse CI rustc config at '{}': {e}", builder_config_path.display());
exit!(2);
},
};
let current_config_toml = Self::get_toml(config_path).unwrap();
// Check the config compatibility
// FIXME: this doesn't cover `--set` flags yet.
let res = check_incompatible_options_for_ci_rustc(
current_config_toml,
ci_config_toml,
);
let disable_ci_rustc_if_incompatible =
env::var_os("DISABLE_CI_RUSTC_IF_INCOMPATIBLE")
.is_some_and(|s| s == "1" || s == "true");
if disable_ci_rustc_if_incompatible && res.is_err() {
println!("WARNING: download-rustc is disabled with `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` env.");
return None;
}
res.unwrap();
}
Some(commit.clone())
}
})
@ -2662,31 +2704,52 @@ impl Config {
}
}
/// Checks the CI rustc incompatible options by destructuring the `Rust` instance
/// and makes sure that no rust options from config.toml are missed.
fn check_incompatible_options_for_ci_rustc(rust: &Rust) -> Result<(), String> {
/// 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(
current_config_toml: TomlConfig,
ci_config_toml: TomlConfig,
) -> Result<(), String> {
macro_rules! err {
($name:expr) => {
if $name.is_some() {
return Err(format!(
"ERROR: Setting `rust.{}` is incompatible with `rust.download-rustc`.",
stringify!($name).replace("_", "-")
));
}
($current:expr, $expected:expr) => {
if let Some(current) = &$current {
if Some(current) != $expected.as_ref() {
return Err(format!(
"ERROR: Setting `rust.{}` is incompatible with `rust.download-rustc`. \
Current value: {:?}, Expected value(s): {}{:?}",
stringify!($expected).replace("_", "-"),
$current,
if $expected.is_some() { "None/" } else { "" },
$expected,
));
};
};
};
}
macro_rules! warn {
($name:expr) => {
if $name.is_some() {
println!(
"WARNING: `rust.{}` has no effect with `rust.download-rustc`.",
stringify!($name).replace("_", "-")
);
}
($current:expr, $expected:expr) => {
if let Some(current) = &$current {
if Some(current) != $expected.as_ref() {
println!(
"WARNING: `rust.{}` has no effect with `rust.download-rustc`. \
Current value: {:?}, Expected value(s): {}{:?}",
stringify!($expected).replace("_", "-"),
$current,
if $expected.is_some() { "None/" } else { "" },
$expected,
);
};
};
};
}
let (Some(current_rust_config), Some(ci_rust_config)) =
(current_config_toml.rust, ci_config_toml.rust)
else {
return Ok(());
};
let Rust {
// Following options are the CI rustc incompatible ones.
optimize,
@ -2744,7 +2807,7 @@ fn check_incompatible_options_for_ci_rustc(rust: &Rust) -> Result<(), String> {
download_rustc: _,
validate_mir_opts: _,
frame_pointers: _,
} = rust;
} = ci_rust_config;
// There are two kinds of checks for CI rustc incompatible options:
// 1. Checking an option that may change the compiler behaviour/output.
@ -2752,22 +2815,23 @@ fn check_incompatible_options_for_ci_rustc(rust: &Rust) -> Result<(), String> {
//
// If the option belongs to the first category, we call `err` macro for a hard error;
// otherwise, we just print a warning with `warn` macro.
err!(optimize);
err!(debug_logging);
err!(debuginfo_level_rustc);
err!(default_linker);
err!(rpath);
err!(strip);
err!(stack_protector);
err!(lld_mode);
err!(llvm_tools);
err!(llvm_bitcode_linker);
err!(jemalloc);
err!(lto);
warn!(channel);
warn!(description);
warn!(incremental);
err!(current_rust_config.optimize, optimize);
err!(current_rust_config.debug_logging, debug_logging);
err!(current_rust_config.debuginfo_level_rustc, debuginfo_level_rustc);
err!(current_rust_config.rpath, rpath);
err!(current_rust_config.strip, strip);
err!(current_rust_config.lld_mode, lld_mode);
err!(current_rust_config.llvm_tools, llvm_tools);
err!(current_rust_config.llvm_bitcode_linker, llvm_bitcode_linker);
err!(current_rust_config.jemalloc, jemalloc);
err!(current_rust_config.default_linker, default_linker);
err!(current_rust_config.stack_protector, stack_protector);
err!(current_rust_config.lto, lto);
warn!(current_rust_config.channel, channel);
warn!(current_rust_config.description, description);
warn!(current_rust_config.incremental, incremental);
Ok(())
}

11
src/bootstrap/src/core/config/tests.rs
View File

@ -14,7 +14,7 @@ use crate::core::config::{LldMode, Target, TargetSelection, TomlConfig};
fn parse(config: &str) -> Config {
Config::parse_inner(
Flags::parse(&["check".to_string(), "--config=/does/not/exist".to_string()]),
|&_| toml::from_str(&config).unwrap(),
|&_| toml::from_str(&config),
)
}
@ -151,7 +151,6 @@ runner = "x86_64-runner"
"#,
)
.unwrap()
},
);
assert_eq!(config.change_id, Some(1), "setting top-level value");
@ -208,13 +207,13 @@ fn override_toml_duplicate() {
"--set=change-id=1".to_owned(),
"--set=change-id=2".to_owned(),
]),
|&_| toml::from_str("change-id = 0").unwrap(),
|&_| toml::from_str("change-id = 0"),
);
}
#[test]
fn profile_user_dist() {
fn get_toml(file: &Path) -> TomlConfig {
fn get_toml(file: &Path) -> Result<TomlConfig, toml::de::Error> {
let contents =
if file.ends_with("config.toml") || env::var_os("RUST_BOOTSTRAP_CONFIG").is_some() {
"profile = \"user\"".to_owned()
@ -223,9 +222,7 @@ fn profile_user_dist() {
std::fs::read_to_string(file).unwrap()
};
toml::from_str(&contents)
.and_then(|table: toml::Value| TomlConfig::deserialize(table))
.unwrap()
toml::from_str(&contents).and_then(|table: toml::Value| TomlConfig::deserialize(table))
}
Config::parse_inner(Flags::parse(&["check".to_owned()]), get_toml);
}

12
src/bootstrap/src/core/download.rs
View File

@ -9,6 +9,7 @@ use std::sync::OnceLock;
use build_helper::ci::CiEnv;
use xz2::bufread::XzDecoder;
use crate::core::config::BUILDER_CONFIG_FILENAME;
use crate::utils::exec::{command, BootstrapCommand};
use crate::utils::helpers::{check_run, exe, hex_encode, move_file, program_out_of_date};
use crate::{t, Config};
@ -273,11 +274,12 @@ impl Config {
let mut tar = tar::Archive::new(decompressor);
let is_ci_rustc = dst.ends_with("ci-rustc");
// `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.
// Cache the entries when we extract it so we only have to read it once.
let mut recorded_entries =
if dst.ends_with("ci-rustc") { recorded_entries(dst, pattern) } else { None };
let mut recorded_entries = if is_ci_rustc { recorded_entries(dst, pattern) } else { None };
for member in t!(tar.entries()) {
let mut member = t!(member);
@ -287,10 +289,12 @@ impl Config {
continue;
}
let mut short_path = t!(original_path.strip_prefix(directory_prefix));
if !short_path.starts_with(pattern) {
let is_builder_config = short_path.to_str() == Some(BUILDER_CONFIG_FILENAME);
if !(short_path.starts_with(pattern) || (is_ci_rustc && is_builder_config)) {
continue;
}
short_path = t!(short_path.strip_prefix(pattern));
short_path = short_path.strip_prefix(pattern).unwrap_or(short_path);
let dst_path = dst.join(short_path);
self.verbose(|| {
println!("extracting {} to {}", original_path.display(), dst.display())

3
src/bootstrap/src/utils/tarball.rs
View File

@ -9,6 +9,7 @@ use std::path::{Path, PathBuf};
use crate::core::build_steps::dist::distdir;
use crate::core::builder::{Builder, Kind};
use crate::core::config::BUILDER_CONFIG_FILENAME;
use crate::utils::exec::BootstrapCommand;
use crate::utils::helpers::{move_file, t};
use crate::utils::{channel, helpers};
@ -320,7 +321,7 @@ impl<'a> Tarball<'a> {
// Add config file if present.
if let Some(config) = &self.builder.config.config {
self.add_renamed_file(config, &self.overlay_dir, "builder-config");
self.add_renamed_file(config, &self.overlay_dir, BUILDER_CONFIG_FILENAME);
}
for file in self.overlay.legal_and_readme() {