Update on GitHub pull requests

Project Infrastructure Code Review
1

Hey all, @tstellar and the LLVM foundation board members had a call to discuss the pull request migration timeline last week. Tom outlined the migration timeline here:

  • Today: Open up pull requests for small self-contained teams to experiment with.
  • September 1: All new patches submitted via GitHub Pull Requests only. Phabricator commit tracking disabled. Existing reviews continue on Phabricator.
  • October 1: Phabricator becomes read-only.
  • October 1, 2024: Phabricator server shutdown. (We’d want to have an HTML archive at this point)

I wanted to share that update more widely with the community to make sure that folks are aware that this is making progress. If you have remaining concerns about the migration, take a look at the documentation that Tom has put together to see if they have been addressed. If they are still not addressed, now is the time to discuss them or prepare to make changes to any workflows that depend on Phabricator.

12 Likes
2

Is that documentation referring to LLVM GitHub User Guide — LLVM 18.0.0git documentation or something else? There’s no mention in there of what stacked diff workflow we’re going to be using with PRs, or what merge styles will be enforced (vs. just suggested via documentation).

3

IMO it would be a good idea to post something in announcements and link to this thread. That would help to get a bit more visibility.

I have a few questions

  • On October first Phab becomes read-only. What does that mean for reviews in progress in Phab at that time? In libc++ we have reviews that take more than a month to complete.
  • Is there any official documentation on how reviews on GitHub are being conducted and how to do stacked reviews?
1 Like
4

We haven’t reached consensus on what the stacked diff workflow will be yet. There is an issue open for working on this.

1 Like
5

To make it public:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork

6

And for people (like me) who are unclear on the “fork” concept:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks
(especially the “About forks” topic)

I’m notoriously bad at navigating these web UI thingies. But I know one time I created a fork and a PR for another project, and I’m moderately sure those documents were sufficient, which is a +100 from me compared to the misery I went through with Phabricator. Making sure we point to them from the LLVM web pages would be good.

4 Likes
7

There is a link in our GitHub User Guide to the official GitHub documentation about creating forks / pull requests. We can add other links to other useful documents too.

8

Just a tip for anyone else that doesn’t like clicking around the web ui, GitHub offers a really excellent cli that can create forks, PRs, merge and do much much more. Check it out: https://cli.github.com/

7 Likes
9

Not only haven’t we reached consensus, but discussion has completely stalled on there and there is no indication anybody is actually trying to resolve the issue, which means at this rate we’re going to go ahead with the migration without any solution or even a pathway to a solution.

I would love to drive that discussion further forward myself in some manner, but I don’t have time to contribute to LLVM beyond the brief comments I make on these mailing lists and the hefty pile of reviews I have in my backlog.

7 Likes
10

