Logistics of the transition to GitHub PRs for libc++/libc++abi/libunwind

ldionne · August 30, 2023, 5:27pm

Hi folks,

As everyone should know by now, LLVM is moving to GitHub Pull Requests (see this). Like the rest of the monorepo, we will start using GitHub Pull Requests for all new libc++/libc++abi/libunwind patches on September 1st 2023. Furthermore, Phabricator will become read-only on October 1st 2023 and we need to prepare for that.

We currently have a significant review queue in the runtimes. We have a lot of outstanding patches in the queue which can be roughly be categorized as follows:

Patches that haven’t been reviewed: those are blocked on libc++ reviewers
Patches that have been reviewed but feedback needs to be addressed on them: those are blocked on patch authors
Patches that are accepted and need to be merged (sometimes with minor feedback): those are blocked on patch authors

First, we need to make sure that we don’t leave important patches behind during the migration. To this effect, I would like all frequent contributors to join forces and clear the review queue. I will be triaging reviews based on what I think the required action is and dispatching them to folks. Furthermore, I would like everyone to focus on clearing their patch backlog instead of creating new ones. I understand that this might not be 100% realistic, but please try to do your best.

Second, I think it is important to find a way to avoid getting into the same situation in the future with GitHub PRs. Indeed, having such a long review queue makes it incredibly difficult to prioritize which reviews to do first and that causes all kinds of problems (and frustration). I am not 100% certain how to achieve this because there are clearly too many patches to review for the total amount of review time we are able to provide. However, we could explore something like requesting that for every created patch a review must be done as well. We could also consider systematically closing PRs for which there has been no activity in some amount of time. Another possibility would be to always process PRs in order of “oldest first”. I’m interested in people’s thoughts on this, but I really think we need to find a way to bound our review queue going forward.

Thoughts?

Louis

philnik · August 30, 2023, 7:15pm

I think there are quite a few ways in which we can improve the review queue.

We should actually mark patches are requiring changes when we requested some. I think there are quite a few patches which never got updated but are still in the queue, making it seems much longer than it actually is, and making it harder to find patches which actually need review.
We should assign individuals to patches (probably self-assign). It happens quite often that someone comments on a patch at some point but doesn’t actually plan to review it for various reasons (not enough time, not familiar with the code, etc.). That makes it sometimes hard to know whether there actually is someone responsible for reviewing a given patch. Having a board or something similar where people list the patches they plan to review would make things a lot clearer. That would also make it a lot harder to forget that you reviewed a patch.
Adding labels to reviews would make it a lot simpler to find patches which you have expertise in. I’ve spent a bit of time triaging all the libc++ bugs and have added quite a few labels. Adding them systematically to patches would make it a lot simpler to quickly search for interesting/important ones.

yronglin · August 31, 2023, 4:21pm

I think the recent active patches should be prioritized, they may be easier to advance to the next process.

Zingam · September 2, 2023, 5:37am

Given the short deadline. This is a bit surprising as LLVM docs page still refers to Phabriractor as the means to submit patches.
https://llvm.org/docs/Contributing.html
Updating that documentation. Instructing to submit new patches as GH PRs and also instruction how to convert local repositories into forks, would have been nice to have.

It looks like the last is possible as described here:

gist.github.com

https://gist.github.com/jagregory/710671?permalink_comment_id=4444019#gistcomment-4444019

gistfile1.txt

So you've cloned somebody's repo from github, but now you want to fork it and contribute back. Never fear!

Technically, when you fork "origin" should be your fork and "upstream" should be the project you forked; however, if you're willing to break this convention then it's easy.

* Off the top of my head *

1. Fork their repo on Github
2. In your local, add a new remote to your fork; then fetch it, and push your changes up to it

    git remote add my-fork git@github...my-fork.git

This file has been truncated. show original

I have a couple of patches I have submitted (and I few I haven’t) and I hope to get them into LLVM18 but I highly doubt that they’ll make it before Oct 1.

Should we wait for Oct 1 or resubmit the patches to GitHub before that?

smeenai · September 2, 2023, 5:46am

[Docs] Update documentation for the new GitHub workflow by tru · Pull Request #65162 · llvm/llvm-project · GitHub was committed with documentation for GitHub PRs, and the website should reflect it, although some places might have been missed. LLVM GitHub User Guide — LLVM 18.0.0git documentation explains the PR workflow.

ldionne · September 5, 2023, 1:11pm

Can you ping us on Discord about these patches? We can see whether we’re able to get them reviewed soon or whether we need to re-upload them as GitHub PRs.