Auto merge of rust-lang#138591 - Kobzol:git-ci, r=<try>

Refactor git change detection in bootstrap

While working on rust-lang#138395, I finally found the courage to delve into the insides of git path change detection in bootstrap, which is used (amongst other things) to detect if we should rebuilt od download `[llvm|rustc|gcc]`. I found it a bit hard to understand, and given that this code was historically quite fragile, I thought that it would be better to rebuild it from scratch.

The previous approach had a bunch of limitations:
- It separated the computation of "are there local changes?" and "what upstream SHA should we use?" even though these two things are intertwined.
- It used hacks to work around what happens on CI.
- It had special cases for CI scattered throughout the codebase, rather than centralized in one place.
- It wasn't documented enough and didn't have tests for the git behavior.

The current approach should hopefully resolve all of that. I implemented a single entrypoint called `check_path_modifications` (naming bikeshed pending, half of the time I spend on this PR was thinking about names, as it's quite tricky here..) that explicitly receives a mode of operation (in CI or outside CI), and accordingly figures out that upstream SHA that we should use for downloading artifacts and it also figures out if there are any local changes. Users of this function can then use this unified output to implement `download-ci-X` and other functionality.

I also added a bunch of integration tests that literally spawn a git repository on disk and then check that the function can deal with various situations (PR CI, auto/try CI, local builds). The tests are super fast and run in parallel, as they are currently in `build_helper` and not in `bootstrap`.

After I built this inner layer, I used it for downloading GCC, LLVM and rustc. The latter two (and especially rustc) were using the `last_modified_commit` function before, but in all cases but one this function was actually only used to check if there are any local changes, which was IMO confusing. The LLVM handling would deserve a bit of refactoring, but that's a larger change that can be done as a follow-up.

In the future we could cache the results of `check_path_modifications` to reduce the number of git invocations, but I don't think that it should be excessive even now.

I hope that the implementation is now clear and easy to understand, so that in combination with the tests we can have more confidence that it does what we want. I tried to include a lot of documentation in the code, so I won't be repeating the actual implementation details here, if there are any questions, I'll add the answers to the documentation too :)

The new approach explicitly supports three scenarios:
- Running on PR CI, where we have one upstream bors parent commit and one PR merge commit made by GitHub.
- Running on try/auto CI, where we have one upstream bors parent commit and one PR merge commit made by bors.
- Running locally, where we assume that we have at least one upstream bors parent commit in our git history.

I removed the handling of upstreams on CI, as I think that it shouldn't be needed and I considered it to be a hack. However, it's possible that there are other use-cases that I haven't considered, so I want to ask around if people have other situations than the three use-cases described above. If there are other such use-cases, I would like to include them in the new centralized implementation and add them to the git test suite, rather than going back to the old ways :)

In particular, the code before relied on `git merge-base`, but I don't see why we can't just lookup the most recent bors commit and assume that is a merge commit that is also upstream? I might be running into Chesterton's Fence here :)

CC `@pietroalbini` To make sure that this won't break downstream users of Rust's CI.

Best reviewed commit by commit.

Companion PRs:
- For testing beta: rust-lang#138597

r? `@onur-ozkan`

try-job: x86_64-gnu-aux

Author: bors

.github/workflows/ci.yml

@@ -116,9 +116,6 @@ jobs:
           # which then uses log commands to actually set them.
           EXTRA_VARIABLES: ${{ toJson(matrix.env) }}
 
-      - name: setup upstream remote
-        run: src/ci/scripts/setup-upstream-remote.sh
-
       - name: ensure the channel matches the target branch
         run: src/ci/scripts/verify-channel.sh
 

Cargo.lock

@@ -304,6 +304,7 @@ version = "0.1.0"
 dependencies = [
  "serde",
  "serde_derive",
+ "tempfile",
 ]
 
 [[package]]
@@ -2076,6 +2077,12 @@ version = "0.4.15"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "d26c52dbd32dccf2d10cac7725f8eae5296885fb5703b261f7d0a0739ec807ab"
 
