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>> 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 -- 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.> > Eli >-- http://www.nuanti.com the browser experts
On Wed, Jun 25, 2014 at 11:10 AM, Alp Toker <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>> 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...] 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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140625/3621f328/attachment.html>
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