RFC: Code Review Process

Hi,

# Proposal

The LLVM Foundation Board of Directors is seeking comment on the current state of Code Review
within the LLVM Project and its sub-projects. Phabricator is no longer actively maintained
and we would like to move away from a self-hosted solution, so our goal is to determine if
GitHub Pull Requests are a good alternative to our current code review tool: Phabricator.

Specifically we are looking for feedback on:
- What features or properties make Github Pull Requests better than Phabricator?
- What features or properties make Phabricator better than GitHub Pull Requests?
- What new workflows or process improvements will be possible with GitHub Pull Requests?
- Which workflows aren’t possible with GitHub Pull Requests?
- Any other information that you think will help the Board of Directors make the best decision.

# Where to Direct Feedback

Please provide feedback on the Infrastructure Working Group ticket[1]. This will make
it easier to collect and consolidate the responses. At the end of the comment period
the Infrastructure Working Group will collect the feedback for further analysis and summarization.

# Timeline

The timeline for this RFC will be as follows:

- RFC posted on llvm-dev for public review and comment
- 30 days after the date of posting, public comment closes.
- IWG will have 14 days from closure of public comments to review and summarize public
   comments into a pros and cons list to be present to LLVM Foundation Board
- Foundation Board will have 30 days to make a final decision about using GitHub Pull Requests
   and then communicate a migration plan to the community.

Thank you,
LLVM Foundation Board of Directors

[1] https://github.com/llvm/llvm-iwg/issues/73

Hi Tom,

Please help me here, I think I’m severely misunderstanding what this means…

I’m reading it that the “Board of Directors” will make a decision and communicate to the community, apparently through some undisclosed internal process.

For example:

  • What about people that are on holidays during the 30 days comment period?
  • What if the points are not made clear after 30 days?
  • How do we know the IWG will correctly summarise the comments to the board?
  • How does the board guarantee it will take all facts in consideration without bias?
  • What kind of recourse would the community have if the decision alienates a large part of the developers?

Please understand that I’m not assuming malice at all. We’re all humans, and in the effort to make some change happen we quite often let unconscious bias be the merits of our decisions.

For context…

Since its inception[1], the foundation has always steered away from technical decisions, always referring to the llvm-dev list for those. Previous long running contentious issues (Github, monorepo, CoC) were all decided by the community, in the llvm-dev list, and executed by the foundation.

Recent discussions about the mailing list, irc, discord, discourse have emphasised that, even with an infrastructure working group, the views of the community are still too hard to predict and make it work for the majority. Neither the board of directors, nor the IWG are wide and diverse enough to make decisions that take most people’s views into consideration. That is why we still rely on the dev list for large technical discussions and decisions.

Code review and bug tracking are very much technical decisions. Not code directly, but how we all work. And there are a lot of us. Giving feedback and having no insight into the decision making process will certainly divide the community even more, if we’re forced to accept whatever outcome.

I can’t see how this “solves” the problem of never-ending discussions, other than further fragmenting the community.

cheers,
–renato

[1] http://blog.llvm.org/2014/04/the-llvm-foundation.html

1 Like

    - Any other information that you think will help the Board of Directors make the best decision.

    - Foundation Board will have 30 days to make a final decision about using GitHub Pull Requests and then communicate a migration plan to the community.

Hi Tom,

Please help me here, I think I'm severely misunderstanding what this means...

I'm reading it that the "Board of Directors" will make a decision and communicate to the community, apparently through some undisclosed internal process.

For example:
* What about people that are on holidays during the 30 days comment period?
* What if the points are not made clear after 30 days?
* How do we know the IWG will correctly summarise the comments to the board?
* How does the board guarantee it will take all facts in consideration without bias?
* What kind of recourse would the community have if the decision alienates a large part of the developers?

Please understand that I'm not assuming malice *at all*. We're all humans, and in the effort to make some change happen we quite often let unconscious bias be the merits of our decisions.

For context...

Since its inception[1], the foundation has always steered away from technical decisions, always referring to the llvm-dev list for those. Previous long running contentious issues (Github, monorepo, CoC) were all decided by the community, in the llvm-dev list, and executed by the foundation.

In my opinion, this is not a technical issue. The Board owns the infrastructure
for the project and is responsible for ensuring that it is well maintained and
functional. From the blog post:

