[LLVMdev] Phabricator and private reviews

Alp Toker
2014-06-25T18:42:03Z

On 25/06/2014 21:25, David Blaikie wrote:
> 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 :-)

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.


>
> - David
>
> On Wed, Jun 25, 2014 at 10:44 AM, Alp Toker <alp at nuanti.com>
wrote:
>> 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.
>>
>> --
>> http://www.nuanti.com
>> the browser experts
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

--
http://www.nuanti.com
the browser experts