Code Review Process Update

[preames]

Just my 2c.

Personally, I don’t care much for github’s code review tools and believe phabricator is significantly easier to use. Given that, I expect this transition to negatively impact how much code I actually review. Despite that, I also believe we have no practical choice here and that moving to github code review is the best of a set of bad options.

The point has been raised that LLVM could sponsor development to maintain phabricator long term. Worth noting, we have already effectively been doing this for a couple of years already. However, I have heard a conspicuous silence from the folks who’ve actually been doing this work on volunteering to keep doing it. I take that to mean they are not volunteering to continue.

Given that, for phabricator to remain a practical answer, one of two things must happen:

1. We must find someone new willing to maintain it. As Chris has already challenged, if you think this is you or your organization speak up. However, to me, the silence on this point has been rather telling.
2. The LLVM foundation could pay a contractor to maintain it. For reasons which remain a bit unclear to me, the board has never seemed to serious consider this option. I suspect this comes down to the donors who actually provide said funding not thinking this is worthwhile.

We’re left with a situation where phabricator is not a viable option going forward. Accepting that premise, github’s code review tools feel like a reasonable default option. I would be perfectly willing to consider other options too, but when we have an obvious default which is “good enough” the burden for selecting something else is high.

I suspect we will have to adjust some workflow pieces - e.g. how we handle multiple patch series - but everything I’ve heard so far is fairly on the margin, and any tool we move to will require some adjustment.

Again, just my 2c, solely because there’s been debate about what silence means.

[mehdi_amini]

@preames

However, I have heard a conspicuous silence from the folks who’ve actually been doing this work on volunteering to keep doing it. I take that to mean they are not volunteering to continue.

I will continue to keep Phab running (with my colleagues and potentially any other volunteers), but I also fully support a plan to move to GitHub PRs: and not just because I don’t want to maintain Phab forever, but because there are other real advantages to it (in particular around tooling support pre-merge CI for example).
I’ll be happy when we pull the plug on Phab. In the meantime I’m also happy to keep Phab running as long as strictly needed to ensure that the transition isn’t rushed.

[jyknight]

As someone who does not favor GitHub PRs in general – and who has not been involved in this decision at all, I think the Board has made the correct decision based on the evidence currently at hand.

1. Unless there’s an about-face from upstream, we cannot stay with Phabricator. It is unmaintained, and while we can keep it limping along for a while, we must migrate to something else. The “community-maintained” fork which was announced also seems to have no maintenance. (Sadly, it appears even more dead than upstream!)
2. The “simplest”/“default” option is to switch to Github PRs. Absent compelling evidence that a different solution is preferable and viable, that is what we should use, and it makes sense that the board has chosen this.
3. Nobody has provided a concrete viable alternative. There’s plenty of comments about how GH is lacking, but knowing that GH lacks features is not enough, itself, to make the case that a different non-phabricator option is preferable.

I would like there to be a viable alternative worth considering. I would, personally, be in favor of switching to Gerrit (an open-source code review tool), “instead of” GH PRs, and have posted the same in previous discussions. (note that the “instead of” is only for the review – it imports a PR opened on GitHub, so that drive-by contributors may still use a PR workflow)

However, I’ve not done any legwork to demonstrate that this is viable, and that it would actually be a better option for LLVM – nor would I even say I myself am convinced it would be. All I can say is that I think it probably would be better, if it is viable, has good support, and good integrations. So, even if I were on the Board, I also would not, based purely on evidence currently available, recommend switching to Gerrit.

but if there were a specific proposal and it met our general goals then of course it would be considered.

I really appreciate you saying this.

While I kind of wish the Board themselves would have done an in-depth investigation of other alternatives, that doesn’t appear to have been on the agenda. (And I do think it’s reasonable to say that folks who really want an alternative solution should be on the hook to investigate and propose such solution themselves.)

So, with a year’s warning, there is now a period of time where anyone who feels like that is a seriously wrong decision can present a concrete proposal for a viable alternative. (I do not plan to do so myself).

