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
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
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.
3 Likes
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.
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
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
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
@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
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?)
14 Likes
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.
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
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
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
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
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
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
Commits are reported to the lists; I suspect we wrote a hook for that, because the email goes to different lists depending on which top-level directory is involved. I don’t think github would do that kind of thing on its own.
I expect to get PRs to send list mail, we’d need to have a listbot user of some kind, which we could then auto-subscribe to reviews. My team is using GitHub Enterprise internally, and we auto-subscribe people, but I don’t know that we can set up a mailing list that way.
Edit: Added an issue for this as requested by @tstellar
We have identified some gaps in this thread, it would be helpful if people could file issues for them so they can be tracked. There are currently issues for some of them, but not all.
This is our current list of issues, if you file a new one please add it to the Pull Request Migration project.
I’m glad we’re tracking the issue (Tom posted a link in another comment) as this is pretty important functionality for code reviewers.
Ah, thank you for that! This could potentially work out.
I think we need some of this to be documented up front (things like how to properly merge PRs when they’re ready, whether we should squash, etc) and some of it is fine to document as we learn more (how to handle GitHub’s lack of utility as a code review tool). The “we” I had in mind was the folks driving the infrastructure changes. Basically, I want to make sure we have enough documentation in place that active reviewers who are unfamiliar with the GitHub workflow can still succeed in reviewing and landing code without a fear of “doing something wrong.” We don’t want folks hung up on “I don’t know the correct way to land this” kind of problems, basically.
There’s two aspects to my question: 1) when someone makes a new PR, does that PR go to the mailing list? 2) when someone lands a PR, does that commit message go to the mailing list? FWIW, both of those are critical pieces of functionality for awareness of what’s happening in the project at the code owner level.
Ah, thank you for the clarification, that’s good!
I know that it’s very common in Clang for code reviews to take longer than a month from start to finish. I also know that we don’t want to kick the can down the road indefinitely (there are code reviews open that are years old at this point, and they sometimes get resurrected). So I’m not certain what the best way forward is for this. I suspect going to a read only mode will be disruptive for some amount of people, but I have no insight into how many folks would be impacted. The big downside of a hard deadline is: there’s no transition option for reviews, so recreating the patch in GitHub means losing all previous discussion on that patch. For long-running reviews, that could be a real problem.
This is a pretty significant change and I don’t think downstreams are used to tracking announcements on Discourse yet (I know some announcements have caught folks by surprise before, at least). Perhaps this is a good time to determine how to communicate major changes with downstreams? I think it’d be healthy for us to have explicit communication channels with major external stakeholders for these kinds of decisions given that they have far-reaching impact.
I’m glad to hear there’s nothing on fire currently and there’s a willingness to delay if needed (just the same, hopefully we don’t run into major roadblocks).
Same!
Excellent, thank you!
1 Like