I remember that there were a couple of alternatives/GH integrations suggested in the initial thread that would provide UX closer to that of Phabricator. Was some conclusion reached on them ( Investigate alternative interfaces for GitHub Pull Requests · Issue #56641 · llvm/llvm-project )?

4 Likes
11

@danilaml Those alternative interfaces are available if we want to turn them on. If you are interested, I can help try to set one of those up for our repo.

1 Like
12

Some open questions that I can’t seem to find answers to (apologies if they are answered and I’m just not seeing it):

  • What is the replacement for Herald so that people can be automatically added to code reviews in their area of interest?

  • What is the replacement for Phabricator’s list of active/open reviews?

  • Will we be documenting best practices where we need a replacement for code review activities that are not supported by GitHub? e.g., best practices for leaving comments on lines of code that are unchanged in the review, reminding people that GitHub hides the most relevant changes in the review by default sometimes, what to do when your code review no longer loads because it has “too many comments”, etc

  • Are we changing permissions/processes as part of this transition? e.g., are code owners going to be required to sign off on a review before it can be merged or continuing to allow anyone to merge? Are we changing who can make labels or introduce milestones/projects in the issue list? etc

  • Do we have what we need to continue to report commits to the cfe-commits and llvm-commits mailing lists once we cut over to GitHub?

  • How are we preventing new patches from being submitted via Phab after Sep 1?

  • What is the plan for (active) open code reviews on Phabricator that are not completed by Oct 1? (Even with cutting off new patch submission on Sep 1, there will be patches that aren’t cleared by Oct 1.) I’m not worried about reviews that have gone stale from years ago as those can be recreated and linked-to from a new PR as appropriate.

  • Is the LLVM Foundation communicating directly with major downstream vendors to ensure that this timeline works for them?

  • What is the fallback plan if any of our steps runs into problems that we can’t solve on the expected timeline? Are we going to forge ahead regardless, or is there wiggle room to delay steps as needed?

  • We’ve switched infrastructure previously (email to phab for code reviews, svn to git, bugzilla to github, etc) so there’s every reason to believe we’ll switch again in the future; are we confident we can move our data back out of GitHub when we need to? I believe we can for things like issues, but I don’t have any insight into how plausible that is for code reviews (e.g., can we get a static HTML archive of reviews should we need to make this same transition again in the future?)

15 Likes
13

Well, reviewable looks nice and we have contributors familiar with it (@kbeyls), but it does look like something that could be seamlessly added latter and if I got it right it just improves the UX but doesn’t cover for some missing features (like stacked reviews). Another resource linked in the issue (graphite) claims to natively support stacked workflow, but I don’t know whether it’s close enough to what Phabricator had (didn’t need to use stacked diffs for my patches) and I don’t know how well it integrates with GitHub (haven’t seen it in the wild before and from the website it seems much more involved, personally I would want to sacrifice the “simplicity” of non-stacked PRs just to support this). I was interested whether anyone have tried them and if there was any decision made (looks like there wasn’t). Since my needs are mostly satisfied by the current PR UI, I think folks that were power users of some Phabricator-specific issues like stacked diffs need to chime in and evaluate those tools to see if they properly address them.

14

What is the replacement for Herald so that people can be automatically added to code reviews in their area of interest?

Anton gave some update on this topic, but my recollection is that we don’t have a great solution to this yet.

What is the replacement for Phabricator’s list of active/open reviews?

I searched “github dashboard” and found some docs referring to such functionality: “You can visit your personal dashboard to keep track of issues and pull requests you’re working on or following” Does that sound sufficient?

Will we be documenting best practices where we need a replacement for code review activities that are not supported by GitHub? e.g., best practices for leaving comments on lines of code that are unchanged in the review, reminding people that GitHub hides the most relevant changes in the review by default sometimes, what to do when your code review no longer loads because it has “too many comments”, etc

We should always document best practices. Who is “we” in your question? I don’t expect these practices and documents can be produced without more experience with pull requests. I hope that the subprojects who trial run code reviews will help us generate the necessary experience to produce these docs.

Are we changing permissions/processes as part of this transition? e.g., are code owners going to be required to sign off on a review before it can be merged or continuing to allow anyone to merge? Are we changing who can make labels or introduce milestones/projects in the issue list? etc

I haven’t heard anyone propose a change to the LLVM approval or permission model so far. I think the transition is limited in scope to just replacing the code review tool.

Do we have what we need to continue to report commits to the cfe-commits and llvm-commits mailing lists once we cut over to GitHub?

As far as I know, we already report commits to the lists. Maybe your question is, will every pull request send an email to a -commits list? I think this is an open question, I don’t have an answer.

How are we preventing new patches from being submitted via Phab after Sep 1?

I think the plan is to allow patch submission on Phab during the transition so that if the new process isn’t working, we can still fall back to the old process.

What is the plan for (active) open code reviews on Phabricator that are not completed by Oct 1? (Even with cutting off new patch submission on Sep 1, there will be patches that aren’t cleared by Oct 1.) I’m not worried about reviews that have gone stale from years ago as those can be recreated and linked-to from a new PR as appropriate.

Phabricator supposedly supports a simple readonly mode, and that’s what I propose we use. We could try to hack Phabricator’s PHP code to block new patch submission, and add an extra phase where comments are allowed before going fully readonly on Nov 1 or something like that. This is extra work, though, and it doesn’t seem necessary to me. Do you feel it is important, or would you rather extend the window of overlap to allow active reviews to finish?

Is the LLVM Foundation communicating directly with major downstream vendors to ensure that this timeline works for them?

Other than these announcements, I’m not aware of other direct communications.

What is the fallback plan if any of our steps runs into problems that we can’t solve on the expected timeline? Are we going to forge ahead regardless, or is there wiggle room to delay steps as needed?

I think the backup plan is to push back the Oct 1 Phabricator readonly cutoff date. If pull requests are really a showstopper, that seems like the most reasonable release valve.

We’ve switched infrastructure previously (email to phab for code reviews, svn to git, bugzilla to github, etc) so there’s every reason to believe we’ll switch again in the future; are we confident we can move our data back out of GitHub when we need to? I believe we can for things like issues, but I don’t have any insight into how plausible that is for code reviews (e.g., can we get a static HTML archive of reviews should we need to make this same transition again in the future?)

That’s a good question. I certainly link to Phabricator review threads to reference the comments and reasoning for our past decision making. I believe pull requsts are part of the github issue tracker, so any export of issue tracker data presumably includes pull request comments.

1 Like
15

This issue is being tracked here. We will likely need to roll our own solution for now.

1 Like
16

We can put a CODEOWNERS file a the root and all PRs touching the clang subdirectory must be approved by the @clang-owners team.

2 Likes
17

This is substantially different from Herald and would be a larger policy discussion. I also personally use Herald to follow along areas where I’m not an owner but still want to keep up to date, and I imagine many others use it for a similar purpose.

4 Likes
18

Any sort of approval requirements for pull requests would be a bigger policy change and something that should be discussed after the migration.

3 Likes
19

It also doesn’t work very well for folks whose ownership duties aren’t determined by a subdirectory but by a file. e.g., attribute code ownership means watching SemaStmtAttr.cpp, SemaDeclAttr.cpp, and a few other select files. Having to sign up for all of Sema is pretty onerous for that situation.

1 Like
20

The CODEOWNERS file supports subscribing to individual files. I think this is likely going to be the best solution for us to auto-subscribe to PRs.

1 Like