"The LLVM Foundation" will allow us to:

  - Solve infrastructure problems.

This is what we are doing here. The project is very much at risk by using
a self-hosted, unmaintained code review tool. We really need to move forward
with a more robust solution otherwise we risk a major disruption to the community.

Recent discussions about the mailing list, irc, discord, discourse have emphasised that, even with an infrastructure working group, the views of the community are still too hard to predict and make it work for the majority. Neither the board of directors, nor the IWG are wide and diverse enough to make decisions that take most people's views into consideration. That is why we still rely on the dev list for large technical discussions and decisions.

Code review and bug tracking are very much technical decisions. Not code directly, but how we all work. And there are a lot of us. Giving feedback and having no insight into the decision making process will certainly divide the community even more, if we're forced to accept whatever outcome.

What additional information about the decision making process would you like to see?

-Tom

In my opinion, this is not a technical issue.

I find that surprising. But maybe it’s just me.

The Board owns the infrastructure
for the project and is responsible for ensuring that it is well maintained and
functional. From the blog post:

“The LLVM Foundation” will allow us to:

  • Solve infrastructure problems.

This is what we are doing here.

In that view, what’s stopping the foundation from moving out of emails and forcing us all to use Discourse?

To me, those are exactly the same kind of problem, and the IWG has already concluded[1] it had to be done. Both mailing lists and phab are self-hosted by us, paid by the foundation.

I’m still confused about the change in the decision process…

cheers,
–renato

[1] https://lists.llvm.org/pipermail/llvm-dev/2021-June/150823.html

  • Any other information that you think will help the Board of Directors make the best decision.

  • Foundation Board will have 30 days to make a final decision about using GitHub Pull Requests and then communicate a migration plan to the community.

Hi Tom,

Please help me here, I think I’m severely misunderstanding what this means…

I’m reading it that the “Board of Directors” will make a decision and communicate to the community, apparently through some undisclosed internal process.

For example:

  • What about people that are on holidays during the 30 days comment period?
  • What if the points are not made clear after 30 days?
  • How do we know the IWG will correctly summarise the comments to the board?
  • How does the board guarantee it will take all facts in consideration without bias?
  • What kind of recourse would the community have if the decision alienates a large part of the developers?

Please understand that I’m not assuming malice at all. We’re all humans, and in the effort to make some change happen we quite often let unconscious bias be the merits of our decisions.

For context…

Since its inception[1], the foundation has always steered away from technical decisions, always referring to the llvm-dev list for those. Previous long running contentious issues (Github, monorepo, CoC) were all decided by the community, in the llvm-dev list, and executed by the foundation.

In my opinion, this is not a technical issue. The Board owns the infrastructure

for the project and is responsible for ensuring that it is well maintained and
functional. From the blog post:

“The LLVM Foundation” will allow us to:

  • Solve infrastructure problems.

This is what we are doing here. The project is very much at risk by using
a self-hosted, unmaintained code review tool. We really need to move forward
with a more robust solution otherwise we risk a major disruption to the community.

I’d like to dispute this on multiple accounts.

  • You write that “the board owns the infrastructure”, but the board has never been involved with Phabricator in any way (the hosting is provided by Google from the beginning: both the machine and human-power to keep it running), nor did the board get in touch recently with the folks currently keeping the instance running as far as I know.
  • You write that the “project is very much at risk”, but Phabricator has been self-hosted for > 8 years and it isn’t clear to me why there is a sudden emergency on this side. The claim of “risk a major disruption to the community” to justify the current push looks like FUD to me.

The RFC states that “we would like to move away from a self-hosted solution”, but who is “we”? How was this decided and why?

Recent discussions about the mailing list, irc, discord, discourse have emphasised that, even with an infrastructure working group, the views of the community are still too hard to predict and make it work for the majority. Neither the board of directors, nor the IWG are wide and diverse enough to make decisions that take most people’s views into consideration. That is why we still rely on the dev list for large technical discussions and decisions.

Code review and bug tracking are very much technical decisions. Not code directly, but how we all work. And there are a lot of us. Giving feedback and having no insight into the decision making process will certainly divide the community even more, if we’re forced to accept whatever outcome.

+1 to everything Renato wrote above, in particular how these tools are fairly core to our development and are technical matters.