If someone else does want to investigate Gerrit, and make such a case, I’d recommend starting by looking into gerrithub.io (this is an instance of Gerrit for open-source projects, run by the company gerritforge.com, who also provides paid commercial gerrit hosting). I don’t know much about this company, but they seem like one of the only options other than self-hosting a Gerrit installation. (And if LLVM project self-hosts Gerrit, who’s actually going to be responsible for running it?)

IMO, the only other potentially-viable option other than Gerrit would be to switch to GitLab and use its review system. While this is likely technically viable, switching from GitHub to GitLab would be a proposal that would require some really really persuasive justification…and I cannot actually imagine that existing at this point.

[h-vetinari]

@mehdi_amini

This contradicts the fact that the same points you agree with are suggesting to use incremental commits to address reviews: if you have dependent changes in separate commits then addressing the feedback of each commits with new commits in the same PR seems totally unmanageable.

I don’t think its contradictory^[1] but I don’t want to sidetrack in this discussion that has played out so many times already. I just thought perhaps the viewpoint of an outsider-and-prospective-contributor might be interesting.

---

1. e.g. using --fixup or manually doing the equivalent; also I really didn’t rule out separate PRs where reasonable, as would be the case if vastly different reviewers are needed. ↩︎

[RKSimon]

Sorry if I’ve missed something, but how do we intend to keep phab running in a read-only state for X years after any transition to retain history for triage etc? Similar to the bugzilla archive.

[mstorsjo]

For me, as already stated by a bunch of others - the handling of incremental revisions to a multi-commit branch is the key issue.

Whenever we switch, the tooling and guides need to be extremely clear (and strict!) about how that should be handled. Both of the extremes that can end up merged accidentally are terrible:

• Merging a dozen of “fix, tweak, tweak some more” commits
• Squashing a bunch of individual commits with long verbal rationales and explanations in their commit messages, down to a single commit consisting of nothing more than the title of the PR

Edit: And as rationale - while others say they have experience in working in successfully in large projects on GitHub, I have similar negative experience: Preparing a branch consisting of a couple commits, doing a multi-step refactoring in small easily reviewable steps, each carrying a multi-paragraph explanation of the step and its motivation and impact. Once put up for review, the reviewer has out of habit pressed “Squash and merge” - and it’s all lost (at least for people looking at a clone of the repo) - both the commit-level split clarity of the sequence is lost, and all the explanations.

[mehdi_amini]

@RKSimon

Sorry if I’ve missed something, but how do we intend to keep phab running in a read-only state for X years after any transition to retain history for triage etc? Similar to the bugzilla archive.

I suspect we’ll archive a static mirror: that is only the HTML pages available at the same URLs. This would be so that we can shutdown the database, etc. That said it is TBD.

[jcranmer]

I want to reiterate that I have yet to see the two questions that I raised addressed, and I think it is important to address these questions:

What alternatives were actually considered, and why were they rejected? Already in this thread, a few alternatives were suggested, and several more were discussed in prior threads. If these alternatives were rejected, it would be instructive to know why. And if the only reason they are not being considered is because no one has explicitly offered to resource them, well, it feels premature to say that it lacks resources when no one ever asked for those resources in the first place!

The second question is what is the actual flexibility of the current commitment. In particular, would this decision actually be revisited if someone proposed an alternative (and what is the burden of proof to cause a revisit)? Furthermore, should GitHub prove to be more sluggish than expected in making changes that we request to support features we need, would we be willing to push the date back? Which changes are actually considered necessary to commit to a move?

To me, it feels like this process has been done backwards: the decision was pre-made to move to Github PRs, and the only real question is how to jam a tolerable workflow on top of that; instead a better approach would have been to first consider the actual workflow we want to go through for code review and committing, and then evaluate alternatives based on how well they do or do not support this workflow. To this end, I would call out specifically Gregory Szorc’s excellent blog post [1], which suggests that the workflow we should strive for is one that seeks to break changes up into small, atomic but related changesets for review, and this workflow is one that is almost completely at odds with the current GitHub interface, and I personally do not believe that GitHub will support this workflow any better anytime soon, much less the one year timeframe currently indicated.

[1] Gregory Szorc's Digital Home | Problems with Pull Requests and How to Fix Them

[kparzysz-quic]

