Merge commits break LLVM CI download

[jachris]

While PRs are not allowed to have merge commits, they can still be useful locally (or for not rewriting history for longer-running branches).

However, they break the commit detection logic for downloading CI artifacts.

Steps to Reproduce

1. git checkout <old-commit>
2. git merge --no-ff master
3. ./x.py build library/std

Expected Result

Builds fine.

Actual Result

...
downloading https://ci-artifacts.rust-lang.org/rustc-builds/<old-commit>/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
curl: (22) The requested URL returned error: 404      #                                                                               

error: failed to download llvm from ci

help: old builds get deleted after a certain time
help: if trying to compile an old commit of rustc, disable `download-ci-llvm` in config.toml:

[llvm]
download-ci-llvm = false

Build completed unsuccessfully in 0:01:43

[Mark-Simulacrum]

Can you say more about their usefulness? What are you merging in? We can maybe support merging non-bors commits, though it brings some complexity (e.g., which parent do we follow for the "nearest" bors commit, and what does nearest mean at that point), so in general I would be inclined to say this isn't supported and you should build locally.

[jachris]

I don't think it's a must-have. I'm personally fine with building locally or rebasing. But this would allow longer-running branches to use the CI artifacts without rewriting history due to rebasing. Also, the current behavior can be a little bit confusing if one is not aware of this issue.

[RalfJung]

Also, the current behavior can be a little bit confusing if one is not aware of this issue.

Yeah it is confusing. :D Could you explain what happens?
Is this about having to detect the "latest commit from rustc master that is in this branch", so that we can download the right LLVM, and that fails?

[jachris]

From what I can tell, yes. The logic that determines that commit seems to not account for merge commits. But @jyn514, who helped me troublshoot this issue initially, might know the details.

[RalfJung]

I now also ran into this because syncing Miri changes back into rustc creates merge commits. Not sure how to work around this problem; rebasing is not an option for these syncs and a local build takes forever...

Cc @jyn514

[RalfJung]

Specifically that Miri history contains commits made by bors in Miri, which then fools the detection logic to think that was a rustc bors commit.

[RalfJung]

Currently, bootstrap uses the following command to determine the commit to fetch LLVM for

git rev-list --author=bors@rust-lang.org -n1 --first-parent HEAD -- src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version

I can see two ways of making things work even for the Miri sync situation:

• Instead of -n1, use -n10 or so and try several commits. For the case I have right now, the 2nd commit should already work.
• Instead of HEAD, use $(git merge-base HEAD origin/master). However we can't really know what the origin remote is called, so it'd be more like

git fetch https://github.com/rust-lang/rust master
git merge-base HEAD FETCH_HEAD

[jachris]

I think the second option would be a solid solution, and it also works for the usecase I had in mind.