With all that said, I think the process and the way the RFC was written distracts unnecessarily from the discussion here: it seems fairly healthy to me to just re-evaluate our tooling and how they fit our needs and revisit the alternatives.

I’d love it if we were able to move to pull-request, but I’m also quite wary of rushing it for other considerations before we can get a roadmap to get to the same feature level on GitHub that we get through Phabricator today (and the narrative pushed through in the RFC does not bring me confidence here).

Best,

     >
     > - Any other information that you think will help the Board of Directors make the best decision.
     >
     > - Foundation Board will have 30 days to make a final decision about using GitHub Pull Requests and then communicate a migration plan to the community.
     >
     > Hi Tom,
     >
     > Please help me here, I think I'm severely misunderstanding what this means...
     >
     > I'm reading it that the "Board of Directors" will make a decision and communicate to the community, apparently through some undisclosed internal process.
     >
     > For example:
     > * What about people that are on holidays during the 30 days comment period?
     > * What if the points are not made clear after 30 days?
     > * How do we know the IWG will correctly summarise the comments to the board?
     > * How does the board guarantee it will take all facts in consideration without bias?
     > * What kind of recourse would the community have if the decision alienates a large part of the developers?
     >
     > Please understand that I'm not assuming malice *at all*. We're all humans, and in the effort to make some change happen we quite often let unconscious bias be the merits of our decisions.
     >
     > For context...
     >
     > Since its inception[1], the foundation has always steered away from technical decisions, always referring to the llvm-dev list for those. Previous long running contentious issues (Github, monorepo, CoC) were all decided by the community, in the llvm-dev list, and executed by the foundation.
     >

    In my opinion, this is not a technical issue. The Board owns the infrastructure

    for the project and is responsible for ensuring that it is well maintained and
    functional. From the blog post:

    "The LLVM Foundation" will allow us to:

      - Solve infrastructure problems.

    This is what we are doing here. The project is very much at risk by using
    a self-hosted, unmaintained code review tool. We really need to move forward
    with a more robust solution otherwise we risk a major disruption to the community.

I'd like to dispute this on multiple accounts.

- You write that "the board owns the infrastructure", but the board has never been involved with Phabricator in any way (the hosting is provided by Google from the beginning: both the machine and human-power to keep it running), nor did the board get in touch recently with the folks currently keeping the instance running as far as I know.

Sorry, 'owns' is probably the wrong word. What I mean is the Board is responsible
for the infrastructure. Yes, it's great that Google has been contributing the
machines and human support to keep it running, this has been a great service to
the community. However, it's not a good position for the Board to be responsible
for something that it doesn't have control over. If Google decided to stop hosting
Phabricator for some reason (unlikely, but not impossible), the Board would be
responsible for finding a replacement.

- You write that the "project is very much at risk", but Phabricator has been self-hosted for > 8 years and it isn't clear to me why there is a sudden emergency on this side. The claim of "risk a major disruption to the community" to justify the current push looks like FUD to me.

This is not meant to insult or diminish the work done by the maintainers over
the last 8 years. The self-hosted part is a risk, but a small one for reasons
mentioned above. The main risk is that Phabricator is no longer maintained upstream.
There was already an issue[1] recently where the arc tool stopped working and won't
be fixed upstream. Using unmaintained software is a bigger risk.

[1] https://lists.llvm.org/pipermail/llvm-dev/2021-September/153019.html

The RFC states that "we would like to move away from a self-hosted solution", but who is "we"? How was this decided and why?

We, meaning the LLVM Board of Directors. And really the problem isn't the self-hosting
so much as it's the lack of an enforceable maintenance agreement the Foundation and the
maintainers.

-Tom

+1 to Renato’s response here. I had the same thought, and Renato phrased it much better than I’d have managed.

Philip

However, it’s not a good position for the Board to be responsible
for something that it doesn’t have control over. If Google decided to stop hosting
Phabricator for some reason (unlikely, but not impossible), the Board would be
responsible for finding a replacement.

Sorry, this is a very weak reason for such a strong worded “RFC”.

I cannot imagine “Google” stopping to support something so quickly as to leave the foundation without recourse. And even if they did, no one would blame the foundation for that.

Even if you ignore all the effort that hundreds of their engineers have done over the past decade to the project, this would hurt Google more than anyone else. It’s a far fetched concern.