“Squash and merge” by default will concatenate all commit messages into one long message. It actually opens a window where you can edit the message before committing. I don’t know what this function did in the past, but that’s the current behavior, so the scenario you describe has a lot lower chance of happening by accident.

[River707]

I actually consistently have a bad experience with the way the messages are constructed by default in “Squash and merge”. From my experience, it’s really easy to end up with “bad” commit messages because people often just hit merge without cleaning up the auto-generated message. The final message ends up containing lots of “* small fix”/“* oops lol” messages, which is horrible.

[mstorsjo]

Hmm, ok, maybe I do misremember about the commit message aspect then - but the individual commit level separation was lost in any case - due to reviewers being used to squashing such changes from fixup commits, while they were meant to be kept separate in my case. And the messages that tried to explain the individual changes don’t make sense if all the diffs are squashed (e.g. if the commit series changes the same lines multiple times).

[kparzysz-quic]

@clattner

I would still love people in favor of the transition to actually say so

I’m very much in favor of this transition. The primary reason is actually that our source code is hosted on GitHub, and it seems logical to use the rest of it as well. I think it looks better for the people coming to the project to see a unified solution rather than a patchwork of different tools.

I have been using gerrit, GH, and phabricator, and it just happens that personally I like GH the best. Phabricator is out, and (in my personal opinion) gerrit is just horrible.

[clattner]

Great question jcranmer, I think there are also some other potential common confusion about what is going on here. Again, speaking for MHO only, a few thoughts:

1. I see a lot of scars given how previous transitions were handled. We can acknowledge previous problems, but the best we can hope to do is learn from them and do better. This is one of the reasons that this transition is over the course of more than a year. Please go into this one with a fresh perspective despite the previous challenges

2. The purpose of the deadline is to provide time to improve and fix the workflow issues that remain to make sure we don’t transition with the current state of things. This could involve Github implementing new features, it could also include adoption of existing third party tooling (e.g. a bunch exist for various stacked review flows, someone pointed me at this thing for monitoring PRs which looks nice, etc), and/or building our own if needed. Github has a huge community, so while change is tough, I suspect we’re not the first community to want stacked reviews and other things, so there are probably solutions out there.

3. of course the deadline will be re-evaluated given new information as it arises. We’re all pragmatic here. We do need to start making progress though, and we want to make this a proactive thing, and not a dragged out until we get to the deadline and everyone is unhappy. We want to “see it coming”, have time to deal with challenges, and manage this transition better than previous ones.

4. The foundation currently has no full time engineers/contractors to do this work. Changing that is up for discussion of course, but until/if that changes, we have to look to help from the community and elsewhere. Other suggestions welcome of course.

5. I know that transitions are hard, but really folks involved in the foundation are not intentionally trying to mess with anyone. No one wants to deal with this, even those driving things forward. We know it will upset people and burn time as we figure out new flows, muscle memory etc. The folks involved are honestly trying to figure out the best thing for the community at large, which is a very difficult thing, and painful particularly for things like this.

One of the things that is clear to me is that there are a few specific pain points (e.g. stacked PRs etc) that are common to many people. There are a bunch of potential solutions to this that have been explored by other communities. It would be awesome if interested folks could start a workgroup to explore this.

I really appreciate folks speaking up on the thread with their perspectives and opinions btw (whether in favor or not) it is great to get more people involved in this! Thank you all,

-Chris

[python3kgae]

@tstellar

October 1, 2023

Could there be a preview period so people can try code review on GitHub for llvm project before October 1, 2023?

Thanks
Xiang

[nhaehnle]

@clattner

One of the things that is clear to me is that there are a few specific pain points (e.g. stacked PRs etc) that are common to many people. There are a bunch of potential solutions to this that have been explored by other communities. It would be awesome if interested folks could start a workgroup to explore this.

I’d be interested in participating in an effort to explore whether/how we can solve the multi-commit reviews issue. (I hesitate to call it “stacked PRs issue” because that presupposes what the correct solution is.)

[tstellar]

I created an issue to track this. Step 1 is probably to define what we mean by "multi-commit’ so we can make a feature request to GitHub and then Step 2 is to plan out how to implement it with the existing functionality. Feel free to assign it to yourself and kick off the discussion. I believe @kparzysz-quic is also interested in this.

[jayfoad]

