Phabricator (Was: Automatically adding llvm-commits as CC)

Moving this to llvmdev - it needs a bit of a wider audience.

There are several issues with phabricator, and in the current state of
things there's a huge amount of confusion on how to even report
problems, let alone try to resolve them.

Recently I started a thread about empty emails, was directed to the
phabricator project's bug tracker, and told there that LLVM has
customized phabricator so there's nothing they (phab) can do. Soon
after, the message I'm replying to below was sent to llvm-admin, and it
was pointed out that they don't maintain phab, so there's nothing *they*
can do:

Chandler Carruth <chandlerc@google.com> writes:

This happens to me as well from time to time. I wonder if there is a
way to have phabricator add llvm-commits to CC as soon as
"repository llvm" or "project llvm" is selected. Or maybe revisions
with an empty subscribers field could be rejected.

llvm-admin doesn't administrate the phabricator. You need to contact:
Manuel Klimek or Chandler Carruth.

This has been discussed before. If you look at the prior discussions on
llvmdev about phabricator you should find lots of references to it.

I don't want to repeat the entire discussion but the essence is "sure, it
could be done, but someone must write the code to do it". The code is posted
where you can get at it, we can even put it in an LLVM repository if that
helps, but so far none have stepped up to write the code to make this happen.
I donated hardware to get this whole thing started for a year, and Manuel did
the much more time consuming work to get it up to the point it is currently
at, but I don't think he has a lot more time to devote to it.

I appreciate the effort that you (Chandler) and Manuel have put into
this, but I find this answer a bit lacking in important details.

Where is the code posted? Where is the documentation about that? The
docs at http://llvm.org/docs/Phabricator.html don't tell me anything
more than "Please let us know whether you like it and what could be
improved!".

Most importantly, where can I file bugs about LLVM's phabricator
instance?

Fundamentally, we need folks in the community to contribute if they have
significant problems with the tools.

Personally, as a reviewer, I find phabricator reviews strictly worse
than sending a patch to the llvm-commits list. Off the top of my head,
with phab:

- The patch doesn't always show up on the mailing list,
- Replies to review comments and the patch that accompanies them come in
  different emails,
- Several emails show up in your inbox with nothing but a link, and no
  indication why they were sent,
- Comments and responses to comments sometimes show up twice - once from
  the person who says them and another time from phab,
- Patches are often (but not always) duplicated - both inline *and*
  attached. This is bizarre, useless, and confuses tools like git-am.

With an email it's trivial to read the diff or to apply the patch to an
LLVM checkout to look at in more detail, including building it or
looking at the result in a text editor.

I realize that quite a few people find the web interface helpful, so
I've refrained from asking people to post patches directly rather than
using phab so far, but that *would* solve my problems with the tool. We
at least need some clear information on how to file bugs and where to
look if we want to try to fix the problems ourselves.

