run change tracker even when config parse fails

Please note that we are currently validating the build configuration on two entry points (e.g., profile validation is handled on the python side), and change-tracker system is handled on the rust side. Once rust-lang#94829 is completed (scheduled for 2024), we will be able to handle this more effectively. Signed-off-by: onur-ozkan <[email protected]>
onur-ozkan · Feb 29, 2024 · 701deca · 701deca
1 parent b6e4299
commit 701deca
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 13 deletions.
diff --git a/src/bootstrap/src/bin/main.rs b/src/bootstrap/src/bin/main.rs
@@ -14,7 +14,8 @@ use std::{
 };
 
 use bootstrap::{
-    find_recent_config_change_ids, t, Build, Config, Subcommand, CONFIG_CHANGE_HISTORY,
+    find_recent_config_change_ids, into_human_readable_string, t, Build, Config, Subcommand,
+    CONFIG_CHANGE_HISTORY,
 };
 
 fn main() {
@@ -154,14 +155,7 @@ fn check_version(config: &Config) -> Option<String> {
         }
 
         msg.push_str("There have been changes to x.py since you last updated:\n");
-
-        for change in changes {
-            msg.push_str(&format!("  [{}] {}\n", change.severity, change.summary));
-            msg.push_str(&format!(
-                "    - PR Link https://github.com/rust-lang/rust/pull/{}\n",
-                change.change_id
-            ));
-        }
+        msg.push_str(&into_human_readable_string(&changes));
 
         msg.push_str("NOTE: to silence this warning, ");
         msg.push_str(&format!(

diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs
@@ -600,7 +600,8 @@ impl Target {
 #[serde(deny_unknown_fields, rename_all = "kebab-case")]
 pub(crate) struct TomlConfig {
     changelog_seen: Option<usize>, // FIXME: Deprecated field. Remove it at 2024.
-    change_id: Option<usize>,
+    #[serde(flatten)]
+    change_id: ChangeIdWrapper,
     build: Option<Build>,
     install: Option<Install>,
     llvm: Option<Llvm>,
@@ -610,6 +611,16 @@ pub(crate) struct TomlConfig {
     profile: Option<String>,
 }
 
+/// Since we use `#[serde(deny_unknown_fields)]` on `TomlConfig`, we need a wrapper type
+/// for the "change-id" field to parse it even if other fields are invalid. This ensures
+/// that if deserialization fails due to other fields, we can still provide the changelogs
+/// to allow developers to potentially find the reason for the failure in the logs..
+#[derive(Deserialize, Default)]
+pub(crate) struct ChangeIdWrapper {
+    #[serde(alias = "change-id")]
+    inner: Option<usize>,
+}
+
 /// Describes how to handle conflicts in merging two [`TomlConfig`]
 #[derive(Copy, Clone, Debug)]
 enum ReplaceOpt {
@@ -651,7 +662,7 @@ impl Merge for TomlConfig {
             }
         }
         self.changelog_seen.merge(changelog_seen, replace);
-        self.change_id.merge(change_id, replace);
+        self.change_id.inner.merge(change_id.inner, replace);
         do_merge(&mut self.build, build, replace);
         do_merge(&mut self.install, install, replace);
         do_merge(&mut self.llvm, llvm, replace);
@@ -1200,6 +1211,21 @@ impl Config {
             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))
+                        .and_then(|change_id| {
+                            Ok(change_id.inner.map(|id| crate::find_recent_config_change_ids(id)))
+                        })
+                    {
+                        if !changes.is_empty() {
+                            let message = format!(
+                                "WARNING: There have been changes to x.py since you last updated:\n{}",
+                                crate::into_human_readable_string(&changes)
+                            );
+                            println!("{}", message);
+                        }
+                    }
+
                     eprintln!("failed to parse TOML configuration '{}': {err}", file.display());
                     exit!(2);
                 })
@@ -1366,7 +1392,7 @@ impl Config {
         toml.merge(override_toml, ReplaceOpt::Override);
 
         config.changelog_seen = toml.changelog_seen;
-        config.change_id = toml.change_id;
+        config.change_id = toml.change_id.inner;
 
         let Build {
             build,

diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs
@@ -51,7 +51,9 @@ mod utils;
 pub use core::builder::PathSet;
 pub use core::config::flags::Subcommand;
 pub use core::config::Config;
-pub use utils::change_tracker::{find_recent_config_change_ids, CONFIG_CHANGE_HISTORY};
+pub use utils::change_tracker::{
+    find_recent_config_change_ids, into_human_readable_string, CONFIG_CHANGE_HISTORY,
+};
 
 const LLVM_TOOLS: &[&str] = &[
     "llvm-cov",      // used to generate coverage report

diff --git a/src/bootstrap/src/utils/change_tracker.rs b/src/bootstrap/src/utils/change_tracker.rs
@@ -60,6 +60,20 @@ pub fn find_recent_config_change_ids(current_id: usize) -> Vec<ChangeInfo> {
         .collect()
 }
 
+pub fn into_human_readable_string(changes: &[ChangeInfo]) -> String {
+    let mut message = String::new();
+
+    for change in changes {
+        message.push_str(&format!("  [{}] {}\n", change.severity, change.summary));
+        message.push_str(&format!(
+            "    - PR Link https://github.com/rust-lang/rust/pull/{}\n",
+            change.change_id
+        ));
+    }
+
+    message
+}
+
 /// Keeps track of major changes made to the bootstrap configuration.
 ///
 /// If you make any major changes (such as adding new values or changing default values),