From 1f5320e57bae9d2fd4cfa5bbad0c59651e356ee7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 21 Mar 2025 22:27:13 +0100 Subject: [PATCH] Unify usages of path modifications and log them in verbose mode --- src/bootstrap/src/core/build_steps/gcc.rs | 29 +++++++-------------- src/bootstrap/src/core/build_steps/llvm.rs | 26 +++++-------------- src/bootstrap/src/core/config/config.rs | 11 +++++--- src/bootstrap/src/core/config/tests.rs | 2 +- src/bootstrap/src/core/download.rs | 22 +++++++++------- src/build_helper/src/git/tests.rs | 30 +++++++++++----------- 6 files changed, 52 insertions(+), 68 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/gcc.rs b/src/bootstrap/src/core/build_steps/gcc.rs index 94abbb35ac5c..339eec184069 100644 --- a/src/bootstrap/src/core/build_steps/gcc.rs +++ b/src/bootstrap/src/core/build_steps/gcc.rs @@ -110,6 +110,9 @@ fn try_download_gcc(builder: &Builder<'_>, target: TargetSelection) -> Option { // Download from upstream CI @@ -285,31 +288,17 @@ fn ci_gcc_root(config: &crate::Config) -> PathBuf { /// Detect whether GCC sources have been modified locally or not. #[cfg(not(test))] fn detect_gcc_freshness(config: &crate::Config, is_git: bool) -> build_helper::git::PathFreshness { - use build_helper::git::{PathFreshness, check_path_modifications}; + use build_helper::git::PathFreshness; - let freshness = if is_git { - Some( - check_path_modifications( - Some(&config.src), - &config.git_config(), - &["src/gcc", "src/bootstrap/download-ci-gcc-stamp"], - config.ci_env(), - ) - .unwrap(), - ) + if is_git { + config.check_path_modifications(&["src/gcc", "src/bootstrap/download-ci-gcc-stamp"]) } else if let Some(info) = crate::utils::channel::read_commit_info_file(&config.src) { - Some(PathFreshness::LastModifiedUpstream { upstream: info.sha.trim().to_owned() }) + PathFreshness::LastModifiedUpstream { upstream: info.sha.trim().to_owned() } } else { - None - }; - - let Some(freshness) = freshness else { eprintln!("error: could not find commit hash for downloading GCC"); eprintln!("HELP: maybe your repository history is too shallow?"); eprintln!("HELP: consider disabling `download-ci-gcc`"); eprintln!("HELP: or fetch enough history to include one upstream commit"); - panic!(); - }; - - freshness + crate::exit!(1); + } } diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index 6e695ee1e7c9..1a6247c42d70 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -14,7 +14,7 @@ use std::path::{Path, PathBuf}; use std::sync::OnceLock; use std::{env, fs}; -use build_helper::git::{PathFreshness, check_path_modifications}; +use build_helper::git::PathFreshness; #[cfg(feature = "tracing")] use tracing::instrument; @@ -183,31 +183,17 @@ pub const LLVM_INVALIDATION_PATHS: &[&str] = &[ /// Detect whether LLVM sources have been modified locally or not. pub(crate) fn detect_llvm_freshness(config: &Config, is_git: bool) -> PathFreshness { - let freshness = if is_git { - Some( - check_path_modifications( - Some(&config.src), - &config.git_config(), - LLVM_INVALIDATION_PATHS, - config.ci_env(), - ) - .unwrap(), - ) + if is_git { + config.check_path_modifications(LLVM_INVALIDATION_PATHS) } else if let Some(info) = crate::utils::channel::read_commit_info_file(&config.src) { - Some(PathFreshness::LastModifiedUpstream { upstream: info.sha.trim().to_owned() }) + PathFreshness::LastModifiedUpstream { upstream: info.sha.trim().to_owned() } } else { - None - }; - - let Some(freshness) = freshness else { eprintln!("error: could not find commit hash for downloading LLVM"); eprintln!("HELP: maybe your repository history is too shallow?"); eprintln!("HELP: consider disabling `download-ci-llvm`"); eprintln!("HELP: or fetch enough history to include one upstream commit"); - panic!(); - }; - - freshness + crate::exit!(1); + } } /// Returns whether the CI-found LLVM is currently usable. diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index a2d13cffbff2..be9afba454e8 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -3193,7 +3193,11 @@ impl Config { let commit = if self.rust_info.is_managed_git_subrepository() { // Look for a version to compare to based on the current commit. // Only commits merged by bors will have CI artifacts. - match self.check_modifications(&allowed_paths) { + let freshness = self.check_path_modifications(&allowed_paths); + self.verbose(|| { + eprintln!("rustc freshness: {freshness:?}"); + }); + match freshness { PathFreshness::LastModifiedUpstream { upstream } => upstream, PathFreshness::HasLocalModifications { upstream } => { if if_unchanged { @@ -3291,13 +3295,14 @@ impl Config { /// Returns true if any of the `paths` have been modified locally. pub fn has_changes_from_upstream(&self, paths: &[&str]) -> bool { - match self.check_modifications(paths) { + match self.check_path_modifications(paths) { PathFreshness::LastModifiedUpstream { .. } => false, PathFreshness::HasLocalModifications { .. } | PathFreshness::MissingUpstream => true, } } - fn check_modifications(&self, paths: &[&str]) -> PathFreshness { + /// Checks whether any of the given paths have been modified w.r.t. upstream. + pub fn check_path_modifications(&self, paths: &[&str]) -> PathFreshness { check_path_modifications(Some(&self.src), &self.git_config(), paths, CiEnv::current()) .unwrap() } diff --git a/src/bootstrap/src/core/config/tests.rs b/src/bootstrap/src/core/config/tests.rs index ee345595ce3d..affabd497203 100644 --- a/src/bootstrap/src/core/config/tests.rs +++ b/src/bootstrap/src/core/config/tests.rs @@ -51,7 +51,7 @@ fn download_ci_llvm() { let if_unchanged_config = parse("llvm.download-ci-llvm = \"if-unchanged\""); if if_unchanged_config.llvm_from_ci && if_unchanged_config.is_running_on_ci { - let has_changes = if_unchanged_config.has_changes_from_upstream(&["src/llvm-project"]); + let has_changes = if_unchanged_config.has_changes_from_upstream(LLVM_INVALIDATION_PATHS); assert!( !has_changes, diff --git a/src/bootstrap/src/core/download.rs b/src/bootstrap/src/core/download.rs index 0051b8747296..57a674c1dd7e 100644 --- a/src/bootstrap/src/core/download.rs +++ b/src/bootstrap/src/core/download.rs @@ -730,15 +730,19 @@ download-rustc = false } let llvm_root = self.ci_llvm_root(); - let llvm_sha = - match detect_llvm_freshness(self, self.rust_info.is_managed_git_subrepository()) { - PathFreshness::LastModifiedUpstream { upstream } => upstream, - PathFreshness::HasLocalModifications { upstream } => upstream, - PathFreshness::MissingUpstream => { - eprintln!("No upstream commit for downloading LLVM found"); - crate::exit!(1); - } - }; + let llvm_freshness = + detect_llvm_freshness(self, self.rust_info.is_managed_git_subrepository()); + self.verbose(|| { + eprintln!("LLVM freshness: {llvm_freshness:?}"); + }); + let llvm_sha = match llvm_freshness { + PathFreshness::LastModifiedUpstream { upstream } => upstream, + PathFreshness::HasLocalModifications { upstream } => upstream, + PathFreshness::MissingUpstream => { + eprintln!("No upstream commit for downloading LLVM found"); + crate::exit!(1); + } + }; let stamp_key = format!("{}{}", llvm_sha, self.llvm_assertions); let llvm_stamp = BuildStamp::new(&llvm_root).with_prefix("llvm").add_stamp(stamp_key); if !llvm_stamp.is_up_to_date() && !self.dry_run() { diff --git a/src/build_helper/src/git/tests.rs b/src/build_helper/src/git/tests.rs index c6ad9de5a75d..43bffb8befa8 100644 --- a/src/build_helper/src/git/tests.rs +++ b/src/build_helper/src/git/tests.rs @@ -10,7 +10,7 @@ fn test_pr_ci_unchanged_anywhere() { git_test(|ctx| { let sha = ctx.create_upstream_merge(&["a"]); ctx.create_nonupstream_merge(&["b"]); - let src = ctx.get_source(&["c"], CiEnv::GitHubActions); + let src = ctx.check_modifications(&["c"], CiEnv::GitHubActions); assert_eq!(src, PathFreshness::LastModifiedUpstream { upstream: sha }); }); } @@ -20,7 +20,7 @@ fn test_pr_ci_changed_in_pr() { git_test(|ctx| { let sha = ctx.create_upstream_merge(&["a"]); ctx.create_nonupstream_merge(&["b"]); - let src = ctx.get_source(&["b"], CiEnv::GitHubActions); + let src = ctx.check_modifications(&["b"], CiEnv::GitHubActions); assert_eq!(src, PathFreshness::HasLocalModifications { upstream: sha }); }); } @@ -30,7 +30,7 @@ fn test_auto_ci_unchanged_anywhere_select_parent() { git_test(|ctx| { let sha = ctx.create_upstream_merge(&["a"]); ctx.create_upstream_merge(&["b"]); - let src = ctx.get_source(&["c"], CiEnv::GitHubActions); + let src = ctx.check_modifications(&["c"], CiEnv::GitHubActions); assert_eq!(src, PathFreshness::LastModifiedUpstream { upstream: sha }); }); } @@ -40,7 +40,7 @@ fn test_auto_ci_changed_in_pr() { git_test(|ctx| { let sha = ctx.create_upstream_merge(&["a"]); ctx.create_upstream_merge(&["b", "c"]); - let src = ctx.get_source(&["c", "d"], CiEnv::GitHubActions); + let src = ctx.check_modifications(&["c", "d"], CiEnv::GitHubActions); assert_eq!(src, PathFreshness::HasLocalModifications { upstream: sha }); }); } @@ -53,7 +53,7 @@ fn test_local_uncommitted_modifications() { ctx.modify("a"); assert_eq!( - ctx.get_source(&["a", "d"], CiEnv::None), + ctx.check_modifications(&["a", "d"], CiEnv::None), PathFreshness::HasLocalModifications { upstream: sha } ); }); @@ -71,7 +71,7 @@ fn test_local_committed_modifications() { ctx.commit(); assert_eq!( - ctx.get_source(&["a", "d"], CiEnv::None), + ctx.check_modifications(&["a", "d"], CiEnv::None), PathFreshness::HasLocalModifications { upstream: sha } ); }); @@ -87,7 +87,7 @@ fn test_local_committed_modifications_subdirectory() { ctx.commit(); assert_eq!( - ctx.get_source(&["a/b"], CiEnv::None), + ctx.check_modifications(&["a/b"], CiEnv::None), PathFreshness::HasLocalModifications { upstream: sha } ); }); @@ -100,7 +100,7 @@ fn test_local_changes_in_head_upstream() { // even if it is currently HEAD let sha = ctx.create_upstream_merge(&["a"]); assert_eq!( - ctx.get_source(&["a", "d"], CiEnv::None), + ctx.check_modifications(&["a", "d"], CiEnv::None), PathFreshness::LastModifiedUpstream { upstream: sha } ); }); @@ -117,7 +117,7 @@ fn test_local_changes_in_previous_upstream() { ctx.modify("d"); ctx.commit(); assert_eq!( - ctx.get_source(&["a"], CiEnv::None), + ctx.check_modifications(&["a"], CiEnv::None), PathFreshness::LastModifiedUpstream { upstream: sha } ); }); @@ -135,7 +135,7 @@ fn test_local_no_upstream_commit_with_changes() { ctx.modify("d"); ctx.commit(); assert_eq!( - ctx.get_source(&["x"], CiEnv::None), + ctx.check_modifications(&["x"], CiEnv::None), PathFreshness::LastModifiedUpstream { upstream: sha } ); }); @@ -144,7 +144,7 @@ fn test_local_no_upstream_commit_with_changes() { #[test] fn test_local_no_upstream_commit() { git_test(|ctx| { - let src = ctx.get_source(&["c", "d"], CiEnv::None); + let src = ctx.check_modifications(&["c", "d"], CiEnv::None); assert_eq!(src, PathFreshness::MissingUpstream); }); } @@ -159,15 +159,15 @@ fn test_local_changes_negative_path() { ctx.commit(); assert_eq!( - ctx.get_source(&[":!b", ":!d"], CiEnv::None), + ctx.check_modifications(&[":!b", ":!d"], CiEnv::None), PathFreshness::LastModifiedUpstream { upstream: upstream.clone() } ); assert_eq!( - ctx.get_source(&[":!c"], CiEnv::None), + ctx.check_modifications(&[":!c"], CiEnv::None), PathFreshness::HasLocalModifications { upstream: upstream.clone() } ); assert_eq!( - ctx.get_source(&[":!d", ":!x"], CiEnv::None), + ctx.check_modifications(&[":!d", ":!x"], CiEnv::None), PathFreshness::HasLocalModifications { upstream } ); }); @@ -198,7 +198,7 @@ impl GitCtx { ctx } - fn get_source(&self, target_paths: &[&str], ci_env: CiEnv) -> PathFreshness { + fn check_modifications(&self, target_paths: &[&str], ci_env: CiEnv) -> PathFreshness { check_path_modifications(Some(self.dir.path()), &self.git_config(), target_paths, ci_env) .unwrap() }