Phabricator and private reviews

Project Infrastructure LLVM Dev List Archives
#1

For whatever reason, patches posted to the Phabricator website still aren't being sent to the mailing list, making it difficult for us to review them.

I've raised this issue a couple of times in the last few weeks.

In practice this has a detrimental effect to the development workflow because it means that code is being seen only by a small group of individuals who have web accounts. The code isn't hitting llvm-commits or cfe-commits where the majority of code maintainers use the mailing lists for review.

At this point I think Phabricator should be disabled and patches should be send to the mailing lists *until* the technical issue is confirmed resolved.

It's really uncool that code is entering ToT through this back-channel -- I appreciate that it might not be intentional, but every single patch that gets committed this way is a real problem for the project.

Alp.

#2

I don't think it's all patches. I've had plenty of patches go up and
get reviewed with the reviews going to the list lately.

I'm going to object to this proposal.

-eric

#3

Phabricator has certainly had its share of technical difficulties lately.
Just last week it suppressed all email to llvm-commits for many hours.
These problems should be solved. That said, talking of "private reviews"
and "back-channels" doesn't strike me as constructive.

Eli

#4

Even if it's a subset of patch - is it unreliable review/workflow?
If it's unreliable why object to pausing it?

#5

I have to agree with Alp here. I’ve seen a number of review threads that either seem to be missing emails or in which the emails arrive days in unintelligible orders. I don’t know that we need to cut off use of it, but we need to prioritize resolving this issue.

—Owen

#6

    For whatever reason, patches posted to the Phabricator website
    still aren't being sent to the mailing list, making it difficult
    for us to review them.

    I've raised this issue a couple of times in the last few weeks.

    In practice this has a detrimental effect to the development
    workflow because it means that code is being seen only by a small
    group of individuals who have web accounts. The code isn't hitting
    llvm-commits or cfe-commits where the majority of code maintainers
    use the mailing lists for review.

    At this point I think Phabricator should be disabled and patches
    should be send to the mailing lists *until* the technical issue is
    confirmed resolved.

    It's really uncool that code is entering ToT through this
    back-channel -- I appreciate that it might not be intentional, but
    every single patch that gets committed this way is a real problem
    for the project.

Phabricator has certainly had its share of technical difficulties lately. Just last week it suppressed all email to llvm-commits for many hours. These problems should be solved. That said, talking of "private reviews" and "back-channels" doesn't strike me as constructive.

Eli, I wasn't making a value judgement. That's exactly what they are:

   1) They're private reviews because they're conducted away from the LLVM community.
   2) It's a back-channel because the only means of veto is to revert the patch or attempt to "fix forward" post-commit.

I already pointed out that it may not be intentional -- Manuel suggested it could be due to a PHP bug -- but the result is the same. Code is going into the repository without due process which is just more work to deal with for upstream LLVM developers.

Alp.

#7

I have to agree with Alp here. I’ve seen a number of review threads that either seem to be missing emails or in which the emails arrive days in unintelligible orders.

Weird. I've only seen the one occasion earlier.

I don’t know that we need to cut off use of it, but we need to prioritize resolving this issue.

But if people are running into it then I definitely agree.

-eric

#8

Can you provide some data to support this by comparing commits with phab URLs in them with the llvm-commits archive?

It may be that llvm-commits is properly forwarding the review mail, but it’s getting caught in people’s spam filters. I’ve personally had problems with this.

#9

It's been reliable for me and the reviews I've been doing.

-eric

#10

I also object to this proposal.

At least some of the issues have been resolved. In my particular case, I had an invalid email listed in phabricator. Rather than simply effecting my own emails, this was preventing phabricator from notifying for entire threads of review discussion (to *anyone*). It's possible others have this issue as well.

If you notice threads which are not making it to the mailing list, why don't you point them out here?

I do think it's worth asking everyone to double check that a phabricator review has made it to the commits list before submission. If not, they should manually forward the link and give time for comments on the public commits list. At the end of the day, the commits list is our "source of truth", not the phabricator review.

Philip

#11

I also object to this proposal.

