Opening up PRs experimentally for a subset of the LLVM project

Hi,

As it’s been discussed previously, LLVM will be moving from Phabricator to Github Pull Requests for code reviews on October 1st 2023. Making this transition smooth and successful will require some amount of preparation. In particular, it would be useful if the general workflow for review notifications, approval and pre-commit CI were fairly well understood before we open up the valves and ask all contributors to start submitting PRs.

Hence, I would like to request that a small subset of the monorepo be open to pull requests immediately so that we can start figuring out some elements of the Github workflow and smooth things out for the rest of the community. The official way of contributing to LLVM will still be the current Phabricator workflow until the transition. This is just a way to empower a few users to start figuring out how things could work. Specifically, I’d like to allow PRs to libcxx/, libcxxabi/ and runtimes/ to be open without being auto-closed. I am proposing these components because we are a small team, we are enthusiastic about the Github transition and we are heavily invested in our infrastructure, with pre-commit CI being extremely central to our workflow. This will allow us to:

  1. Set up a libc++ and a libc++abi team and figure out the notifications story for code reviews and issues
  2. Set up our pre-commit CI (we’ll most likely trigger our current Buildkite setup via Github) and see how this can work for other projects like Clang
  3. Figure out how we can migrate or exhaust our current pool of open code reviews in Phabricator to avoid just dropping contributions on the floor
  4. Figure out any other issues we find along the way

Once that’s done, we can report on our experience and this can be used as an input for the rest of the community to decide how they want to do things. Note that we won’t be opening up the door for all libc++ contributions to go through Github PRs – only a few contributors invested in our pre-commit CI setup would investigate that.

The goal here is to keep things centralized inside Phabricator until the transition, while allowing a small workgroup to pave the way so that the transition can happen on solid bases when the time comes.

Cheers,
Louis

17 Likes

Thank you for looking into that!

we will probably need to fork some of the workflows in buildkite for that

For people like me who lost track of the discussion (was there a panel or round table or something at the fall Dev Meeting, that I missed?) the relevant thread is here: Code Review Process Update - #121 by tstellar

(Which is in the LLVM Project category, not Project Infrastructure/Code Review, making it less easy to find than I’d like. Thanks to @jh7370 for digging it up.)

1 Like

Hi! Thanks for posting this! I think this is a good step to figure out some of the infrastructure things that needs to change before we migrate everything.

One possibility would be to use another repo similar to llvm-project-release-prs that is synced with the main repo and open the PRs there if we want to keep the separation a bit longer.

I’m in favor of this proposal. We will need some kind of trial period prior to moving the whole project, and I think starting with a small and well-defined subset of the community is the best way to go.

Doing this will also make it possible to tryout some alternative pull request interfaces, which may be helpful to people who find the default interface not intuitive or hard to use.

I think we are close enough to October that it makes sense to just start with the llvm/llvm-project repo. I also think we should consider moving the release pull requests to llvm/llvm-project for the LLVM 17 release cycle, since that cycle will overlap with the move and it might be better if we don’t have to change our process in the middle of the release.

Will linear history still be enforced for these PRs, i.e. requiring the rebase and/or squash merge options be used, not the true merge option?

2 Likes

Yes, we will still be maintaining linear history. This is enforced via branch protections/rulesets.

7 Likes

When I was writing the presentation for EuroLLVM I was thinking about this and thought that it would make more sense to have LLVM 17 use the current system and switch over to running on the main repository for 18 to avoid having multiple error sources for the release. Even if we start working on enabling PR’s for the main repo in the middle of the 17 release train, the old cherry-pick system should still work fine with the separate repo, and we would avoid unnecessary churn in the process during the release.

But if 1. October 2023 is a fixed data, we should slowly increase the PR traffic before the date to get experience.

1 Like

@tobiashieta No objections to that either. It’s up to you since you are going to be managing the release.