On 25/06/2014 21:18, Eli Bendersky wrote:> > > > On Wed, Jun 25, 2014 at 11:10 AM, Alp Toker <alp at nuanti.com > <mailto:alp at nuanti.com>> wrote: > > > On 25/06/2014 21:03, Eli Bendersky wrote: > > On Wed, Jun 25, 2014 at 10:44 AM, Alp Toker <alp at nuanti.com > <mailto:alp at nuanti.com> <mailto:alp at nuanti.com > <mailto: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. > > > 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 :-) 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.> > Again, no disagreement whatsoever that the bugginess is harmful and > should be addressed. But the tone could be friendlier. > > Eli > >-- http://www.nuanti.com the browser experts
On Wed, Jun 25, 2014 at 12:32 PM, Alp Toker <alp at nuanti.com> wrote:> > On 25/06/2014 21:18, Eli Bendersky wrote: > >> >> >> >> On Wed, Jun 25, 2014 at 11:10 AM, Alp Toker <alp at nuanti.com <mailto: >> alp at nuanti.com>> wrote: >> >> >> On 25/06/2014 21:03, Eli Bendersky wrote: >> >> On Wed, Jun 25, 2014 at 10:44 AM, Alp Toker <alp at nuanti.com >> <mailto:alp at nuanti.com> <mailto:alp at nuanti.com >> >> <mailto: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. >> >> >> 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 :-) > > 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?Right. It would be useful to see actual examples and discuss them on a case-by-case basis. I imagine you sent the original email when observing such a case (or a few) - could you share the review link? I think this thread discussed a number of ways in which such things could go wrong, and it would be interesting to see which of those applies to the example you're presenting. Eli -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140625/9d71150e/attachment.html>
> As I understand, some people legitimately use Phabricator for internal > review, ...MIPS currently do this for patches that only touch the MIPS backend (details can be found at http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140602/220385.html). Any patch that touches common code is sent to the list. One thing I haven't said on that thread yet is that I'm considering trying the 'everything to llvm-commits' workflow myself in a couple weeks when my urgent work is finished. I have some reservations about that workflow but I'm willing to try it to see if my concerns are justified or not. Manuel: I'm aware of one other way for a review to be invisible to llvm-commits despite being CC'd. If you forget to CC llvm-commits when creating the differential revision and add it later in the web interface, Phabricator doesn't send an email to the list. Adding a comment (at the same time or afterwards) triggers an email. The same thing happens when adding reviewers without a comment.
On Thu, Jun 26, 2014 at 11:34 AM, Daniel Sanders <Daniel.Sanders at imgtec.com> wrote:> > As I understand, some people legitimately use Phabricator for internal > > review, ... > > MIPS currently do this for patches that only touch the MIPS backend > (details can be found at > http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140602/220385.html). > Any patch that touches common code is sent to the list. One thing I haven't > said on that thread yet is that I'm considering trying the 'everything to > llvm-commits' workflow myself in a couple weeks when my urgent work is > finished. I have some reservations about that workflow but I'm willing to > try it to see if my concerns are justified or not. >Not knowing your concerns, I'd say they are mostly unjustified ;)> Manuel: I'm aware of one other way for a review to be invisible to > llvm-commits despite being CC'd. If you forget to CC llvm-commits when > creating the differential revision and add it later in the web interface, > Phabricator doesn't send an email to the list. Adding a comment (at the > same time or afterwards) triggers an email. > The same thing happens when adding reviewers without a comment. >That is intentional. We don't want to spam the list with changes in phab - use phab like you would use email, and everything should be fine. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140626/c26bf146/attachment.html>
> On 2014-Jun-25, at 12:32, Alp Toker <alp at nuanti.com> wrote: > > As I understand, some people legitimately use Phabricator for internal review,Is this is a use case we need to support on <http://reviews.llvm.org>?> while others *think* they're submitting public patches but the system doesn't forward them to the reviews listsAside from temporary infrastructure failures, which Manuel responds to quickly, there seem to be two main reasons patches (and comments) don't get sent to the list: 1. Reviewer neglects to add llvm-commits (or adds it after the fact, and the patch isn't sent). This seems easy to fix by requiring llvm-commits on llvm patches (and cfe-commits on clang patches). Is there a technical hurdle to requiring this? 2. Reviewer (or commenter) has an invalid or un-subscribed email on Phab. Is it technically feasible to disallow reviews (or comments on a review) from logins that aren't subscribed to the appropriate list and/or have invalid email addresses? I suspect designing away these failures would fix most of the problems. Since the purpose of this Phab instance is to support on-list reviews, we should make it hard to get that wrong (even if it means making off-list reviews harder). -- dpnes
"Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:>> On 2014-Jun-25, at 12:32, Alp Toker <alp at nuanti.com> wrote: >> >> As I understand, some people legitimately use Phabricator for >> internal review, > > Is this is a use case we need to support on <http://reviews.llvm.org>? > >> while others *think* they're submitting public patches but the >> system doesn't forward them to the reviews lists > > Aside from temporary infrastructure failures, which Manuel responds to > quickly, there seem to be two main reasons patches (and comments) > don't get sent to the list: > > 1. Reviewer neglects to add llvm-commits (or adds it after the fact, > and the patch isn't sent). > > This seems easy to fix by requiring llvm-commits on llvm patches > (and cfe-commits on clang patches). Is there a technical hurdle > to requiring this?+1. This is the situation I've seen most often, and while innocuous it can be pretty annoying.> 2. Reviewer (or commenter) has an invalid or un-subscribed email on > Phab. > > Is it technically feasible to disallow reviews (or comments on a > review) from logins that aren't subscribed to the appropriate list > and/or have invalid email addresses? > > I suspect designing away these failures would fix most of the > problems. > > Since the purpose of this Phab instance is to support on-list reviews, > we should make it hard to get that wrong (even if it means making > off-list reviews harder). > > -- dpnes