At least some of the issues have been resolved. In my particular case, I
had an invalid email listed in phabricator. Rather than simply effecting my
own emails, this was preventing phabricator from notifying for entire
threads of review discussion (to *anyone*). It's possible others have this
issue as well.

If you notice threads which are not making it to the mailing list, why don't
you point them out here?

I do think it's worth asking everyone to double check that a phabricator
review has made it to the commits list before submission. If not, they
should manually forward the link and give time for comments on the public
commits list. At the end of the day, the commits list is our "source of
truth", not the phabricator review.

Agreed wholeheartedly with everything here.

-eric

#12

"May" not be intentional suggests that it also "may" be intentional. Or is
it English comprehension failing me? [sorry, 3rd language...]

When llvm-commits is CCd on the Phabricator review, any suggestion of
intentional hiding is not only inappropriate, but also somewhat ridiculous.
Unless you're suggesting someone is planting those PHP bugs? [that could be
unintentional, I concede, since PHP is just one big bug in general]

Again, no disagreement whatsoever that the bugginess is harmful and should
be addressed. But the tone could be friendlier.

Eli

#13

As Manuel has pointed out previously - Phabricator, just like email,
can be used to conduct off-list reviews for a number of valid reasons.
The existence of Phabricator reviews that don't have the mailing lists
attached isn't, in and of itself, a bug.

Are there particular review threads you've got in mind that appear
problematic? (where a Phabricator-driven, off-list review, was used as
sign-off/authoritative for the purposes of community interaction?).
I've seen one or two cases where people send reviews and forget to CC
the mailing list (this happens with or without Phab) - that case is
not some systematic problem, all it takes is an email response (from
someone who can see the review) suggesting that the mailing list be
added to the thread. There have also been some bugs - as we've
discussed previously, which are resolved fairly swiftly once the issue
is raised. The most recent of which lead to a few hours delay in mail
from Phab to the mailing list, but was in no way catastrophic to the
smooth running of things (and was a "this thing just broke - OK, we
now see it's broken and fix it" - disabling Phab wouldn't really help
us if we don't know that something's broken)

Essentially - if there are particular broken cases, let's look at
them. If they're easy to fix, we'll just fix them, as Manuel did for
the issue last week. If they're harder problems that may take weeks to
fix, then yes, it may make sense to disable or otherwise modify Phab
in the short term while the longer term issues are addressed.

So what are the threads/issues you're seeing currently?

- David

#14

As Manuel has pointed out previously - Phabricator, just like email,
can be used to conduct off-list reviews for a number of valid reasons.
The existence of Phabricator reviews that don't have the mailing lists
attached isn't, in and of itself, a bug.

Are there particular review threads you've got in mind that appear
problematic? (where a Phabricator-driven, off-list review, was used as
sign-off/authoritative for the purposes of community interaction?).
I've seen one or two cases where people send reviews and forget to CC
the mailing list (this happens with or without Phab) - that case is
not some systematic problem, all it takes is an email response (from
someone who can see the review) suggesting that the mailing list be
added to the thread. There have also been some bugs - as we've
discussed previously, which are resolved fairly swiftly once the issue
is raised. The most recent of which lead to a few hours delay in mail
from Phab to the mailing list, but was in no way catastrophic to the
smooth running of things (and was a "this thing just broke - OK, we
now see it's broken and fix it" - disabling Phab wouldn't really help
us if we don't know that something's broken)

Essentially - if there are particular broken cases, let's look at
them. If they're easy to fix, we'll just fix them, as Manuel did for
the issue last week. If they're harder problems that may take weeks to
fix, then yes, it may make sense to disable or otherwise modify Phab
in the short term while the longer term issues are addressed.

So what are the threads/issues you're seeing currently?

Thanks, that's a brilliant question :slight_smile:

Today's concern was "Re: [PATCH] cmake: NDEBUG needlessly defined in non-Release builds" which Reid somehow saw and replied to but still isn't on any of the LLVM public mailing list archives.

I'm coordinating the 3.5 header/feature macro cleanup goal and really need to be in the loop on review requests like this. In fact these changes affect the whole source tree, so the mailing list is the ideally the place for review.

Alp.

#15