@clattner

One other thing I’ll add is that this thread is another indication that people who are grumpy with things often speak up, but folks that are happy with things often stay silent. I can speculate why, but I would still love people in favor of the transition to actually say so

As a reviewer I am in favour of the transition, and for exactly one reason: with github PRs the thing I’m reviewing is a real git commit that I can apply locally and play around with. With phabricator, downloading and applying patches locally is a significant pain point. For me this outweighs all other concerns.

Jay.

[Whisperity]

What will be the suggested solution for commandeering a patch? This does not seem like a feature GitHub supports, which is a crucial feature for long-running workflows where the original contributor is no longer accessible – mostly in our case a university student that has since graduated and started working for a company that is not interested in LLVM, or in the extreme case the student even fled the country… With Phabricator, we were able to take ownership of the change and keep existing discussion valid and in place, while on GitHub this would incur having to create a brand new pull request, make sure we do not forget linking to the old one, and then anyone reading the discussion will potentially have to jump back and forth continuously.

[tobiashieta]

AFAIK this is not possible and we would just have to remember to back reference the old PR and close it with a label that makes it clear it has been replaced.

This process should be documented and I suggest you file a issue in the project Tom linked to above so that we can remember to do that.

Personally - while commandeering would be nice it’s not really make or break feature.

[rengolin]

@AaronBallman

@clattner

One other thing I’ll add is that this thread is another indication that people who are grumpy with things often speak up, but folks that are happy with things often stay silent.

FWIW, when I see someone who has made the same point I want to make, I hit the heart button or stay silent and assume that’s sufficient because the point was already raised. The more prominent the person making the comment, the less compelled I feel to reiterate the point. So I think it’s dangerous to assume that silence is acceptance.

To @clattner’s point: I am in favour of moving away from Phabricator (never liked it and it only got worse), and GH is one way. To @pogo59’s point: I also have used so many infrastructure things that I’m sure I’ll cope, like we all coped with, and some even liked, Phab.

@AaronBallman is spot on when he says the heart things are to be used for that purpose and I don’t like that this is the only reaction. There are other positive reactions that can be used with different meaning. LinkedIn has an attempt at that (FWIW).

I am aware Phab has features we’ve all grown accustomed to (herald, stacked) and it will be weird (at least at first) to do that in GH. We’ll need some mix of tooling, documentation and collective good will to get used to it. We’ve done it before, we can do it again.

@clattner

massive companies use and contribute to LLVM. That said, in practice, no one has stepped up to help with things like this.

None of the companies that work on compilers (and already pay the upstream price, against legal/marketing pressure) want to add infrastructure on top of that. Most of those companies already struggle with infrastructure locally, so…

@clattner

Similarly, if you or your organization has connections into Github, then please use them to get improvements.

Github has a strong focus and cannot be swayed. @akorobeynikov and I have tried for many years, and “success” came at random times for what seems to be random reasons. I wouldn’t rely on them doing something because it makes sense and it’s easy.

@tstellar

To do this, we set the target transition date to October 1, 2023. Allowing over a year should provide enough time to adapt, and time to find workarounds/solutions for at least some of the deficiencies that remain.

This is a good time frame. Sets the drumbeat without pushing people around. Much better than the mailing list move, thank you all for listening to our concerns.

@mehdi_amini

I also would have preferred that the date was announced as a tentative one / a soft commitment, with a pre-condition to the move that the proposed future workflow would be documented and got through an RFC in front of the community first.

Quite so. However, despite some strong reservations from key people (representative in this thread), most people do want to move away from Phab and accept GH as the destination, so I’d do an RFC on “what are the necessary steps to enable GH and disable Phab”, not rehash the previous RFCs.

But I think we can use the current project as a base for that RFC. In theory, once all issues on that list are done, we collective agree it’s “good enough”. I’m assuming this is what @tstellar meant to do with the project thingy, so in a way, we are doing a technical review in the community, just not in a mailing list kind of way. IMO, it’s better this way.

I don’t speak for other people, so @AaronBallman @jcranmer @mehdi_amini, I’m hoping you guys would be (at least) ok with solving the main problems (herald, stacked, etc). If not, perhaps we could use the project to iterate through more issues?