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.