Skip to content

Commit

Permalink
Rollup merge of rust-lang#121787 - onur-ozkan:improve-change-tracker,…
Browse files Browse the repository at this point in the history
… 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
  • Loading branch information
matthiaskrgr authored Mar 17, 2024
2 parents 3ec2b7b + c36f493 commit 2448162
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 14 deletions.
12 changes: 3 additions & 9 deletions src/bootstrap/src/bin/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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!(
Expand Down
31 changes: 28 additions & 3 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
})
Expand Down Expand Up @@ -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,
Expand Down
19 changes: 18 additions & 1 deletion src/bootstrap/src/core/config/tests.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{flags::Flags, Config};
use super::{flags::Flags, ChangeIdWrapper, Config};
use crate::core::config::{LldMode, TomlConfig};

use clap::CommandFactory;
Expand Down Expand Up @@ -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));
}
4 changes: 3 additions & 1 deletion src/bootstrap/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions src/bootstrap/src/utils/change_tracker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down

0 comments on commit 2448162

Please sign in to comment.