+[[package]]
+name = "linux-raw-sys"
+version = "0.9.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "fe7db12097d22ec582439daf8618b8fdd1a7bef6270e9af3b1ebcd30893cf413"
+
 [[package]]
 name = "litemap"
 version = "0.7.4"
@@ -4711,7 +4718,20 @@ dependencies = [
  "bitflags",
  "errno",
  "libc",
- "linux-raw-sys",
+ "linux-raw-sys 0.4.15",
+ "windows-sys 0.59.0",
+]
+
+[[package]]
+name = "rustix"
+version = "1.0.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f7178faa4b75a30e269c71e61c353ce2748cf3d76f0c44c393f4e60abf49b825"
+dependencies = [
+ "bitflags",
+ "errno",
+ "libc",
+ "linux-raw-sys 0.9.3",
  "windows-sys 0.59.0",
 ]
 
@@ -5118,15 +5138,14 @@ dependencies = [
 
 [[package]]
 name = "tempfile"
-version = "3.15.0"
+version = "3.19.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9a8a559c81686f576e8cd0290cd2a24a2a9ad80c98b3478856500fcbd7acd704"
+checksum = "488960f40a3fd53d72c2a29a58722561dee8afdd175bd88e3db4677d7b2ba600"
 dependencies = [
- "cfg-if",
  "fastrand",
- "getrandom 0.2.15",
+ "getrandom 0.3.1",
  "once_cell",
- "rustix",
+ "rustix 1.0.2",
  "windows-sys 0.59.0",
 ]
 
@@ -6454,8 +6473,8 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "e105d177a3871454f754b33bb0ee637ecaaac997446375fd3e5d43a2ed00c909"
 dependencies = [
  "libc",
- "linux-raw-sys",
- "rustix",
+ "linux-raw-sys 0.4.15",
+ "rustix 0.38.43",
 ]
 
 [[package]]

src/bootstrap/Cargo.lock

@@ -225,12 +225,12 @@ dependencies = [
 
 [[package]]
 name = "errno"
-version = "0.3.9"
+version = "0.3.10"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "534c5cf6194dfab3db3242765c03bbe257cf92f22b38f6bc0c58d59108a820ba"
+checksum = "33d852cb9b869c2a9b3df2f71a3074817f01e1844f839a144f5fcef059a4eb5d"
 dependencies = [
  "libc",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -334,9 +334,9 @@ checksum = "bbd2bcb4c963f2ddae06a2efc7e9f3591312473c50c6685e1f298068316e66fe"
 
 [[package]]
 name = "libc"
-version = "0.2.167"
+version = "0.2.171"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "09d6582e104315a817dff97f75133544b2e094ee22447d2acf4a74e189ba06fc"
+checksum = "c19937216e9d3aa9956d9bb8dfc0b0c8beb6058fc4f7a4dc4d850edf86a237d6"
 
 [[package]]
 name = "libredox"

src/bootstrap/src/core/build_steps/compile.rs

@@ -111,14 +111,10 @@ impl Step for Std {
         // the `rust.download-rustc=true` option.
         let force_recompile = builder.rust_info().is_managed_git_subrepository()
             && builder.download_rustc()
-            && builder.config.last_modified_commit(&["library"], "download-rustc", true).is_none();
+            && builder.config.has_changes_from_upstream(&["library"]);
 
         trace!("is managed git repo: {}", builder.rust_info().is_managed_git_subrepository());
         trace!("download_rustc: {}", builder.download_rustc());
-        trace!(
-            "last modified commit: {:?}",
-            builder.config.last_modified_commit(&["library"], "download-rustc", true)
-        );
         trace!(force_recompile);
 
         run.builder.ensure(Std {

src/bootstrap/src/core/build_steps/format.rs

@@ -94,7 +94,7 @@ fn get_modified_rs_files(build: &Builder<'_>) -> Result<Option<Vec<String>>, Str
         return Ok(None);
     }
 
-    get_git_modified_files(&build.config.git_config(), Some(&build.config.src), &["rs"])
+    get_git_modified_files(&build.config.git_config(), Some(&build.config.src), &["rs"]).map(Some)
 }
 
 #[derive(serde_derive::Deserialize)]

src/bootstrap/src/core/build_steps/gcc.rs

@@ -98,6 +98,8 @@ pub enum GccBuildStatus {
 /// Returns a path to the libgccjit.so file.
 #[cfg(not(test))]
 fn try_download_gcc(builder: &Builder<'_>, target: TargetSelection) -> Option<PathBuf> {
+    use build_helper::git::PathFreshness;
+
     // Try to download GCC from CI if configured and available
     if !matches!(builder.config.gcc_ci_mode, crate::core::config::GccCiMode::DownloadFromCi) {
         return None;
@@ -106,18 +108,30 @@ fn try_download_gcc(builder: &Builder<'_>, target: TargetSelection) -> Option<Pa
         eprintln!("GCC CI download is only available for the `x86_64-unknown-linux-gnu` target");
         return None;
     }
-    let sha =
-        detect_gcc_sha(&builder.config, builder.config.rust_info.is_managed_git_subrepository());
-    let root = ci_gcc_root(&builder.config);
-    let gcc_stamp = BuildStamp::new(&root).with_prefix("gcc").add_stamp(&sha);
-    if !gcc_stamp.is_up_to_date() && !builder.config.dry_run() {
-        builder.config.download_ci_gcc(&sha, &root);
-        t!(gcc_stamp.write());
+    let source = detect_gcc_freshness(
+        &builder.config,
+        builder.config.rust_info.is_managed_git_subrepository(),
+    );
+    match source {
+        PathFreshness::LastModifiedUpstream { upstream } => {
+            // Download from upstream CI
+            let root = ci_gcc_root(&builder.config);
+            let gcc_stamp = BuildStamp::new(&root).with_prefix("gcc").add_stamp(&upstream);
+            if !gcc_stamp.is_up_to_date() && !builder.config.dry_run() {
+                builder.config.download_ci_gcc(&upstream, &root);
+                t!(gcc_stamp.write());
+            }
+
+            let libgccjit = root.join("lib").join("libgccjit.so");
+            create_lib_alias(builder, &libgccjit);
+            Some(libgccjit)
+        }
+        PathFreshness::HasLocalModifications { .. } => {
+            // We have local modifications, rebuild GCC.
+            eprintln!("Found local GCC modifications, GCC will *not* be downloaded");
+            None
+        }
     }
-
-    let libgccjit = root.join("lib").join("libgccjit.so");
-    create_lib_alias(builder, &libgccjit);
-    Some(libgccjit)
 }
 
 #[cfg(test)]
@@ -266,31 +280,34 @@ fn ci_gcc_root(config: &crate::Config) -> PathBuf {
     config.out.join(config.build).join("ci-gcc")
 }
 
-/// This retrieves the GCC sha we *want* to use, according to git history.
+/// Detect whether GCC sources have been modified locally or not.
 #[cfg(not(test))]
-fn detect_gcc_sha(config: &crate::Config, is_git: bool) -> String {
-    use build_helper::git::get_closest_merge_commit;
-
-    let gcc_sha = if is_git {
-        get_closest_merge_commit(
-            Some(&config.src),
-            &config.git_config(),
-            &[config.src.join("src/gcc"), config.src.join("src/bootstrap/download-ci-gcc-stamp")],
+fn detect_gcc_freshness(config: &crate::Config, is_git: bool) -> build_helper::git::PathFreshness {
+    use build_helper::git::{PathFreshness, check_path_modifications};
+
+    let freshness = if is_git {
+        Some(
+            check_path_modifications(
+                Some(&config.src),
+                &config.git_config(),
+                &["src/gcc", "src/bootstrap/download-ci-gcc-stamp"],
+                CiEnv::current(),
+            )
+            .unwrap(),
         )
-        .unwrap()
     } else if let Some(info) = crate::utils::channel::read_commit_info_file(&config.src) {
-        info.sha.trim().to_owned()
+        Some(PathFreshness::LastModifiedUpstream { upstream: info.sha.trim().to_owned() })
     } else {
-        "".to_owned()
+        None
     };
 
-    if gcc_sha.is_empty() {
+    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!();
-    }
+    };
 
-    gcc_sha
+    freshness
 }

src/bootstrap/src/core/build_steps/llvm.rs

@@ -15,7 +15,7 @@ use std::sync::OnceLock;
 use std::{env, fs};
 
 use build_helper::ci::CiEnv;
-use build_helper::git::get_closest_merge_commit;
+use build_helper::git::{PathFreshness, check_path_modifications};
 #[cfg(feature = "tracing")]
 use tracing::instrument;
 
@@ -174,35 +174,38 @@ pub fn prebuilt_llvm_config(
     LlvmBuildStatus::ShouldBuild(Meta { stamp, res, out_dir, root: root.into() })
 }
 
-/// This retrieves the LLVM sha we *want* to use, according to git history.
-pub(crate) fn detect_llvm_sha(config: &Config, is_git: bool) -> String {
-    let llvm_sha = if is_git {
-        get_closest_merge_commit(
-            Some(&config.src),
-            &config.git_config(),
-            &[
-                config.src.join("src/llvm-project"),
-                config.src.join("src/bootstrap/download-ci-llvm-stamp"),
-                // the LLVM shared object file is named `LLVM-12-rust-{version}-nightly`
-                config.src.join("src/version"),
-            ],
+/// 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(),
+                &[
+                    "src/llvm-project",
+                    "src/bootstrap/download-ci-llvm-stamp",
+                    // the LLVM shared object file is named `LLVM-12-rust-{version}-nightly`
+                    "src/version",
+                ],
+                CiEnv::current(),
+            )
+            .unwrap(),
         )
-        .unwrap()
     } else if let Some(info) = crate::utils::channel::read_commit_info_file(&config.src) {
-        info.sha.trim().to_owned()
+        Some(PathFreshness::LastModifiedUpstream { upstream: info.sha.trim().to_owned() })
     } else {
-        "".to_owned()
+        None
     };
 
-    if llvm_sha.is_empty() {
+    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!();
-    }
+    };
 
-    llvm_sha
+    freshness
 }
 
 /// Returns whether the CI-found LLVM is currently usable.
@@ -282,12 +285,7 @@ pub(crate) fn is_ci_llvm_modified(config: &Config) -> bool {
         return false;
     }
 
-    let llvm_sha = detect_llvm_sha(config, true);
-    let head_sha = crate::output(
-        helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD").as_command_mut(),
-    );
-    let head_sha = head_sha.trim();
-    llvm_sha == head_sha
+    matches!(detect_llvm_freshness(config, true), PathFreshness::HasLocalModifications { .. })
 }
 
 #[derive(Debug, Clone, Hash, PartialEq, Eq)]

src/bootstrap/src/core/build_steps/tool.rs

@@ -687,8 +687,7 @@ impl Step for Rustdoc {
             let files_to_track = &["src/librustdoc", "src/tools/rustdoc"];
 
             // Check if unchanged
-            if builder.config.last_modified_commit(files_to_track, "download-rustc", true).is_some()
-            {
+            if !builder.config.has_changes_from_upstream(files_to_track) {
                 let precompiled_rustdoc = builder
                     .config
                     .ci_rustc_dir()

src/bootstrap/src/core/builder/tests.rs

@@ -268,7 +268,7 @@ fn ci_rustc_if_unchanged_logic() {
             paths.push("library");
         }
 
-        let has_changes = config.last_modified_commit(&paths, "download-rustc", true).is_none();
+        let has_changes = config.has_changes_from_upstream(&paths);
 
         assert!(
             !has_changes,