Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bootstrap.py: use git rev-list for robustness #87532

Merged
merged 3 commits into from
Aug 5, 2021
Merged

bootstrap.py: use git rev-list for robustness #87532

merged 3 commits into from
Aug 5, 2021

Conversation

tlyu
Copy link
Contributor

@tlyu tlyu commented Jul 27, 2021 •
edited
Loading

Use git rev-list instead of git log to be more robust against
UI changes in git. Also, use the full email address for bors,
because --author uses a substring match.

Based on #87513, but is separate because it's less minimal and may require additional manual testing.

Open questions:

  • Should the merge_base search also use --first-parent?
  • Do we exclude non-merge commits from bors? There are a few, and I'm not sure what they have in common. Some of them look like squashes, and some look like they're in rollup branches.

r? @jyn514
@rustbot label +A-rustbuild +C-cleanup

hudson-ayers and others added 2 commits July 27, 2021 14:53
@hudson-ayers
bootstrap.py: remove unused git log option
1259742 
When determining which LLVM artifacts to download, bootstrap.py
calls: `git log --author=bors --format=%H -n1 -m --first-parent --
src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version`.
However, the `-m` option has no effect, per the `git log` help:

> -m
> This option makes diff output for merge commits to be shown in the
> default format. -m will produce the output only if -p is given as
> well. The default format could be changed using log.diffMerges
> configuration parameter, which default value is separate.

Accordingly, this commit removes use of the -m option in favor of
`--no-patch`, to make clear that this command should never output
diff information, as the SHA-1 hash is the only desired output.
Tested using git 2.32, this does not change the
output of the command.

The motivation for this change is that some patched versions of git
change the behavior of the `-m` flag to imply `-p`, rather than to do
nothing unless `-p` is passed. These patched versions of git lead to
this script not working. Google's corp-provided git is one such example.
@tlyu
bootstrap.py: use git rev-list for robustness
5d28520 
Use `git rev-list` instead of `git log` to be more robust against
UI changes in git. Also, use the full email address for bors,
because `--author` uses a substring match.
@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Jul 27, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 27, 2021

Do we exclude non-merge commits from bors? There are a few, and I'm not sure what they have in common. Some of them look like squashes, and some look like they're in rollup branches.

I don't see why we would. The reason to use --author=bors is to make sure the commit has CI artifacts; as long as it does it doesn't matter if it's a merge or not.

Should the merge_base search also use --first-parent?

Hmm, the man page says:

Follow only the first parent commit upon seeing a merge commit. This option can give a better overview when viewing the evolution of a particular topic branch, because merges into a topic branch tend to be only about adjusting to updated upstream from time to time, and this option allows you to ignore the individual commits brought in to your history by such a merge.

I don't know why we use this in the LLVM download - I'm not confident that the first parent will always be the one with a bors author, and --author=bors should be enough of a filter anyway. cc @Mark-Simulacrum, you added this originally I believe.

Either way I don't expect it to make a big difference, we say pretty clearly on https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy that you shouldn't use merge commits.

jyn514
jyn514 approved these changes Jul 27, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

@Mark-Simulacrum
Copy link
Member

author=bors is insufficient (in general) because clippy and potentially other repos also use bors but (obviously) don't upload the full set of rustc artifacts (or any artifacts at all today).

I believe first parent means we traverse the chain of commits appropriately, always going along rust-lang/rust bors merges.

@tlyu
Copy link
Contributor Author

tlyu commented Jul 28, 2021

Do we exclude non-merge commits from bors? There are a few, and I'm not sure what they have in common. Some of them look like squashes, and some look like they're in rollup branches.

I don't see why we would. The reason to use --author=bors is to make sure the commit has CI artifacts; as long as it does it doesn't matter if it's a merge or not.

I think bors might only run the CI on its auto-merge commits, not its non-merge commits? But I'd want to check with someone who knows bors/homu better.

Should the merge_base search also use --first-parent?

I don't know why we use this in the LLVM download - I'm not confident that the first parent will always be the one with a bors author, and --author=bors should be enough of a filter anyway. cc @Mark-Simulacrum, you added this originally I believe.

I think that --first-parent might help exclude merge commits that are parts of rollup merges, but I'm not sure. It does seem to exclude some of the squash commits made by bors. (See below.)

Either way I don't expect it to make a big difference, we say pretty clearly on https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy that you shouldn't use merge commits.

Most of the non-bors merge commits that I see at a casual glance seem to be parts of rollup merges.

Details on non-merge bors commits:

git log --no-merges --author=bors@rust-lang.org --pretty=%h upstream/master
76a3b609d0b
98b11c8f7bf
4b806878549
cb4553bdae5
733cb54d18c
204bb9b54b1

but

git log --first-parent --no-merges --author=bors@rust-lang.org --pretty=%h upstream/master

produces no output.

Also, digging through git log --graph for the commits listed by the first command shows that the next commit after each of those non-merge bors commits seems to be a merge commit from bors.

So now I'm leaning towards yes on --first-parent and probably yes on --merges.

@tlyu
boostrap.py: only look for merges by bors
e0d7a59 
Only look for commits by bors that are merge commits, because
those are the only ones with CI artifacts. Also, use
`--first-parent` to avoid traversing stuff like rollup branches.
@tlyu
Copy link
Contributor Author

tlyu commented Jul 28, 2021

Pushed an update. Now both of them use --first-parent and --merges.

@tlyu
Copy link
Contributor Author

tlyu commented Jul 28, 2021

Looks like #87513 has merged; should I rebase and/or squash?

@jyn514
Copy link
Member

jyn514 commented Aug 5, 2021

@tlyu I don't think you need to, there aren't conflicts.

@jyn514
Copy link
Member

jyn514 commented Aug 5, 2021

@bors r+

I'm not sure I buy that this is avoiding clippy commits, but it doesn't seem any worse than before, and we can fix it if something goes wrong.

@bors
Copy link
Contributor

bors commented Aug 5, 2021

📌 Commit e0d7a59 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2021
@bors
Copy link
Contributor

bors commented Aug 5, 2021

⌛ Testing commit e0d7a59 with merge 2ddb65c...

@bors
Copy link
Contributor

bors commented Aug 5, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 2ddb65c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 5, 2021
@bors bors merged commit 2ddb65c into rust-lang:master Aug 5, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 5, 2021
@ghost ghost mentioned this pull request Aug 12, 2021
Closed
@tlyu tlyu deleted the bootstrap-rev-list branch November 16, 2021 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 approved these changes

Assignees

@jyn514 jyn514

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.
7 participants
@tlyu @jyn514 @Mark-Simulacrum @bors @rust-highfive @rustbot @hudson-ayers