From 502b630cd1042d8b2b613a12278a01c641cf096a Mon Sep 17 00:00:00 2001 From: Waffle Lapkin Date: Tue, 15 Apr 2025 13:11:14 +0200 Subject: [PATCH 1/5] tidy: don't crush on non-existent submodules --- src/tools/tidy/src/deps.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/tools/tidy/src/deps.rs b/src/tools/tidy/src/deps.rs index 88c2a02798ace..46e55859a5785 100644 --- a/src/tools/tidy/src/deps.rs +++ b/src/tools/tidy/src/deps.rs @@ -682,8 +682,10 @@ pub static CRATES: &[&str] = &[ pub fn has_missing_submodule(root: &Path, submodules: &[&str]) -> bool { !CiEnv::is_ci() && submodules.iter().any(|submodule| { + let path = root.join(submodule); + !path.exists() // If the directory is empty, we can consider it as an uninitialized submodule. - read_dir(root.join(submodule)).unwrap().next().is_none() + || read_dir(path).unwrap().next().is_none() }) } From 52f4b16075bf01d2c8f8539c26fb9b135967713f Mon Sep 17 00:00:00 2001 From: Waffle Lapkin Date: Tue, 15 Apr 2025 14:06:51 +0200 Subject: [PATCH 2/5] use helper function instead of writing rustfmt stamp by hand --- src/bootstrap/src/core/build_steps/format.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/build_steps/format.rs b/src/bootstrap/src/core/build_steps/format.rs index b1a97bde97b5b..43b831adf1f70 100644 --- a/src/bootstrap/src/core/build_steps/format.rs +++ b/src/bootstrap/src/core/build_steps/format.rs @@ -81,7 +81,8 @@ fn update_rustfmt_version(build: &Builder<'_>) { let Some((version, stamp_file)) = get_rustfmt_version(build) else { return; }; - t!(std::fs::write(stamp_file.path(), version)) + + t!(stamp_file.add_stamp(version).write()); } /// Returns the Rust files modified between the `merge-base` of HEAD and From dda4d7bc44aa4909aae72cd38965a11ccf46c02e Mon Sep 17 00:00:00 2001 From: Waffle Lapkin Date: Tue, 15 Apr 2025 14:06:51 +0200 Subject: [PATCH 3/5] slightly correct comments and diagnostics about checking modifications I feel like they are still wrong, but maybe less so .-. The `info:` was unhelpful -- we only use upstream in CI nowdays. --- src/bootstrap/src/core/build_steps/format.rs | 17 +++++++++-------- src/build_helper/src/git.rs | 6 ++++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/format.rs b/src/bootstrap/src/core/build_steps/format.rs index 43b831adf1f70..25d5f97d7eec5 100644 --- a/src/bootstrap/src/core/build_steps/format.rs +++ b/src/bootstrap/src/core/build_steps/format.rs @@ -85,11 +85,15 @@ fn update_rustfmt_version(build: &Builder<'_>) { t!(stamp_file.add_stamp(version).write()); } -/// Returns the Rust files modified between the `merge-base` of HEAD and -/// rust-lang/master and what is now on the disk. Does not include removed files. +/// Returns the Rust files modified between the last merge commit and what is now on the disk. +/// Does not include removed files. /// /// Returns `None` if all files should be formatted. fn get_modified_rs_files(build: &Builder<'_>) -> Result>, String> { + // In CI `get_git_modified_files` returns something different to normal environment. + // This shouldn't be called in CI anyway. + assert!(!build.config.is_running_on_ci); + if !verify_rustfmt_version(build) { return Ok(None); } @@ -104,7 +108,7 @@ struct RustfmtConfig { // Prints output describing a collection of paths, with lines such as "formatted modified file // foo/bar/baz" or "skipped 20 untracked files". -fn print_paths(build: &Builder<'_>, verb: &str, adjective: Option<&str>, paths: &[String]) { +fn print_paths(verb: &str, adjective: Option<&str>, paths: &[String]) { let len = paths.len(); let adjective = if let Some(adjective) = adjective { format!("{adjective} ") } else { String::new() }; @@ -115,9 +119,6 @@ fn print_paths(build: &Builder<'_>, verb: &str, adjective: Option<&str>, paths: } else { println!("fmt: {verb} {len} {adjective}files"); } - if len > 1000 && !build.config.is_running_on_ci { - println!("hint: if this number seems too high, try running `git fetch origin master`"); - } } pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) { @@ -190,7 +191,7 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) { ) .map(|x| x.to_string()) .collect(); - print_paths(build, "skipped", Some("untracked"), &untracked_paths); + print_paths("skipped", Some("untracked"), &untracked_paths); for untracked_path in untracked_paths { // The leading `/` makes it an exact match against the @@ -319,7 +320,7 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) { }); let mut paths = formatted_paths.into_inner().unwrap(); paths.sort(); - print_paths(build, if check { "checked" } else { "formatted" }, adjective, &paths); + print_paths(if check { "checked" } else { "formatted" }, adjective, &paths); drop(tx); diff --git a/src/build_helper/src/git.rs b/src/build_helper/src/git.rs index 693e0fc8f46d8..f5347c3082410 100644 --- a/src/build_helper/src/git.rs +++ b/src/build_helper/src/git.rs @@ -114,7 +114,9 @@ fn git_upstream_merge_base( Ok(output_result(git.arg("merge-base").arg(&updated_master).arg("HEAD"))?.trim().to_owned()) } -/// Searches for the nearest merge commit in the repository that also exists upstream. +/// Searches for the nearest merge commit in the repository. +/// +/// **In CI** finds the nearest merge commit that *also exists upstream*. /// /// It looks for the most recent commit made by the merge bot by matching the author's email /// address with the merge bot's email. @@ -165,7 +167,7 @@ pub fn get_closest_merge_commit( Ok(output_result(&mut git)?.trim().to_owned()) } -/// Returns the files that have been modified in the current branch compared to the master branch. +/// Returns the files that have been modified in the current branch compared to the last merge. /// The `extensions` parameter can be used to filter the files by their extension. /// Does not include removed files. /// If `extensions` is empty, all files will be returned. From 8934ac575988f2c15b7910d2f683b2792e5916cb Mon Sep 17 00:00:00 2001 From: Waffle Lapkin Date: Tue, 15 Apr 2025 14:06:51 +0200 Subject: [PATCH 4/5] add a comment for code that isn't --- src/bootstrap/src/core/build_steps/format.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/build_steps/format.rs b/src/bootstrap/src/core/build_steps/format.rs index 25d5f97d7eec5..6641c4b4e139b 100644 --- a/src/bootstrap/src/core/build_steps/format.rs +++ b/src/bootstrap/src/core/build_steps/format.rs @@ -214,7 +214,13 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) { override_builder.add(&format!("/{file}")).expect(&file); } } - Ok(None) => {} + Ok(None) => { + // NOTE: `Ok(None)` signifies that we need to format all files. + // The tricky part here is that if `override_builder` isn't given any white + // list files (i.e. files to be formatted, added without leading `!`), it + // will instead look for *all* files. So, by doing nothing here, we are + // actually making it so we format all files. + } Err(err) => { eprintln!("fmt warning: Something went wrong running git commands:"); eprintln!("fmt warning: {err}"); From 90aec1367481ab05c78fefad314e03b26e84564f Mon Sep 17 00:00:00 2001 From: Waffle Lapkin Date: Tue, 15 Apr 2025 14:06:51 +0200 Subject: [PATCH 5/5] commit rustfmt stump in `x t tidy` even on `check` If checking succeeded, it's equivalent to successfully formatting. --- src/bootstrap/src/core/build_steps/format.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/format.rs b/src/bootstrap/src/core/build_steps/format.rs index 6641c4b4e139b..9da8b27a91778 100644 --- a/src/bootstrap/src/core/build_steps/format.rs +++ b/src/bootstrap/src/core/build_steps/format.rs @@ -336,7 +336,10 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) { crate::exit!(1); } - if !check { - update_rustfmt_version(build); - } + // Update `build/.rustfmt-stamp`, allowing this code to ignore files which have not been changed + // since last merge. + // + // NOTE: Because of the exit above, this is only reachable if formatting / format checking + // succeeded. So we are not commiting the version if formatting was not good. + update_rustfmt_version(build); }