And if the foundation wants “control” of a piece of infrastructure that Google has been maintaining for years, then this is a different discussion. Hopefully one that doesn’t involve unilateral decisions.

The main risk is that Phabricator is no longer maintained upstream.
There was already an issue[1] recently where the arc tool stopped working and won’t
be fixed upstream. Using unmaintained software is a bigger risk.

I don’t like using unmaintained software either, but I think our Phab has had more attention than the upstream project. And no one has to use arc, I certainly never have.

Don’t get me wrong, I don’t like Phab and I think Github would bring new people to the project, but it’s gotta be done the right way, and pushing it isn’t it.

We, meaning the LLVM Board of Directors. And really the problem isn’t the self-hosting
so much as it’s the lack of an enforceable maintenance agreement the Foundation and the
maintainers.

The problem isn’t self-hosting at all, given that Google is doing that. (apologies, I assumed otherwise earlier).

Neither is maintenance, given Google is doing that too.

The only thing that’s left is control, and I don’t really understand why this is important, as I explained above.

cheers,
–renato

Hello! The purpose of this email is to start a discussion about our code review tools. No decisions have been made about changing tools. The idea behind a timeline is so that information could be gathered in a timely manner. The Infrastructure Working Group was formed to bring together community members who have an experience and/or passion regarding infrastructure. Anyone can participate in this working group and like the LLVM Foundation, the minutes are all made public.

The LLVM Foundation’s mission is to support the LLVM project and help ensure the health and productivity of of the community and this is done through numerous ways including infrastructure. I do not think it is a negative thing that the foundation board of directors would be discussing our current tools and gathering information how how well they work and how we can make them better. As the legal entity who bares financial and legal responsibility for a lot of the infrastructure, this would make sense. This also makes sense because of the people involved who care a lot about LLVM and the project. But, the LLVM Foundation does not pay for Phabricator and we are very grateful for Google’s support of this critical piece of our infrastructure.

