[llvm][utils] Add a script to use PRs over pushing to main #166473
Conversation
As discussed on discourse https://discourse.llvm.org/t/rfc-require-pull-requests-for-all-llvm-project-commits/88164 This is an attempt at a script to automatically make and land PRs. It creates a branch (or a series of branches for stacks), and lands them one by one, without waiting for review or checks to pass. It supports --auto-merge, for single PRs. For stacks, use Graphite or some other tool. It can work with GitHub tokens for https or ssh keys. Example: ```console GITHUB_TOKEN=$(gh auth token) python3 llvm_push_pr.py --upstream-remote origin ```
llvm/utils/llvm_push_pr.py
Outdated
| self.printer.print(f"On branch: {self.original_branch}") | ||
|
|
||
| try: | ||
| self._rebase_current_branch() |
There was a problem hiding this comment.
Why is this part of the flow?
There was a problem hiding this comment.
Let me see what it looks like w/o a rebase here.
There was a problem hiding this comment.
I would think that without a stack, I shouldn't have to rebase to open my PR (just like I would normally).
(Not that it is a big deal either I guess)
There was a problem hiding this comment.
yeah, after we started rebasing reliably after every PR lands, running successive commands no longer runs into the class of problems I was seeing.
llvm/utils/llvm_push_pr.py
Outdated
| "For stacks, the script must merge sequentially.", | ||
| file=sys.stderr, | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Why is that? When pushing branches to upstream, you can cascade pull-requests to create your stack.
There was a problem hiding this comment.
I didn't want to build support for every feature under the sun, just some MVP that people that will actually use this tool can enhance. certainly its possible in the API to set the base branch, but that was much more complicated IMO than what was asked for as an MVP on discourse.
There was a problem hiding this comment.
I didn't want to build support for every feature under the sun,
Sure, but then a good starting point would be to just not support stacks at all and bail out when there are multiple commits!
Otherwise I would argue we should implement just the most straightforward flow for stacks, which I think is a cascade of branches (which is the only way you can correctly setup a stack without merging it AFAIK).
There was a problem hiding this comment.
Fair, but landing a stack was specifically asked for in the discourse thread. I have no plans to implement a cascade of related PRs. IMO that's work out of scope for the MVP, while making it possible to land a stack was mostly straight forward, since it was mostly just a loop around the existing logic for a single PR.
boomanaiden154
left a comment
boomanaiden154
left a comment
There was a problem hiding this comment.
I'm assuming you've tested this somewhere?
llvm/utils/llvm_push_pr.py
Outdated
| """Wrapper for run_command that passes the dry_run flag.""" | ||
| return self.printer.run_command(command, read_only=read_only, **kwargs) | ||
|
|
||
| def _get_repo_slug(self) -> str: |
There was a problem hiding this comment.
This duplicates functionality below and should also be constant given we only need to support the monorepo.
There was a problem hiding this comment.
done. Also handled pretty differently now. I just assumed a bunch of things about LLVM that are basically always going to be true.
llvm/utils/llvm_push_pr.py
Outdated
| self.printer.print(f"On branch: {self.original_branch}") | ||
|
|
||
| try: | ||
| self._rebase_current_branch() |
There was a problem hiding this comment.
We don't need to rebase on top of main. Github handles the actual merge for us, and I think not rebasing is one of the selling points of using this script.
There was a problem hiding this comment.
If you have a stack, its going to get rebased anyway. it seemed better to find out right away that it failed due to a merge conflict or something, but we can certainly how it works if I omit it.
There was a problem hiding this comment.
Also, I found in my refactoring that if you don't have an accurate merge base w/ this approach, and you have a stack, you'll make redundant empty PRs if you don't say rebase your branch on top of main before trying to land again.
There was a problem hiding this comment.
I shored this up, and it seems to be working fine now that any PR landing always has a rebase operation following it. so the initial rebase is no longer necessary.
| def _get_commit_stack(self) -> List[str]: | ||
| """Gets the stack of commits between the current branch's HEAD and its merge base with upstream.""" | ||
| target = f"{self.args.upstream_remote}/{self.args.base}" | ||
| merge_base_result = self._run_cmd( |
There was a problem hiding this comment.
Why do we need to find a merge base here? Why can't we just do a three dot diff between main and the user's branch?
I don't think this handles causes like someone rebasing a bunch of merged commits any more nicely.
There was a problem hiding this comment.
IIRC it was to handle the case where main is ahead of your branch. IIRC I actually ran into this when prototyping.
There was a problem hiding this comment.
The three dot diff will handle this case. You only run into problems with that with a two dot diff.
llvm/utils/llvm_push_pr.py
Outdated
| self.printer.print("Cleaning up temporary local branches...") | ||
| self._run_cmd(["git", "branch", "-D"] + self.created_branches) | ||
| self.printer.print("Cleaning up temporary remote branches...") | ||
| self._run_cmd( |
There was a problem hiding this comment.
Will this delete branches that have automerge enabled?
There was a problem hiding this comment.
hmm, maybe. this wasn't something I could easily test. maybe the delete operation should only happen if its not an automerge situation?
There was a problem hiding this comment.
I don't think the delete operation ever happens when we have automerge enabled now. Do double check my logic, but when I tested it, that didn't happen, and I didn't see it in the verbose logging. The repo where I tested it didn't have much in the way of CI though, so its was just some linting. Still, I didn't see it retry or anything under --verbose when I tried it out last.
There was a problem hiding this comment.
Ack. Probably something that's easiest to test once it lands.
llvm/utils/llvm_push_pr.py
Outdated
| def _get_commit_details(self, commit_hash: str) -> tuple[str, str]: | ||
| """Gets the title and body of a commit.""" | ||
| result = self._run_cmd( | ||
| ["git", "show", "-s", "--format=%s%n%n%b", commit_hash], |
There was a problem hiding this comment.
| ["git", "show", "-s", "--format=%s%n%n%b", commit_hash], | |
| ["git", "show", "-s", "--format=%B", commit_hash], |
Why not just %B?
There was a problem hiding this comment.
I prototyped this with a lot of 1 line commits (subject only) and some weirdly formatted bodies. having the subject line then \n\n and the rest being the body worked well.
There was a problem hiding this comment.
Are you saying that you saw issues with %B?
There was a problem hiding this comment.
I updated the logic to use %B. Turns out I was overcomplicating the parsing behavior in my head. thanks.
llvm/utils/llvm_push_pr.py
Outdated
| self._run_cmd(["git", "branch", "-f", branch_name, commit_hash]) | ||
| push_command = ["git", "push", self.args.remote, branch_name] |
There was a problem hiding this comment.
Why are we creating local branches? We can just push a sha directly:
| self._run_cmd(["git", "branch", "-f", branch_name, commit_hash]) | |
| push_command = ["git", "push", self.args.remote, branch_name] | |
| push_command = ["git", "push", self.args.remote, f"{commit_hash}:refs/head/{branch_name}"] |
There was a problem hiding this comment.
hmm, I actually just used the same workflow I normally would. It seemed nicer for debugging, but maybe you're right, and we don't need that ATM.
llvm/utils/llvm_push_pr.py
Outdated
| """Checks if git is installed and if inside a git repository.""" | ||
| printer.print("Checking prerequisites...") | ||
| printer.run_command(["git", "--version"], capture_output=True, read_only=True) | ||
| if not os.getenv("GITHUB_TOKEN"): |
There was a problem hiding this comment.
Can we name it GITHUB_LLVM_TOKEN?
In general folks may want to segregate their tokens to be finer grain, and so having a name that is more specific allows them to put it in a .bashrc or similar without colliding with other token needs.
llvm/utils/llvm_push_pr.py
Outdated
| parser = argparse.ArgumentParser( | ||
| description="Create and land a stack of Pull Requests." | ||
| ) | ||
| GITHUB_REMOTE_NAME = "origin" |
There was a problem hiding this comment.
I'm not sure why "origin" is the best name, instead of "fork" for example?
There was a problem hiding this comment.
if you're in your fork, isn't "origin" always your remote? or are you suggesting it's more common for folks to have a checkout of the upstream repository, and set "fork" to track their personal fork of LLVM (origin being llvm/llvm-project)? Or maybe I'm missing your point?
There was a problem hiding this comment.
I always clone https://github.com/llvm/llvm-project, so origin points to upstream and then I have fork as a remote to my fork.
Not sure what's more common though.
There was a problem hiding this comment.
good point. I wrote this assuming forks are more common. That constant is just the default though, and you can override it w/ --remote and the upstream w/ --upstream-remote. Maybe there are better names for these though.
After starting to address a few comments, I basically realized I needed a new approach. I by and large kept the structure the same, but have revised the library usage to ust use things from the standard library. I've slightly modified some of the initial classes to be provide more useful abstractions. There's probably room for more improvements, but I wanted to keep this as simple as possible.
|
✅ With the latest revision this PR passed the Python code formatter. |
Also fix up some comments
This isn't needed anymore, since we reliably rebase after every PR lands.
This was less trouble to adapt than I thought.
🐧 Linux x64 Test Results
|
boomanaiden154
left a comment
boomanaiden154
left a comment
There was a problem hiding this comment.
I think this is pretty close.
| def _get_commit_stack(self) -> List[str]: | ||
| """Gets the stack of commits between the current branch's HEAD and its merge base with upstream.""" | ||
| target = f"{self.args.upstream_remote}/{self.args.base}" | ||
| merge_base_result = self._run_cmd( |
There was a problem hiding this comment.
The three dot diff will handle this case. You only run into problems with that with a two dot diff.
llvm/utils/llvm_push_pr.py
Outdated
| self.printer.print("Cleaning up temporary local branches...") | ||
| self._run_cmd(["git", "branch", "-D"] + self.created_branches) | ||
| self.printer.print("Cleaning up temporary remote branches...") | ||
| self._run_cmd( |
There was a problem hiding this comment.
Ack. Probably something that's easiest to test once it lands.
| git_env["GIT_TERMINAL_PROMPT"] = "0" | ||
| return git_env | ||
|
|
||
| def _run_cmd( |
There was a problem hiding this comment.
Is an extra function here really necessary?
| parser.add_argument( | ||
| "--dry-run", action="store_true", help="Print commands without executing them." | ||
| ) | ||
| group = parser.add_mutually_exclusive_group() |
There was a problem hiding this comment.
Is there a reason we can't name the two groups differently? I think that would keep things more clear even though it doesn't impact functionality.
3 participants
As discussed on discourse
https://discourse.llvm.org/t/rfc-require-pull-requests-for-all-llvm-project-commits/88164
This is an attempt at a script to automatically make and land PRs.
It creates a branch (or a series of branches for stacks), and lands them one by one, without waiting for review or checks to pass.
It supports --auto-merge, for single PRs. For stacks, use Graphite or some other tool.
It can work with GitHub tokens for https or ssh keys.
Example:
GITHUB_TOKEN=$(gh auth token) python3 llvm_push_pr.py --upstream-remote origin