Rollup merge of rust-lang#121787 - onur-ozkan:improve-change-tracker, r=albertlarsan68

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.

Fixes rust-lang#121756

Author: matthiaskrgr

src/bootstrap/src/bin/main.rs

@@ -15,7 +15,8 @@ use std::{
 };
 
 use bootstrap::{
-    find_recent_config_change_ids, t, Build, Config, Subcommand, CONFIG_CHANGE_HISTORY,
+    find_recent_config_change_ids, human_readable_changes, t, Build, Config, Subcommand,
+    CONFIG_CHANGE_HISTORY,
 };
 
 fn main() {
@@ -164,14 +165,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(&human_readable_changes(&changes));
 
         msg.push_str("NOTE: to silence this warning, ");
         msg.push_str(&format!(

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

@@ -606,7 +606,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>,
@@ -616,6 +617,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")]
+    pub(crate) inner: Option<usize>,
+}
+
 /// Describes how to handle conflicts in merging two [`TomlConfig`]
 #[derive(Copy, Clone, Debug)]
 enum ReplaceOpt {
@@ -657,7 +668,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);
@@ -1210,6 +1221,20 @@ 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() {
+                            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);
                 })
@@ -1376,7 +1401,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,

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

@@ -1,4 +1,4 @@
-use super::{flags::Flags, Config};
+use super::{flags::Flags, ChangeIdWrapper, Config};
 use crate::core::config::{LldMode, TomlConfig};
 
 use clap::CommandFactory;
@@ -237,3 +237,20 @@ fn rust_lld() {
     assert!(matches!(parse("rust.use-lld = true").lld_mode, LldMode::External));
     assert!(matches!(parse("rust.use-lld = false").lld_mode, LldMode::Unused));
 }
+
+#[test]
+#[should_panic]
+fn parse_config_with_unknown_field() {
+    parse("unknown-key = 1");
+}
+
+#[test]
+fn parse_change_id_with_unknown_field() {
+    let config = r#"
+        change-id = 3461
+        unknown-key = 1
+    "#;
+
+    let change_id_wrapper: ChangeIdWrapper = toml::from_str(config).unwrap();
+    assert_eq!(change_id_wrapper.inner, Some(3461));
+}

src/bootstrap/src/lib.rs

@@ -50,7 +50,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, human_readable_changes, CONFIG_CHANGE_HISTORY,
+};
 
 const LLVM_TOOLS: &[&str] = &[
     "llvm-cov",      // used to generate coverage report

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 human_readable_changes(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),