From 1dabb76f40a4186a68d98bb3d04a3b089608c656 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Tue, 18 Mar 2025 12:42:28 +0100 Subject: [PATCH] Explicitly model missing upstream It shouldn't really happen, but if it does, at least we will have an explicit record of it. --- src/bootstrap/src/core/build_steps/gcc.rs | 4 +++ src/bootstrap/src/core/config/config.rs | 6 +++- src/bootstrap/src/core/download.rs | 4 +++ src/build_helper/src/git.rs | 42 +++++++++++++++-------- src/build_helper/src/git/tests.rs | 32 +++++++++++++++-- 5 files changed, 70 insertions(+), 18 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/gcc.rs b/src/bootstrap/src/core/build_steps/gcc.rs index 867874fc7cc8..94abbb35ac5c 100644 --- a/src/bootstrap/src/core/build_steps/gcc.rs +++ b/src/bootstrap/src/core/build_steps/gcc.rs @@ -129,6 +129,10 @@ fn try_download_gcc(builder: &Builder<'_>, target: TargetSelection) -> Option { + eprintln!("No upstream commit found, GCC will *not* be downloaded"); + None + } } } diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 1406ebeb6216..a2d13cffbff2 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -3210,6 +3210,10 @@ impl Config { upstream } + PathFreshness::MissingUpstream => { + eprintln!("No upstream commit found"); + return None; + } } } else { channel::read_commit_info_file(&self.src) @@ -3289,7 +3293,7 @@ impl Config { pub fn has_changes_from_upstream(&self, paths: &[&str]) -> bool { match self.check_modifications(paths) { PathFreshness::LastModifiedUpstream { .. } => false, - PathFreshness::HasLocalModifications { .. } => true, + PathFreshness::HasLocalModifications { .. } | PathFreshness::MissingUpstream => true, } } diff --git a/src/bootstrap/src/core/download.rs b/src/bootstrap/src/core/download.rs index 3c9cfead5a9d..0051b8747296 100644 --- a/src/bootstrap/src/core/download.rs +++ b/src/bootstrap/src/core/download.rs @@ -734,6 +734,10 @@ download-rustc = false 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 stamp_key = format!("{}{}", llvm_sha, self.llvm_assertions); let llvm_stamp = BuildStamp::new(&llvm_root).with_prefix("llvm").add_stamp(stamp_key); diff --git a/src/build_helper/src/git.rs b/src/build_helper/src/git.rs index bc8dc3472d34..399505689871 100644 --- a/src/build_helper/src/git.rs +++ b/src/build_helper/src/git.rs @@ -113,6 +113,9 @@ pub enum PathFreshness { /// `upstream` is the latest upstream merge commit that made modifications to the /// set of paths. HasLocalModifications { upstream: String }, + /// No upstream commit was found. + /// This should not happen in most reasonable circumstances, but one never knows. + MissingUpstream, } /// This function figures out if a set of paths was last modified upstream or @@ -177,21 +180,29 @@ pub fn check_path_modifications( // artifacts. // Do not include HEAD, as it is never an upstream commit - get_closest_upstream_commit(git_dir, config, ci_env)? + // If we do not find an upstream commit in CI, something is seriously wrong. + Some( + get_closest_upstream_commit(git_dir, config, ci_env)? + .expect("No upstream commit was found on CI"), + ) } else { - // Outside CI, we have to find the most recent upstream commit that - // modified the set of paths, to have an upstream reference. - let upstream_sha = get_latest_commit_that_modified_files( + // Outside CI, we want to find the most recent upstream commit that + // modified the set of paths, to have an upstream reference that does not change + // unnecessarily often. + // However, if such commit is not found, we can fall back to the latest upstream commit + let upstream_with_modifications = get_latest_commit_that_modified_files( git_dir, target_paths, config.git_merge_commit_email, )?; - let Some(upstream_sha) = upstream_sha else { - eprintln!("No upstream commit that modified paths {target_paths:?} found."); - eprintln!("Try to fetch more upstream history."); - return Err("No upstream commit with modifications found".to_string()); - }; - upstream_sha + match upstream_with_modifications { + Some(sha) => Some(sha), + None => get_closest_upstream_commit(git_dir, config, ci_env)?, + } + }; + + let Some(upstream_sha) = upstream_sha else { + return Ok(PathFreshness::MissingUpstream); }; // For local environments, we want to find out if something has changed @@ -253,7 +264,7 @@ fn get_closest_upstream_commit( git_dir: Option<&Path>, config: &GitConfig<'_>, env: CiEnv, -) -> Result { +) -> Result, String> { let mut git = Command::new("git"); if let Some(git_dir) = git_dir { @@ -264,7 +275,7 @@ fn get_closest_upstream_commit( CiEnv::None => "HEAD", CiEnv::GitHubActions => { // On CI, we always have a merge commit at the tip. - // We thus skip it, because although it can be creatd by + // We thus skip it, because although it can be created by // `config.git_merge_commit_email`, it should not be upstream. "HEAD^1" } @@ -277,7 +288,8 @@ fn get_closest_upstream_commit( &base, ]); - Ok(output_result(&mut git)?.trim().to_owned()) + let output = output_result(&mut git)?.trim().to_owned(); + if output.is_empty() { Ok(None) } else { Ok(Some(output)) } } /// Returns the files that have been modified in the current branch compared to the master branch. @@ -291,7 +303,9 @@ pub fn get_git_modified_files( git_dir: Option<&Path>, extensions: &[&str], ) -> Result, String> { - let merge_base = get_closest_upstream_commit(git_dir, config, CiEnv::None)?; + let Some(merge_base) = get_closest_upstream_commit(git_dir, config, CiEnv::None)? else { + return Err("No upstream commit was found".to_string()); + }; let mut git = Command::new("git"); if let Some(git_dir) = git_dir { diff --git a/src/build_helper/src/git/tests.rs b/src/build_helper/src/git/tests.rs index cc502f08387f..c6ad9de5a75d 100644 --- a/src/build_helper/src/git/tests.rs +++ b/src/build_helper/src/git/tests.rs @@ -94,7 +94,7 @@ fn test_local_committed_modifications_subdirectory() { } #[test] -fn test_changes_in_head_upstream() { +fn test_local_changes_in_head_upstream() { git_test(|ctx| { // We want to resolve to the upstream commit that made modifications to a, // even if it is currently HEAD @@ -107,7 +107,7 @@ fn test_changes_in_head_upstream() { } #[test] -fn test_changes_in_previous_upstream() { +fn test_local_changes_in_previous_upstream() { git_test(|ctx| { // We want to resolve to this commit, which modified a let sha = ctx.create_upstream_merge(&["a", "e"]); @@ -124,7 +124,33 @@ fn test_changes_in_previous_upstream() { } #[test] -fn test_changes_negative_path() { +fn test_local_no_upstream_commit_with_changes() { + git_test(|ctx| { + ctx.create_upstream_merge(&["a", "e"]); + ctx.create_upstream_merge(&["a", "e"]); + // We want to fall back to this commit, because there are no commits + // that modified `x`. + let sha = ctx.create_upstream_merge(&["a", "e"]); + ctx.create_branch("feature"); + ctx.modify("d"); + ctx.commit(); + assert_eq!( + ctx.get_source(&["x"], CiEnv::None), + PathFreshness::LastModifiedUpstream { upstream: sha } + ); + }); +} + +#[test] +fn test_local_no_upstream_commit() { + git_test(|ctx| { + let src = ctx.get_source(&["c", "d"], CiEnv::None); + assert_eq!(src, PathFreshness::MissingUpstream); + }); +} + +#[test] +fn test_local_changes_negative_path() { git_test(|ctx| { let upstream = ctx.create_upstream_merge(&["a"]); ctx.create_branch("feature");