Regarding Phabricator, there are a couple of pieces of information that were provided to the LLVM Foundation by maintainers (maybe previous it sounds like) of this instance and how we may need to look into alternative ways to support it. In addition, Phacility itself has publicly stated that it is winding down operations. (https://admin.phacility.com/phame/post/view/11/phacility_is_winding_down_operations/). Lastly, there are questions about why we are not using GitHub pull requests as we are on GitHub and that might be the natural path to take for a number of reasons.

The above reasons are why the RFC was written. Perhaps it wasn’t written in the best way, but I also feel like it is being read in a negative way which is incredibly disappointing given I don’t feel there is a valid reason for this.

-Tanya

Hi,

# Proposal

The LLVM Foundation Board of Directors is seeking comment on the current state of Code Review
within the LLVM Project and its sub-projects. Phabricator is no longer actively maintained
and we would like to move away from a self-hosted solution, so our goal is to determine if
GitHub Pull Requests are a good alternative to our current code review tool: Phabricator.

Specifically we are looking for feedback on:
- What features or properties make Github Pull Requests better than Phabricator?

I appreciate the ability to have bots help participate in code review,
though perhaps this isn't actually specific to github.

In particular, I think rustc's use of bors for merging is pretty
awesome; people literally cannot merge unless the bot has run all unit
tests on all platforms. As is, anyone can commit without running any
tests.

I frequently break the build for non-linux platforms. That LLVM has so
many reverts for breakage on platforms tested post submit is
embarrassing. People chasing such breakages is a waste of their time,
IMO. We should have more pre-submit testing; I think the board should
focus on that particular problem first, BEFORE pursuing changing code
review platforms.

It seems an obvious risk to me that phab is no longer maintained, but
as others have noted, it hasn't all come crashing down yet.

+1 for Nick’s comment

Anshil

Forgive me if I’m wrong, but if the community consensus is that we should continue to use Phabricator, and Phabricator is not being provided/maintained by the LLVM Foundation, isn’t it moot what the LLVM Foundation/Infrastructure Working Group recommends/wants to happen? The current maintainers would continue to maintain Phabricator (assuming they wanted to), and people would still be able to review things there. What would happen if the Foundation officially supported PRs, without community consensus (in particular from the Phabricator maintainers), is a potential split in the community, with some continuing in the old way and others using the new way (and presumably some choosing to review on both platforms). This cannot be good.

I’m all for the discussion to be had, about whether we switch, but as far as I can see, nothing’s really changed from the previous conversations on PRs versus Github, apart from the announcement of end of support by the upstream company, but that was quite a while ago now, and even with the stale Arcanist issue, there hasn’t been a big push from community members to change: the consensus in the posts discussing this and the moving to PRs seems to still be “there are things that are blocking switching still”.

At the most, from this IWG/Foundation consultation, it should be that the Foundation recommends one or other approach, and is willing to provide X infrastructure required. The community can then choose to agree with whatever approach is recommended or stick with the status quo. There shouldn’t be an edict that says we will do one thing or the other.

Another side-point: whilst the IWG may consist of people who care about LLVM, there are far more people who care as much, but who just don’t have the time to participate in such a group. This is particularly important to note, because the community does not elect members to this group. To an extent, the same is also true of the Foundation board itself, since there are plenty of people who may not agree with their decisions, but don’t have the time to volunteer for the board. I’m not suggesting that there’s any malice in this discussion, and indeed, the fact that it’s open to community comments certainly is helpful, but I’d be worried of some kind of echo chamber/unconscious bias within the small groups suggesting there is consensus for one approach, when the wider community thinks otherwise.

James

… nothing’s really changed from the previous conversations on PRs versus Github, apart from the announcement of end of support by the upstream company, but that was quite a while ago now, and even with the stale Arcanist issue, there hasn’t been a big push from community members to change …

James, If you’ll forgive me for cherry-picking a small part of your point, I think it bears mentioning that human beings tend to ignore future problems until they become current problems. Most of us here want to work on compilers, not deal with infrastructure. This doesn’t mean that the status quo is ok.

As I see it, it would be a mistake to just continue on with zombie-phabricator as we have. Perhaps the board of directors could have taken a different tone when presenting this issue, but I think they are doing the right thing by forcing a change soon. Tools are degrading, and security fixes are not being implemented. If we do nothing we’re all going to wake up some day and find that the github repo has had its owner changed or somesuch catastrophe. We need to do something, and I think setting a deadline for a change was the right call.

+1 on finding a replacement before everything is burning. Thank you

(Note: I’m speaking as an individual here, not with any particular organizational hat; FWIW I personally use both Phabricator and GitHub daily).

Various bits of LLVM infra have had organic roots, e.g. the mailing lists originally hosted at UIUC, Phabricator being set up by individuals, etc. However, this has historically always become a problem: e.g. UIUC prevented us from having a wiki early on because of user-generated content rules, the mailing lists get overrun by spam, Phabricator becomes unmaintained.

The root issue here is that LLVM is a >20 year old project, and many of the technologies that have successfully propelled it to where it is evolve and change, and the people working on LLVM wander into and out of the project over the years.

Meanwhile, LLVM is critical infra for many teams, companies and products. I consider it to be a serious strategic risk that we depend on infrastructure that is maintained by (well intentioned, and obvious incredibly valued) community volunteers: for example, SVN used to go down when the server filled up. This simply isn’t a way to run a scalable project, and makes us very susceptible to the “bus factor”. We as a project need to depend on maintained and hosted infra, not run it ourselves.

Furthermore, while community consensus is an important part of decision making in the LLVM project, it is clear that not every individual will be happy. Further, from a project management perspective, we don’t want multiple ways to do patch reviews: we need a single standard solution here, this can’t practically be a matter of personal preference.

Coming back to the details at hand, there is a proposal to move to GitHub PR’s for a variety of reasons that other people have made. It also seems that there are some serious gaps that we need to figure out before it is plausible replacement, which is what the thread is exploring.

This all seems like a good and healthy discussion to be having. It seems that some folks are surprised by the premise of wanting to get on maintained and hosted infrastructure (e.g. SVN → GitHub). I thought this was an extremely clear direction for years now - and that we were problem solving towards the best way to do this.

-Chris

Since I think we’re risking confusion on the point here, let me clarify that at least my response to this thread should not be read as opposition (or support) for a migration to github. I am expressing no opinion on that matter. I see the primary point being discussed in this thread being the decision making process proposed, not the decision itself.

Philip

Just two comments on the process here:

1. I don't think we can really make a 'wrong' decision regarding what
code review platform we use. While Github's review interface has its
own problems, it's not so bad that it would actually prevent people
from reviewing code. Having said that, the only way we could mess up
is if we end up with the situation that James describes, where we have
two groups in the community using different review platforms. And the
only way I can see this could happen is if we do the same thing we did
with Discord or Discourse where we just push some new system but we
don't have the consensus or critical mass to make everyone switch.
Both Discord and Discourse were IMHO good ideas in general and were
well intentioned, but the way we switched to them split & hurt the
community in the end. Given how critical code review is, I would
rather wait until we have enough consensus for the switch than just do
it and hope it works out well.

2. I do appreciate that the foundation/infrastructure group is trying
to improve our infrastructure, but I am not sure why the code review
system has become an apparently urgent issue. While Phabricator is
clearly going the way of the Dodo, from my own experience it at least
still works. Meanwhile both the mailing list and the bug tracker are
pretty much completely broken (the mailing list in the weird legal
spam limbo and the bugtracker having a disabled user registration). I
don't want to be the person that tells other people what to do, but if
we have resources/time to spend on fixing up our infrastructure, I
hope we could spend it on fixing the things that currently don't work.
Having a working bug tracker or some new mailing list system where I
(and other moderators) don't have to manually approve messages 24/7
and actually accept subscribers would be nice.

Thanks,
- Raphael

+1

As I already mentioned on GitHub[1], nothing really has changed since
the winding down of Phacility, Phabricator is continuing receiving
security patches. A project fork already exists [2].

[1] https://github.com/llvm/llvm-iwg/issues/73#issuecomment-936716573
[2] https://we.phorge.it/

I want to take the other side here, and say that I appreciate that the board is trying to provide more structure for this decision making process. I don’t think the board is trying to step in and take ownership of the decision, they are trying to solicit input and reflect it back to LLVM developers in an efficient way. It has long been clear that LLVM needs a more effective process for building consensus and making decisions, and in the absence of that, the board came up with this ad hoc process. That seems very reasonable to me.

For everyone else who has suggestions on how we could run our infra better, my understanding is that you can get involved in the infrastructure working group by emailing iwg@llvm.org.

I want to take the other side here, and say that I appreciate that the board is trying to provide more structure for this decision making process. I don’t think the board is trying to step in and take ownership of the decision, they are trying to solicit input and reflect it back to LLVM developers in an efficient way. It has long been clear that LLVM needs a more effective process for building consensus and making decisions, and in the absence of that, the board came up with this ad hoc process. That seems very reasonable to me.

Ad-hoc isn’t really sensible for such an important thing. We have done this before, so it’s not lack of prior art either.

In every past similar situation it has been the consensus that the board does not decide on technical matters. They may help, organise, spend resources, gather information, build tools, but the ultimate decision is up to the community (whatever that means).

So far, the harder technical decisions (for example, migrating to Github), have been taken after enough consensus was built in the list and enough discussions happened in the conferences, until such a day the vast majority agreed it should be done.

There are three main pending issues:

  • Bugzilla, where everyone thinks we have to change but GH issues are nowhere near complete enough.
  • Phabricator, where we’re mostly in favour of GH PRs, but there’s still at least one major hurdle, patch sets.
  • Mailing list, where it’s a pretty even split, with the IRC/Discord split being a major counter-example.

Hosting on github vs self-hosted was a small change, and most people were in favour, but the problem was mostly around monorepo vs submodules.

Starting a discord channel is not something people need “permission”, but it did fragment the just-in-time interactions. Starting a Slack channel or whatever is the new thing would be the same problem, but nothing too terrible.

However, code review, technical discussions and bug tracking are pretty core to how we all interact, and we should not have more than one of any of those things. so, whatever decision is taken it will be a decision to move, not add.

This is a pretty serious decision, and I believe we’d have a lot less friction if we do in the same way we did the Github. Proposals, discussions, BoF sessions and a final decision when it’s clear that the majority of the community is on board with the changes.

But to get there, we’ll need to hash out all issues. Right now, the discussion is around patch sets, and until that gets sorted, we really shouldn’t even try to use PRs. It may take less then 30 days, it may take more, but that discussion must happen in the list, not in a working-group or in the foundation board’s meeting.

This is how we’ve always done it so far and it has been working well. At least most people I know think that this is better than most other alternatives, including ad-hoc decision making plans.

cheers,
–renato