(Side note: Though I'm sure why this uses --first-parent. If we are on master, there should be no merge commits, and this has no effect. If we are on another branch, it discards merges into that branch... which is basically the original issue.)

[RalfJung]

On master there are tons of merge commits -- the ones created by bors. In fact the commit we are looking for is always a merge commit, so maybe we should add --merges.

[jachris]

Ah yes, right, of course. The "no merge-commit policy" is only for PRs... Nevermind.

[Mark-Simulacrum]

Fetching introduces a network call on each x.py invocation, I think, since we need to know if something has changed. I guess we could try to cache based on the head sha, but I'm not generally a fan of that.

I think the right answer is probably to change bors to include a repository in the commits it generates more explicitly - perhaps the email could be bors+rust@rust-lang.org, for example.

Would need some care to update all of our automation, but seems achievable.

[RalfJung]

Yeah I agree having to invoke the network is not great. In principle we could search the user's remotes for one that points to our github repo and use $remote/master... but that seems pretty fragile.

Having different bors email addresses for different repos would help for the Miri case, yeah. And I guess for @jachris we could replace the --first-parent by --merges; the email filtering should be enough to trim down the candidates.

@Mark-Simulacrum I take it you are also not a fan of trying multiple commit candidates? I guess that would require caching negative lookups so it's also not great.

A temporary hack that would be helpful is some way to overwrite the HEAD commit from which it starts searching. For the Miri PR I just locally patched bootstrap to be able to do x.py check which I needed to update the lockfile.

[Mark-Simulacrum]

Updating the lockfile should be cargo metadata >/dev/null or something like that, no need for x.py.

Yeah, multiple candidates I think has similar problems in terms of network hits (or negative cache of some kind).

--merges seems fine to me, but I'm not sure we can do away with --first-parent - for example, in the case of rollups, we need to be sure we're looking at the first parent commit to traverse the commit graph. I guess with -n1 that might not matter but it feels iffy to me.

Can you point me at the SHA/branch for the miri merges that are causing trouble? I wonder if the merge commits might be distinguishable by (for example) not having a tree associated with them that contains src/stage0.json or something like that.

[RalfJung]

Yeah you can just check out the branch for #103392 to see the problem.

I wonder if the merge commits might be distinguishable by (for example) not having a tree associated with them that contains src/stage0.json or something like that.

No that wouldn't help... this is Miri history reconstructed into the rust repo, it also has all of the rest of the rust repo around.

Updating the lockfile should be cargo metadata >/dev/null or something like that, no need for x.py.

Ah, I never even tried doing anything outside of x.py in the rustc repo, it seemed hopeless. But anyway sometimes I might also have to actually build that branch.

[Mark-Simulacrum]

Yeah, I see. OK, I think the best option then is going to be the adjustment to bors to highlight which repository the merge commit is from. We'll want to check against triagebot, crater, rustc-perf, cargo-bisect-rustc, and the promote-release infrastructure which I think are the infrastructure places that have some amount of processing of rustc commit history.

[jyn514]

We don't need to fetch to support remotes named something other than origin, we can reuse the logic in

rust/src/tools/build_helper/src/git.rs

Lines 22 to 30 in 3e565f1

	/// Finds the remote for rust-lang/rust.
	/// For example for these remotes it will return `upstream`.
	/// ```text
	/// origin https://github.com/Nilstrieb/rust.git (fetch)
	/// origin https://github.com/Nilstrieb/rust.git (push)
	/// upstream https://github.com/rust-lang/rust (fetch)
	/// upstream https://github.com/rust-lang/rust (push)
	/// ```
	pub fn get_rust_lang_rust_remote(git_dir: Option<&Path>) -> Result<String, String> {

• $(git merge-base HEAD origin/master)

👍 this seems like a good solution

[RalfJung]

This problem still regularly hits me when I synchronize Miri changes.

An LLVM_FETCH_COMMIT env var hack would help a lot to bridge the gap until a proper solution emerges.

EDIT: For the record, here's a hacky patch that helps:

diff --git a/src/bootstrap/download.rs b/src/bootstrap/download.rs
index 3e82a381a1b..7600365d29b 100644
--- a/src/bootstrap/download.rs
+++ b/src/bootstrap/download.rs
@@ -568,6 +568,7 @@ pub(crate) fn maybe_download_ci_llvm(&self) {
         let llvm_root = self.ci_llvm_root();
         let llvm_stamp = llvm_root.join(".llvm-stamp");
         let llvm_sha = detect_llvm_sha(&self, self.rust_info.is_managed_git_subrepository());
+        let llvm_sha = "PUT_SHA_HERE"; // output of `git merge-base HEAD origin/master` goes here
         let key = format!("{}{}", llvm_sha, self.llvm_assertions);
         if program_out_of_date(&llvm_stamp, &key) && !self.dry_run() {
             self.download_ci_llvm(&llvm_sha);

[RalfJung]

Reopened by #114345. I hope the perf team can help figure out what is going on; just calling git merge-base shouldn't cause any side-effects.

[RalfJung]

Uh, why should I call it "upstream"? The standard name is "origin" isn't it?

[RalfJung]

Anyway I think the main question is whether the --first-parent in that logic is load-bearing or whether we can remove it.

[Mark-Simulacrum]

I would definitely expect it to be load bearing if there's merge commits between HEAD and the closest bors commit. (If there's not then there's no parents to choose between).

I think that situation is relatively rare, but would e.g. come up with subtree syncs.

You shouldn't need a particular name for the remote though, iirc we lookup a remote that has the right target (rust-lang/rust). Maybe we don't do that in all cases though...

[RalfJung]

I think that situation is relatively rare, but would e.g. come up with subtree syncs.

A Miri subtree sync is exactly where --first-parent is causing problems. :)

[Mark-Simulacrum]

@RalfJung Can you say more about how you ended up with f461981 as HEAD? By the first parent history that traces to a bors commit on Tue Oct 22 08:21:37 2024 (quite old...), which makes sense in terms of why that broke things.

For bors merges, we take care that the first parent of the merge commit is the previous bors merge. This property is also kept for rollup merges (e.g., 0a33d7c has a first parent of 3536503 which ends up chaining recursively to c49fc91 and finally a9e7b30 for that rollup's bors merge parent).

It seems like miri is not respecting that property, sometimes the "rustc side" of the merge is most recent through the 2nd parent commit -- which is why --first-parent is causing issues here. If that property was respected, then first-parent would work fine.

The reason for first-parent (or something like it -- doesn't have to be first necessarily) is that the alternative (most recent commit ("reverse chronological order" by docs in rev-list) with bors@rust-lang.org as the email) could have found a clippy, miri, etc. bors merge commit as being more recent in the past, even in rust-lang/rust history, if you were syncing in merge commits from those repos (first-parent avoided that there because it refused to traverse the "sideways" history). But now that we no longer use bors anywhere except rustc, it seems plausible that just dropping --first-parent is actually fine? The most recent bors@rust-lang.org commit you can find should always be the right one to use, since nothing else is producing those commits other than rust-lang/rust CI these days IIUC...

It seems like you came to that conclusion earlier (#101907 (comment)) but we didn't actually follow up and improve the finding logic given that realization :)

[RalfJung]

Can you say more about how you ended up with f461981 as HEAD? By the first parent history that traces to a bors commit on Tue Oct 22 08:21:37 2024 (quite old...), which makes sense in terms of why that broke things.

It was created by ./miri rustc-push in the Miri repo. IOW, this is a standard Miri subtree sync. The way those come out, the PR itself contains a merge commit (which we usually forbid but [have to] allow for subtree syncs) where the first parent is the Miri history and the 2nd parent is the rustc history. By going up the first parents, you find an old bors commit created in the Miri repo, from the time when Miri used bors.

The reasons the merge commit looks like that is that it got created in Miri (by a GH merge), where as usual the first commit is "the previous HEAD of this repo" and the 2nd commit is "the changes of this PR". The rustc history is "changes made in the PR" when that PR is a rustc-pull (i.e. syncing rustc changes into Miri).

[RalfJung]

Somehow the removal of --first-parent caused #137661 to fail so it had to be reverted. Does anyone understand why? That PR doesn't even contain a merge commit. It also changes LLVM so it shouldn't download anything anyway, right?

Not being able to test Miri syncs locally is quite annoying so it'd be really good to finally resolve this issue.

[onur-ozkan]

Somehow the removal of --first-parent caused #137661 to fail so it had to be reverted. Does anyone understand why? That PR doesn't even contain a merge commit. It also changes LLVM so it shouldn't download anything anyway, right?

Yeah, it shouldn't. I am planning to have a look at that (both on git and LLVM download logics) in a couple of days.

[Mark-Simulacrum]

That PR doesn't even contain a merge commit

Note that CI does run on a merge commit (f43d771 in the case of the failed CI run).

However both parents of the merge commit are recent (a46c755, the first parent, looks right to me), it is also the indirect parent of the 2nd parent (e045a02 -> a46c755).

An empty rev-list result so far for me is from this¹²:

$ git rev-list --first-parent --author=bors@rust-lang.org -n1 f43d771d2eafb5ae9b94f2734073c91e213320db -- src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version
a46c755c6ad2c9543a836a1ca134178b2345acc2

$ git rev-list --author=bors@rust-lang.org -n1 f43d771d2eafb5ae9b94f2734073c91e213320db -- src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version
# empty

It's possible my local env is messed up, but if I set it up correctly, all the "known" commits are:

$ git rev-list f43d771d2eafb5ae9b94f2734073c91e213320db
f43d771d2eafb5ae9b94f2734073c91e213320db
e045a025661358ca83887b68a7069b9143202ca1
a46c755c6ad2c9543a836a1ca134178b2345acc2

(because we have a shallow fetch: 2025-02-26T08:52:41.3655527Z [command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=2 origin +f43d771d2eafb5ae9b94f2734073c91e213320db:refs/remotes/pull/137661/merge)

That said, it's not very clear to me how first-parent managed to find a commit when regular traversal doesn't. I wouldn't expect that to increase what results are available to git. Maybe the diff attributed to a commit changes depending on how that commit is reached, given the shallow history?

Footnotes

1. Note that I thought originally CI was using git merge-base origin/master HEAD to compute the commit to pass to rev-list (see code: https://github.com/rust-lang/rust/blob/0c72c0d11adeba449886089c6bd5d48363f7a2cd/src/build_helper/src/git.rs#L133-153), but this does not currently run. src/ci/channel is "nightly\n", not "nightly", and we are in a rust-lang managed CI job. ↩

2. Also note that neither result ends up leading to a correct outcome. This PR CI should not download LLVM, because it is modifying LLVM. We refuse to do that if the detected LLVM SHA is the same as HEAD, but in the case of pull request CI, the HEAD sha is not a bors merge, so we'd happily download some other LLVM AFAICT, not actually testing the changes until the change hits proper bors CI. ↩

[RalfJung]

Uh, removing --first-parent finds fewer things? Yeah that makes no sense to me. Can we just treat "empty result" as "do not download LLVM"?

[onur-ozkan]

This would silently hide actual bugs in the commit finding logic. I just made #137994 which adds a fallback logic to git_upstream_merge_base and aims to make both sides (regular CI flows and subtree syncs) work.

[RalfJung]

Note that CI does run on a merge commit (f43d771 in the case of the failed CI run).

Where does that commit come from? It says @nikic created it, but it doesn't show up in the PR itself...?

Is this the commit created by github as part of CI? I didn't realize that would be attributed to the PR author, interesting.

An empty rev-list result so far for me is from this

How do you reproduce this? Did you do a shallow clone and then get that result?

Locally when I run that comment on my regular clone, I get

$ git rev-list --author=bors@rust-lang.org -n1 f43d771d2eafb5ae9b94f2734073c91e213320db -- src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version
ce36a966c79e109dabeef7a47fe68e5294c6d71e

This is the same result I also get with --first-parent. However, that commit is quite a ways back in the history; a --depth=2 checkout would not contain it. But that would always happen, right? A --depth=2 clone will not contain the last commit that changed LLVM most of the time.

The main thing I am wondering about how: given that we have a shallow clone in CI, it makes no sense at all to try and determine anything about the git history. What even is our plan here, what is the intended behavior?

Also, what is the intended behavior if LLVM has changed since the most recent merge commit that updates LLVM? It seems the current logic would just blatantly ignore that and use the downloaded LLVM anyway, or is there a check somewhere that I missed?

[RalfJung]

How do you reproduce this? Did you do a shallow clone and then get that result?

Okay yeah I can reproduce this as well, using

git init rustc
cd rustc
git remote add origin https://github.com/rust-lang/rust
git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=2 origin +f43d771d2eafb5ae9b94f2734073c91e213320db:refs/remotes/pull/137661/merge

The answer you get with --first-parent is still wrong: it's just the current master commit (i.e. what got merged into the PR commit to form the CI commit), no matter whether it changed LLVM or not.

The docs for --first-parent mention something interesting:

This option also changes default diff format for merge commits to first-parent, see --diff-merges=first-parent for details.

And indeed, using --diff-merges=first-parent instead of --first-parent does produce a non-empty result:

git rev-list --author=bors@rust-lang.org -n1 f43d771d2eafb5ae9b94f2734073c91e213320db --diff-merges=first-parent -- src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version
a46c755c6ad2c9543a836a1ca134178b2345acc2

And it seems like that option will also work for Miri sync commits:

$ git rev-list --author=bors@rust-lang.org -n1 716dd22 --diff-merges=first-parent -- src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version
07697360aee0cebcb4e304236ba1884d8dde5469

However, I am still not convinced it makes sense to run get_closest_merge_commit at all in a shallow clone, as the commit you're getting out will always just be the most recent bors commit, no matter whether it changed LLVM or not.

This would silently hide actual bugs in the commit finding logic.

We are already silently hiding bugs in that logic, it seems.

[RalfJung]

FWIW the points about shallow clones could also explain this:

            // FIXME: When running on rust-lang managed CI and it's not a nightly build,
            // `git_upstream_merge_base` fails with an error message similar to this:

with such a shallow view of what the history is, there's no way git can reliably compute the merge base.

On nightly CI, git merge-base still works since the very latest master commit is part of the shallow clone (it is the first parent of HEAD). On beta/stable CI, ofc the merge base is further away, and therefore not part of the shallow clone any more, so git merge-base fails.

So we have to either make a full clone, or stop doing any kind of git history queries in CI. (But also, on beta/stable CI, I don't think the merge base with master should even be relevant.)

[Skgland]

So we have to either make a full clone, or stop doing any kind of git history queries in CI. (But also, on beta/stable CI, I don't think the merge base with master should even be relevant.)

Would a partial (blobless) clone instead of a shallow clone work maybe?

# https://github.com/actions/checkout/issues/1152#issuecomment-1752027288
steps:
  - name: Checkout
    uses: actions/checkout@v4
    with:
      filter: blob:none
      fetch-depth: 0 # (no history limit)

[RalfJung]

I don't think that would help since we are filtering based on the diff of the commits, and determining the diff needs the blobs.

[Skgland]

I don't think that would help since we are filtering based on the diff of the commits, and determining the diff needs the blobs.

Right, I was for some reason thinking that filtering was based on commit message content.
Blobless clone should cause it to fetch blobs one by one when it's missing a blob for a diff.
While it might fix the problem, loading blobs one by one sounds undesirable.

[jieyouxu]

Reopening because I'm posting a revert for #137593 to fix perf.

[RalfJung]

Where can I see the failing CI log that is fixed by the revert?

[jieyouxu]

(Asking in #t-infra > benchmarks failed?)