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

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 rust-lang#122709

Author: bors

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

@@ -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(test)]
+    fn get_toml(_: &Path) -> Result<TomlConfig, toml::de::Error> {
+        Ok(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)
-                            );
-                        }
+    #[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)
+                        );
                     }
+                }
+            })
+    }
 
-                    eprintln!("failed to parse TOML configuration '{}': {err}", file.display());
-                    exit!(2);
-                })
-        }
-        Self::parse_inner(flags, get_toml)
+    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) -> TomlConfig) -> Config {
+    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,30 +2807,31 @@ 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.
     //    2. Checking an option that have no effect on the compiler behaviour/output.
     //
     // 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(())
 }

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

@@ -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);
 }