Quick update from IRC chat:
Justin (and anybody else who wants to) is going to file bugs against our phab workflow on the llvm-bugtracker until we get a component for it. Help with keeping our phab instance merged and implementing features we need would be highly appreciated (let me know if you’d like to help with PHP hacking :wink:

Hi Manuel,

I like Phabricator for code review much much more than emails. Let me know how I can help (I’m not afraid of PHP).

Mehdi Amini <mehdi.amini@apple.com> writes:

Hi Manuel,

I like Phabricator for code review much much more than emails. Let me know how
I can help (I’m not afraid of PHP).

Chandler updated the llvm phabricator doc to point at what we're deploying:

  http://llvm.org/docs/Phabricator.html#status

That'll lead you here:

  https://github.com/r4nt/llvm-reviews
  https://github.com/r4nt/phabricator

And soon there'll be some bugs to squash in llvm.org/bugs :wink:

From: "Justin Bogner" <mail@justinbogner.com>
To: "llvmdev" <llvmdev@cs.uiuc.edu>
Cc: "Matthias Braun" <matze@braunis.de>
Sent: Wednesday, May 27, 2015 1:54:39 AM
Subject: [LLVMdev] Phabricator (Was: Automatically adding llvm-commits as CC)

Moving this to llvmdev - it needs a bit of a wider audience.

There are several issues with phabricator, and in the current state
of
things there's a huge amount of confusion on how to even report
problems, let alone try to resolve them.

Recently I started a thread about empty emails, was directed to the
phabricator project's bug tracker, and told there that LLVM has
customized phabricator so there's nothing they (phab) can do. Soon
after, the message I'm replying to below was sent to llvm-admin, and
it
was pointed out that they don't maintain phab, so there's nothing
*they*
can do:

Chandler Carruth <chandlerc@google.com> writes:
>>> This happens to me as well from time to time. I wonder if there
>>> is a
>>> way to have phabricator add llvm-commits to CC as soon as
>>> "repository llvm" or "project llvm" is selected. Or maybe
>>> revisions
>>> with an empty subscribers field could be rejected.
>>
>> llvm-admin doesn't administrate the phabricator. You need to
>> contact:
>> Manuel Klimek or Chandler Carruth.
>
> This has been discussed before. If you look at the prior
> discussions on
> llvmdev about phabricator you should find lots of references to it.
>
> I don't want to repeat the entire discussion but the essence is
> "sure, it
> could be done, but someone must write the code to do it". The code
> is posted
> where you can get at it, we can even put it in an LLVM repository
> if that
> helps, but so far none have stepped up to write the code to make
> this happen.
> I donated hardware to get this whole thing started for a year, and
> Manuel did
> the much more time consuming work to get it up to the point it is
> currently
> at, but I don't think he has a lot more time to devote to it.

I appreciate the effort that you (Chandler) and Manuel have put into
this, but I find this answer a bit lacking in important details.

Where is the code posted? Where is the documentation about that? The
docs at http://llvm.org/docs/Phabricator.html don't tell me anything
more than "Please let us know whether you like it and what could be
improved!".

Most importantly, where can I file bugs about LLVM's phabricator
instance?

> Fundamentally, we need folks in the community to contribute if they
> have
> significant problems with the tools.

Personally, as a reviewer, I find phabricator reviews strictly worse
than sending a patch to the llvm-commits list. Off the top of my
head,
with phab:

- The patch doesn't always show up on the mailing list,
- Replies to review comments and the patch that accompanies them come
in
  different emails,
- Several emails show up in your inbox with nothing but a link, and
no
  indication why they were sent,
- Comments and responses to comments sometimes show up twice - once
from
  the person who says them and another time from phab,

I find all of these to be only a minor inconvenience.

- Patches are often (but not always) duplicated - both inline *and*
  attached. This is bizarre, useless, and confuses tools like git-am.

I disagree; this is a convenient feature for small patches.

With an email it's trivial to read the diff or to apply the patch to
an
LLVM checkout to look at in more detail, including building it or
looking at the result in a text editor.

There are three things that the web interface gives me that I find invaluable:

1. The ability to see, selectively, the full context (or at least additional context) of the patch

2. The ability to filter out white-space only changes, and identify moved regions of code, and highlight sub-line changes

3. The ability to quickly compare changes between different versions of a patch

In addition, the ability to insert formatted comments near particular lines of code is really quite nice.

I realize that quite a few people find the web interface helpful, so
I've refrained from asking people to post patches directly rather
than
using phab so far, but that *would* solve my problems with the tool.
We
at least need some clear information on how to file bugs and where to
look if we want to try to fix the problems ourselves.

I agree; we certainly need to clarify this.

-Hal

On a related note we're using Phabricator in FreeBSD, with a similar
workflow to LLVM and with a similar set of user opinions. Some of our
users really like web-based review and some can't stand it, and so
we'll need to solve many of the same issues so that it's useful (or at
least tolerable) to all. We haven't yet made a lot of usability
improvements, but our current set of changes can be found in our
github repo here: https://github.com/freebsd/phabricator

(As an aside, inspiration for setting up our Phabricator instance came
from LLVM's.)

> Mehdi Amini <mehdi.amini@apple.com> writes:
>> Hi Manuel,
>>
>> I like Phabricator for code review much much more than emails. Let me
know how
>> I can help (I’m not afraid of PHP).
>
> Chandler updated the llvm phabricator doc to point at what we're
deploying:
>
> http://llvm.org/docs/Phabricator.html#status
>
> That'll lead you here:
>
> https://github.com/r4nt/llvm-reviews
> https://github.com/r4nt/phabricator
>
> And soon there'll be some bugs to squash in llvm.org/bugs :wink:

On a related note we're using Phabricator in FreeBSD, with a similar
workflow to LLVM and with a similar set of user opinions. Some of our
users really like web-based review and some can't stand it, and so
we'll need to solve many of the same issues so that it's useful (or at
least tolerable) to all. We haven't yet made a lot of usability
improvements, but our current set of changes can be found in our
github repo here: https://github.com/freebsd/phabricator

Maybe FreeBSD + LLVM is enough use case to convince upstream to work on
this, or at least get our use case on their TODO list?

-- Sean Silva

Hi Ed,

I field https://secure.phabricator.com/T8336 - feel free to add your requirements to that bug.

Hi Manuel,

I like Phabricator for code review much much more than emails. Let me know how I can help (I’m not afraid of PHP).

See here:
https://github.com/r4nt/llvm-reviews/

Things that would help right now:
a) make it easier to start hacking on it (part of that would probably be to create a small dockerfile around the scripts there; I don’t think we’ll want to host with docker, because docker seems to not really match a “1 lamp machine” setup well)
b) help merging with phab upstream changes, testing that everything still works after the merge, and send me a pull request; the closer we keep to upstream the better our security is, and the easier it is to hack on features
c) implement the things people have complained about: for example, figuring out why we sometimes send empty mails to the llvm-dev mailing list would be a good start

