llvm-iwg
llvm-iwg copied to clipboard
llvm
A Request for Comment on Code Review Process
Proposal
The LLVM Foundation Board of Directors is seeking comment on the current state of Code Review within the LLVM Project and its sub-projects. Phabricator is no longer actively maintained and we would like to move away from a self-hosted solution, so our goal is to determine if GitHub Pull Requests are a good alternative to our current code review tool: Phabricator.
Specifically we are looking for feedback on:
- What features or properties make Github Pull Requests better than Phabricator?
- What features or properties make Phabricator better than GitHub Pull Requests?
- What new workflows or process improvements will be possible with GitHub Pull Requests?
- Which workflows aren’t possible with GitHub Pull Requests?
- Any other information that you think will help the Board of Directors make the best decision.
Where to Direct Feedback
Please provide feedback on this Infrastructure Working Group ticket. This will make it easier to collect and consolidate the responses. At the end of the comment period the Infrastructure Working Group will collect the feedback for further analysis and summarization.
Timeline
The timeline for this RFC will be as follows:
- RFC posted for public review and comment
- 30 days after the date of posting, public comment closes.
- IWG will have 14 days from closure of public comments to review and summarize public comments into a pros and cons list to be present to LLVM Foundation Board
- Foundation Board will have 30 days to make a final decision about using GitHub Pull Requests and then communicate a migration plan to the community.
What features or properties make Github Pull Requests better than Phabricator?
TL;DR - new users can contribute to the project using tooling that they are already familiar with.
Clang got a really cool feature because some non-LLVMer (not part of the core community) did some modifications and then our community was fortunate enough that someone within the community took the time to pull the GitHub PR for it over into Phabricator - I'm talking about -time-trace.
From talking to people outwith the core LLVM community - the number one reason that people don't attempt to push legit fixes/features back to LLVM is that the code review process is a) non-normative, and b) scary as a result. I think moving to GitHub PRs, or at least offering them as an option, would greatly help onboarding people to the project.
[ For context: I vastly prefer GitHub's PR flow to Phabricator ]
Which workflows aren’t possible with GitHub Pull Requests?
GitHub's PRs make it very difficult to provide a patch set that can be reviewed independently. The only way that this can be done at all is:
- Create a PR for the first patch in the series.
- Create a PR for the second patch in the series to merge not to main, but to the branch of the first.
- Repeat step 2 for each patch in the sequence up to N.
- Merge PR for patch N into N-1.
- Repeat step 4 until everything is merged into the first PR.
- Merge the first PR.
This is very error-prone. The root problem with GitHub PRs is that the target branch cannot be changed after the PR is created. If it could, then we could create PRs to merge into the next branch in the sequence but then merge them into main in order and reset the next PR to try to merge to main (or merge it into the intermediate branch if reviewers are happy with the roll-up).
GH Bad:
- GitHub confuses discussion comments with review comments in the code view.
- You can only answer multiple comments in the code view.
- Old discussion view keep some comments that are not reachable from code view.
- GitHub doesn't allow to see diffs between arbitrary revisions, only commits directly.
- Commenting on a commit doesn't guarantee users can reach from discussion or code views
- Patch sets, as @davidchisnall explained above.
- However, a patch set that is in a single branch is dead easy.
GH Good:
- It's free, it's under development
- GitHub is accessible to millions of developers that already use PRs
- GitHub has lots of options to protect branches, required reviews, run actions, etc.
- GitHub allows direct merge into the repo, while Phab requires offline merge / arcanist.
Phab Bad:
- It's dying, it's custom, it's self hosted
- It's a hassle to register and it's an interface that isn't intuitive to a lot of people.
- I have to upload a diff file... Or use "arcanist", yet another unintuitive tool.
- I have to rebase/merge by hand... or use "arcanist"...
Phab Good:
- Possible to make dependent reviews (series), but it's not really easy
- I have to create each review separate, add parent/child links, etc.
- When I change my branch(es), it doesn't update, I have to upload again
- Even if the patch set is in a single branch, it's still not automatically updated
@davidchisnall @rengolin Assume that you have 2 dependent patches and you push 2 branches:
- PR1:
branch1->main - PR2:
branch2->branch1
When you merge PR1, and delete branch1, PR2 will automatically retarget itself from branch1 to main.
Also, a PR can have the base changed manually: https://docs.github.com/en/github/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request
@kparzysz-quic yes that works but is has two drawbacks I believe:
-
It forces you to push all the branches to the main repo and open pull-request from within the repo, because otherwise the pull-request from branch2 to branch1 is not visible in the main repo.
-
It means that updating these requires to rebase / keep a clean history locally. While I like this and this is how I work with Phabricator, that allows goes against other advice of not rebasing or amend/force-push a pull-request to preserve the review history best.
In 2019 at the Women in Compilers and Tools Workshop before the LLVM Developers meeting and again this year at the Community.o summit in March, we held workshops discussing barriers for new contributors. In both workshops utilizing GitHub pull requests was identified as a way of lowering the barrier for contributions.
Good, bad, or otherwise, git has become the dominant revision control system for software, especially open source, and hosting services like GitHub, GitLab, Bitbucket and Azure DevOps have made the pull-request model the standard workflow for the industry.
Whatever warts GitHub's pull requests may have, they are the most widely used model for contributing code and providing code reviews. LLVM going against the grain is an unfortunate detriment to our community.
While I'm unhappy with github PRs, @llvm-beanz point is more important than my personal preference.
While I'm also unhappy with Phabricator, the reverse is also true.
To me, the best course of action is to find a set of guidelines for people that are used to Phabricator to do the same on pull requests, and keep that document up-to-date.
I want to make sure we don't alienate existing contributors for the sake of new ones, but that we also don't make it hard to have new ones at all.
My personal view is that existing contributors know more about llvm and our community and would be more comfortable adapting to a new paradigm, for the time and love invested in the project.
But that's my personal view and in no way takes into consideration other people's difficulties, which could be many, and serious.
@joker-eph Yeah, we could end up with hundreds of branches in the main repo. I guess we could agree that branches that are merged should be deleted, and, if left undeleted, can be deleted by anyone (unless they are explicitly made exceptions). The force pushes are probably inevitable given that PRs work on commits, not just diffs...
I would suggest that all series have to be managed in the developers repos, and only one of their branches to be requested against the official repo's main branch.
So we can all have different branches and a meta branch that Cherry picks from those, probably even have a script for that, and then when the metà branch is pushed to origin, the PR is updated as expected.
To me, what tool I use for code review is one of the most important technical decisions that impacts me as a community member. Last time I gathered data, I participate in approx 80-100 distinct reviews a month in the clang and clang-tools-extra projects, so the choice of code review tools impacts me basically all day, every day.
Specifically we are looking for feedback on:
- What features or properties make Github Pull Requests better than Phabricator?
Once you get used to the GitHub workflow, creating patches from PRs off a branch is quite easy. I'd note that some people use arcanist to simplify creating a Phab patch the same way (I'm not one of those folks, I use Phab directly through the web interface). I also like how easy it is to link to issues with the GitHub UI compared to Phab, but this is a lesser benefit to me (copy-pasting a link to bugzilla is not a ton of effort). The more full-featured markdown abilities in the editor are a nice touch, but not a critical thing to me.
- What features or properties make Phabricator better than GitHub Pull Requests?
The code review interface has far less surprises and annoyances. I'm sure this comes somewhat from the amount of time I've used Phab (many years) vs the amount of time I've used GitHub (~2 years), but I've never found GitHub's code review workflow to be particularly ergonomic. It hides the most important files in a review because there are "too many changes" in it, too many comments on a review slows down the web interface, etc. It's basically the same usability concerns that have been brought up by myself and others every time this topic comes up.
Having used both for a few years now, I still find I am productive in Phab in ways that I'm not productive in GitHub, but it's not easy to boil it down to a succinct "if you fix this, my issue goes away" issue so much as an overall workflow within the tool issue. Anecdotally, it seems to take me about 25% longer when reviewing in GitHub compared to Phabricator, so I'm worried whether I'll be capable of reviewing at the same pace I currently do if we switched.
- What new workflows or process improvements will be possible with GitHub Pull Requests?
None that matter to my day-to-day work, at least that I'm aware of.
- Which workflows aren’t possible with GitHub Pull Requests?
Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)
- Any other information that you think will help the Board of Directors make the best decision.
My personal preference is to continue to support Phabricator (whether through Phorge, our own fork, whatever). However, I understand the desire to not host our own solutions or have to maintain them, and there's a lot of allure for using GitHub because of its popularity.
But I'd like to reiterate my concern about the danger of switching tools (regardless of what we switch to). The Clang frontend already struggles with review velocity, as can be seen by the number of time people need to "ping" a review thread. This isn't because reviewers are struggling with the tools, it's because reviewers who know the internals of the product well enough to make a decision about a patch are hard to get time from. Increasing the number of PRs the community receives while decreasing the amount of qualified reviews is a very real possibility and is not the kind of experience we want people new to the community to have. I don't know how to address that, but I do know that for myself, I definitely see a decreased amount of productivity when working in GitHub compared to Phab and I know that my review queue already sometimes fills up faster than I can perform the reviews today.
@joker-eph Yeah, we could end up with hundreds of branches in the main repo. I guess we could agree that branches that are merged should be deleted, and, if left undeleted, can be deleted by anyone (unless they are explicitly made exceptions). The force pushes are probably inevitable given that PRs work on commits, not just diffs...
Having hundreds of branches in the main repo because of PRs and patch sets is unmaintainable. Anyone deleting any branches is dangerous, as GH doesn't really check much before deleting. I don't think that's a viable solution.
IMHO we should prioritize switching from Bugzilla from GitHub over Phabricator. Reasons:
- bugs.llvm.org is still on version 5.0.2 (released in 2016)
- Latest release of Bugzilla was in 2019 and its last development commit was in February, while Phabricator went out of maintenance just this June
- As by the announcement, there will still be some maintenance, just not new features
- A 3rd party might fork Phabricator and continue its development
- Switching to GitHub Issues seems to be less controversial
- bugs.llvm.org still has self-registration disabled, a discouraging first obstacle for new community members; IMHO more than new developers having to become familiar with Phabricator.
Given point 3 and 4, there seems no immediate reason to switch away from Phabricator, especially when compared to the state of our bugtracker.
Having hundreds of branches in the main repo because of PRs and patch sets is unmaintainable. Anyone deleting any branches is dangerous, as GH doesn't really check much before deleting. I don't think that's a viable solution.
- Github allows you to set branch protection rules, so that important branches cannot be deleted.
- You cannot delete a branch if there is an open PR from it.
- Switching to GitHub Issues seems to be less controversial
I wish that was true. While most focus of PR vs Phab is around patch sets, GH issues have a lot more shortcomings than bugzilla (by design, I assume), so there are a lot more things to worry about than PRs.
However, I'd also refrain from commenting about bug tracker on a code review thread. We can only handle so much controversy at a time. :)
I think having a bot enforce that presubmit tests run, with documentation on how downstream consumers of LLVM might help set up bots to help participate in the CI presubmit, might be nice. The bot could work as a "merge queue" in that patches must pass CI before being merged, and that the bot alone is responsible for merges, of patches passing CI in FIFO. Also, having some dedicated reviewers could help replace "herald" from phab, AND help new contributors get reviewers auto assigned. (I suspect many newbies don't know who to ask for code review of their initial patches; having a bot analyze git log for the most recent contributors (akin to what the Linux kernel has a script for; scripts/get_maintainer.pl) might be better than this round robin stuff.)
I've used I don't know how many code-review systems over 40 years. They all fail in one way or another. Moving to GitHub PRs will if anything simplify my life, as I'm already used to GH Enterprise for my downstream repo.
Migrating to GitHub PRs (and Issues) is inevitable. It's how the open-source world works nowadays; let's go ahead and bite the bullet. But let's also not shoot ourselves in the foot.
- Work on bot infrastructure FIRST. It's eternally painful to have every single push send me a half-dozen or more bot-fail messages.
- Move bugzilla to Issues SECOND. Bug numbers are embedded in too many places, they need to be preserved.
- Move off of Phabricator LAST. Phab hates me with a deep abiding passion, but I've learned how to appease it and get the minimum necessary done. We'll want to keep it available in archive mode forever, but at least links to it from commit logs are URLs and not just numbers, so it's okay to have a new numbering scheme afterward.
@pogo59 that sounds like a good plan. I also support moving to github, both issues and pull requests, but the work to get there isn't trivial and I would hate to rush this and make things worse in the end.
I think that in moving to pull requests the bots sending mass emails problems should get easier. Most PR testing systems support bots commenting in the PR instead of emailing so the audience is just people subscribed to the PR.
Also with the expansion of pre-merge testing the number of failures post-merge should drop dramatically.
I strongly support moving to GH PRs. We've been using them in the CIRCT project from the beginning. Yes, there are downsides mostly stemming from GH's desire to simplify things; but, I personally haven't come across anything which I needed to do which just isn't possible. Our project is far smaller and far less complex than LLVM, so mileage may vary.
- GH now has a CLI tool. I've not used it much, but it's got a ton of functionality. Presumably, it's just a thin wrapper on top of the very full featured GH REST API. Could be useful for anyone concerned about reviewing productivity. https://cli.github.com/
- A killer feature of said CLI (IMO) is the ability to 'git checkout' the PR which I'm reviewing -- even if it's off a fork. This enables running tests locally and using my editor to crawl around code with which I am unfamiliar but the PR uses. Yeah, there's probably a way to get
arcto do the same, but I'm scared ofarc. - There are alternative interfaces to GH PRs. There are also some alternative PR review systems which use GH PRs as a backend (e.g. https://www.pullrequest.com/) so as to maintain some compatibility.
- GH workflows can be triggered on PRs (and pretty much anything https://docs.github.com/en/actions/learn-github-actions/events-that-trigger-workflows). This could be useful as an escape hatch.
- There exist a ton of actions that can be run in a workflow some of which to do useful stuff on each PR. (e.g. identify the correct reviewers based on the files modified). There are a few gotchas involved though.
- A bunch of IDEs/editors have integrations with GH PRs.
Some downsides:
- The GH rules for email notifications are either non-existent or overly obtuse. I looked into setting them up for myself a while back, but it was easier just to use email filters.
- Not-unrelated: the GH hosted agents (on which we may want to run actions) run on a very small VM size. While larger ones are supported on private repos, AFAIK they are not yet supported for public ones: https://github.community/t/github-hosted-runners-vm-size/136482. Setting up our own would be fraught with security and setup issues. The CIRCT project uses GH workflows on GH hosted agents to do all of our validation builds. Doing an MLIR build is downright painful -- it routinely takes 1.5 to 2+ hours for a 'unified' build. Fortunately, we cache the MLIR build so our PR checks don't take that long to run.
Overall, I think the network effect is extremely beneficial and probably the best reason to switch. If there's a feature we need, we could likely add it via either a GH workflow or a bot. I've also noticed a significant acceleration in GH feature velocity over the last few years, so things are just going to get more featureful. To that end, GH does publish their roadmap: https://github.com/github/roadmap/projects/1.
- The GH rules for email notifications are either non-existent or overly obtuse. I looked into setting them up for myself a while back, but it was easier just to use email filters.
For what it's worth, GitHub actually has documentation on the special [email protected] CC addresses that you can use for setting up your own email filters: https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/configuring-notifications#filtering-email-notifications. This doesn't beat having a more sophisticated way to configure email notifications, but at least it does give you something a bit better to filter on than just the email subject.
I'm strongly against moving to PRs at this time, for reasons I'll detail below. I don't think any of my concerns are unsolveable necessarily, but if we move without addressing them, I'll really not be happy, and my ability to review contributions will decrease. This is particularly important on both counts, because I'm currently not even assigned to an LLVM-related team in my company, so I can't justify spending more than a relatively small amount of time reviewing things upstream, and I'm also one of about 2 people who currently reviews changes in most of the LLVM binutils, so my review bandwidth is critical to development in this area.
Also, for context, my company uses Github Enterprise internally, and has done for a number of years. I feel like I have about equal experience in both GHE and Phabricator, and the latter is my preferred review system by an absolutely huge margin.
What features or properties make Github Pull Requests better than Phabricator?
- Integration with other infrastructure, such as Github issues, and the main repo definitely are useful.
- UI options for directly merging in changes are useful (although I find I only use them about 20% of the time in my internal company work, preferring the command-line instead).
- New contributors will often be more familiar with PRs with Phab reviews (but not always).
- No "missing context" due to people forgetting to add -U999999 to their diff generation.
What features or properties make Phabricator better than GitHub Pull Requests?
- For new contributors who have no familiarity with Phabricator or GH PRs, I think Phabricator's entry barrier is actually lower as things stand as there are fewer things that need to be got right. Certainly, it didn't take me very long to get up to speed with it.
- Patches don't have to correspond to branches: I often do my work in a feature branch, with a Phabricator review per commit. This makes updating individual commits easier for me. It also means that where I have a small piece of work, I can just have a commit directly on top of main, without needing to update things, which is simpler than going through the branch creation process.
- Github PRs don't really provide any true commit stacks, for patch series, where you want to keep the reviews separate: instead, you have to have a branch off a branch off a branch off ... which has the potential to get confusing very quickly. There's also no at-a-glance view option of what the patch series looks like in such a model, whereas in Phabricator the "Stack" button shows all linked-together patches.
- The comments in Phabricator don't disappear, unless the file they were in is completely removed from the patch (even then they are still available in the history). In Github, they often "hide" in various odd places, due to being "out-of-date" when the relevant area is moved. In Phabricator, there is only one view of the whole patch, with inline comments always being visible unless explicitly hidden by the reviewer. Yes, they sometimes move and lose some context, but it's usually possible to figure out where they're from. This point is perhaps my biggest complaint against Github. It is much easier in Github to miss comments, and much harder to keep conversations in an area going.
- Comments in Phabricator can be placed anywhere in the available context. In Github, they can only be placed within a few lines of modified code. Sometimes, this makes commenting where a comment is necessary impossible.
- It can be easy to not be looking at the full patch as planned to land in Github, due to it sometimes deciding to show you only the latest commit. Whilst the same is possible in Phabricator, I find it much less likely to happen.
- Both provide the ability to view diffs in the history of the review, but in Github, this only is possible if people haven't force-pushed changes, yet it seems to be a common workflow to do so. In Phabricator, the History tab provides the sequence of diffs that have existed for the patch, and it is sometimes useful to go back and see the differences between two specific stages, neither of which are the latest. I feel like this isn't as straightforward in Github.
- Github allows a person to mark an item as "Resolved", but if memory serves me rightly, this prevents further commenting on the comment chain, which is inferior to the "Mark Done" option in Phabricator.
- Having to switch tabs between reviewing files and discussion comments is a too-many clicks issue, and also makes it harder to easily flick between things. I often end up with two separate browser tabs when reviewing GHE PRs. This is exarcebated by the "disappearing comments" issue.
What new workflows or process improvements will be possible with GitHub Pull Requests?
- As noted above, the merging options can make integrating patches simpler.
- Sometimes, dividing a single review into multiple commits, is useful, as each commit can have its own comments etc, and this can only be done in Github.
Which workflows aren’t possible with GitHub Pull Requests?
- Patch series are more intuitive in Phabricator.
- Auto-subscribing/auto-reviewing seems to be more straightforward. In partciular, if memory serves me rightly, there is a hard limit on the number of reviewers in our GHE instance, which may be problematic in some cases.
- Which workflows aren’t possible with GitHub Pull Requests?
Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)
PRs will get reviewers added automatically for specific files/folders if you have a CODEOWNERS file in the repository.
- Which workflows aren’t possible with GitHub Pull Requests?
Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)
PRs will get reviewers added automatically for specific files/folders if you have a CODEOWNERS file in the repository.
Within LLVM, there's a big difference between code owners and reviewers. I'm automatically subscribed as a reviewer to several areas of code, but I don't consider myself a code owner of these. Also, IIRC within our GHE instance at my company, when trying to add reviewers to a list that had been prefilled with many code owners, several of the existing reviewers were removed (or new ones not added) due to there being a relatively low limit.
A negative against arcanist: A newcomer may have to learn both the idiosyncrasies of git (which I think most would agree is not straightforward) compounded against those of Phabricator and arcanist. One specific harm of this is that it makes it harder to pull down a patch from Phabricator without mangling something or ending up in a strange git repository state. Simple enough for an experienced person to fix, but a barrier to entry for a newcomer. Phabricator's tooling, documentation and ethos is playful in admitting that it is 'arcane' but this is not practical and utilitarian when it comes to growing a community. Hosting on GitHub is better in this regard since it exposes pull requests as part of the usual 'git push/pull' mechanism that everyone learns if they use Git anyway.
As a fairly newcomer to the project I vastly prefer GitHub Pull Requests because of the ease of use and the familiarity. Phab was a huge obstacle and learning curve for me. I have not been able to install arcanist since it depends on PHP.
- Which workflows aren’t possible with GitHub Pull Requests?
Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)
PRs will get reviewers added automatically for specific files/folders if you have a CODEOWNERS file in the repository.
Within LLVM, there's a big difference between code owners and reviewers. I'm automatically subscribed as a reviewer to several areas of code, but I don't consider myself a code owner of these.
Well that depends on how you interpret "code owners". In GitHub it just means someone that normally reviews changes to a particular file, folder etc. I would be added for the Arm backend, for example, as my colleagues add me as a reviewer to their Arm backend patches.
- Which workflows aren’t possible with GitHub Pull Requests?
Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)
PRs will get reviewers added automatically for specific files/folders if you have a CODEOWNERS file in the repository.
Within LLVM, there's a big difference between code owners and reviewers. I'm automatically subscribed as a reviewer to several areas of code, but I don't consider myself a code owner of these.
Well that depends on how you interpret "code owners". In GitHub it just means someone that normally reviews changes to a particular file, folder etc. I would be added for the Arm backend, for example, as my colleagues add me as a reviewer to their Arm backend patches.
Right, but my point is that the LLVM definition doesn't match GitHub's (see https://llvm.org/docs/DeveloperPolicy.html#code-owners) - code owners perhaps should be reviewers on all patches (even that's debateable), but not all people who want to always be a reviewer are code owners.
While I always hated the GitHub interface for reviewing code and still do (for reasons others have mentioned so I won't repeat), I will of course adapt to whatever tool the community chooses.
One thing that seems quite concerning to me though is that I have seen several mentions that "force push will be inevitable". Although I don't understand why that would be, I always think that using the force option is an indication that something has gone terribly wrong and needs to be fixed. So I am honestly concerned/afraid of any process that routinely requires using the force option.
Also, I find branches to be a very useful concept when there is a very limited number of them (i.e. main and release branches that we have now). However, once everyone needs to push a branch to get their code reviewed in a project as large as LLVM, the number of branches is essentially guaranteed to be so high that in my opinion, the whole concept of branches becomes rather useless. But of course, this is just my opinion and many people on many projects have found ways to work effectively in the presence of so many branches.
One thing that seems quite concerning to me though is that I have seen several mentions that "force push will be inevitable".
I think this was only about non-main branches and patch sets, which was a side discussion on how to make that work with multiple branches. I seriously hope this does not become the norm.
The main branch is protected against force pushes.
Also, I find branches to be a very useful concept when there is a very limited number of them (i.e. main and release branches that we have now). However, once everyone needs to push a branch to get their code reviewed in a project as large as LLVM, the number of branches is essentially guaranteed to be so high that in my opinion, the whole concept of branches becomes rather useless.
I concur. With hundreds of active contributors, we could easily end up with thousands of branches at any given point, and we'll need branch naming conventions, and name policing and cleanups, and the maintenance doesn't scale.
I believe developers should keep their internal structure to their own repositories and only expose it via a pull request from their own local branches.
If they want more branches to create dependent development, but want to publish PRs that can be merged, then they should merge multiple branches into a single one (on their repo) and create a PR on that branch.
If the need is only to inform (ex. this is what the next stage looks like), then they can just add a link to their own branch from the first PR, and when that gets merged, they rebase and create a new PR from the second branch, etc.