My theory is that in this specific instance (http://reviews.llvm.org/D4257), the original sender is not a subscriber of llvm-commits. Therefore the review request got held in moderation. You can see in the archive that it (eventually) made it through here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140623/223335.html

I don’t know where the initial email went.

I agree, it’s really annoying how unreliable email has gotten lately.

#16

Can you provide some data to support this by comparing commits with phab
URLs in them with the llvm-commits archive?

It may be that llvm-commits is properly forwarding the review mail, but
it's getting caught in people's spam filters. I've personally had problems
with this.

There was certainly a problem some time last week or the one before,
wherein Phab wasn't forwarding emails to the mailing lists for many hours,
and at some point flushed them all out. We had a number of outstanding
comments that didn't appear anywhere until the flushing, and we checked the
spam folders. It wasn't that - the emails were just delayed.

Eli

#17

Eric Christopher wrote:

I have to agree with Alp here. I’ve seen a number of review threads that either seem to be missing emails or in which the emails arrive days in unintelligible orders.

Weird. I've only seen the one occasion earlier.

I don’t know that we need to cut off use of it, but we need to prioritize resolving this issue.

But if people are running into it then I definitely agree.

I'm concerned that if people don't use it then its problems will never get discovered, prioritized or fixed?

Nick

PS. I caused a few of these recently, my email in phab was set to an account that isn't subscribed to llvm-commits, so my reviews were held for moderation. Fixed on my end.

#18

It may be that llvm-commits is properly forwarding the review mail, but it's
getting caught in people's spam filters. I've personally had problems with
this.

I did a quick check of llvm-commits archives (at lists.cs.uiuc.edu)
against my recent Phab issues, and significant numbers from this month
haven't shown up (as far as I can tell).

I think there are real issues, though I tend to agree that "private
reviews" is a bit excessive.

Cheers.

Tim.

#19

I am prioritizing email issues. Please always make sure to send them directly to me when you encounter them. Thanks.

#20

            For whatever reason, patches posted to the Phabricator website
            still aren't being sent to the mailing list, making it
        difficult
            for us to review them.

            I've raised this issue a couple of times in the last few
        weeks.

            In practice this has a detrimental effect to the development
            workflow because it means that code is being seen only by
        a small
            group of individuals who have web accounts. The code isn't
        hitting
            llvm-commits or cfe-commits where the majority of code
        maintainers
            use the mailing lists for review.

            At this point I think Phabricator should be disabled and
        patches
            should be send to the mailing lists *until* the technical
        issue is
            confirmed resolved.

            It's really uncool that code is entering ToT through this
            back-channel -- I appreciate that it might not be
        intentional, but
            every single patch that gets committed this way is a real
        problem
            for the project.

        Phabricator has certainly had its share of technical
        difficulties lately. Just last week it suppressed all email to
        llvm-commits for many hours. These problems should be solved.
        That said, talking of "private reviews" and "back-channels"
        doesn't strike me as constructive.

    Eli, I wasn't making a value judgement. That's exactly what they are:

      1) They're private reviews because they're conducted away from
    the LLVM community.
      2) It's a back-channel because the only means of veto is to
    revert the patch or attempt to "fix forward" post-commit.

    I already pointed out that it may not be intentional --

"May" not be intentional suggests that it also "may" be intentional. Or is it English comprehension failing me? [sorry, 3rd language...]

Hi Eli,

The sentence is definitely written as intended and it's that way in order to avoid making value judgements.

I don't have any evidence whether the public lists have been excluded intentionally or due to technical issues so I can't say "is" or "isn't".

As I understand, some people legitimately use Phabricator for internal review, while others *think* they're submitting public patches but the system doesn't forward them to the reviews lists so my usage of "may" is entirely correct.

When llvm-commits is CCd on the Phabricator review, any suggestion of intentional hiding is not only inappropriate, but also somewhat ridiculous. Unless you're suggesting someone is planting those PHP bugs? [that could be unintentional, I concede, since PHP is just one big bug in general]

Clearly I'm not suggesting that someone is planting PHP bugs :slight_smile:

But yes, you can probably tell I'm disappointed that it's still happening, because ultimately I care about the quality of the code and delivering a great compiler to our users. So how about we get to the bottom of these issues?

Alp.