Thanks!
/Manuel

FWIW: which branch is the deployed llvm one? llvm/master ? llvm/r4nt-master ?

Mehdi Amini <mehdi.amini@apple.com> writes:

Hi Manuel,

I like Phabricator for code review much much more than emails. Let me know how
I can help (I’m not afraid of PHP).

Chandler updated the llvm phabricator doc to point at what we’re deploying:

http://llvm.org/docs/Phabricator.html#status

That’ll lead you here:

https://github.com/r4nt/llvm-reviews
https://github.com/r4nt/phabricator

FWIW: which branch is the deployed llvm one? llvm/master ? llvm/r4nt-master ?

Ah, sorry, it’s r4nt-staging (for completely bogus reasons, I need to change that).

Thanks!
/Manuel

Thanks for the info - I wanted to take a look at what changes llvm had
made. While looking I noticed that you have an extension requiring
specific mailing lists be included with every diff: just a head's up
that you will want to change that extension soon since mailing lists
are becoming users, similar to bots. See
https://secure.phabricator.com/T8387

Mehdi Amini <mehdi.amini@apple.com> writes:

Hi Manuel,

I like Phabricator for code review much much more than emails. Let me
know how
I can help (I’m not afraid of PHP).

Chandler updated the llvm phabricator doc to point at what we’re
deploying:

http://llvm.org/docs/Phabricator.html#status

That’ll lead you here:

https://github.com/r4nt/llvm-reviews
https://github.com/r4nt/phabricator

FWIW: which branch is the deployed llvm one? llvm/master ?
llvm/r4nt-master ?

Ah, sorry, it’s r4nt-staging (for completely bogus reasons, I need to change
that).

Thanks for the info - I wanted to take a look at what changes llvm had
made. While looking I noticed that you have an extension requiring
specific mailing lists be included with every diff: just a head’s up
that you will want to change that extension soon since mailing lists
are becoming users, similar to bots. See
https://secure.phabricator.com/T8387

Thanks for the heads up - havent’ integrated in a while, and dreading it. On the other hand, the longer I wait, the more work it’ll be. Oh well…