From 701deca7f3b8ba65b33f18876f0153f8585c2388 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Thu, 29 Feb 2024 12:53:33 +0300 Subject: [PATCH] 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 #94829 is completed (scheduled for 2024), we will be able to handle this more effectively. Signed-off-by: onur-ozkan --- src/bootstrap/src/bin/main.rs | 12 +++------ src/bootstrap/src/core/config/config.rs | 32 ++++++++++++++++++++--- src/bootstrap/src/lib.rs | 4 ++- src/bootstrap/src/utils/change_tracker.rs | 14 ++++++++++ 4 files changed, 49 insertions(+), 13 deletions(-) diff --git a/src/bootstrap/src/bin/main.rs b/src/bootstrap/src/bin/main.rs index 070d951dba99..252523fc5de8 100644 --- 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 { } 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 index 875a4efae02f..07a5fa9e447f 100644 --- 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, // FIXME: Deprecated field. Remove it at 2024. - change_id: Option, + #[serde(flatten)] + change_id: ChangeIdWrapper, build: Option, install: Option, llvm: Option, @@ -610,6 +611,16 @@ pub(crate) struct TomlConfig { profile: Option, } +/// 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, +} + /// 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 index 965d788bb837..754f282d375d 100644 --- 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 index 9a50ad4437e7..8f237144871c 100644 --- 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 { .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),