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
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 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
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.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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140625/4db779cd/attachment.html>
On 06/26/14 12:59 AM, Eric Christopher wrote:> 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.Even if it's a subset of patch - is it unreliable review/workflow? If it's unreliable why object to pausing it?
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 On Jun 25, 2014, at 10:59 AM, Eric Christopher <echristo at gmail.com> wrote:> 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 > > 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 > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
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
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. 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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140625/6f550786/attachment.html>
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 On 06/25/2014 10:59 AM, Eric Christopher wrote:> 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 > > 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 > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
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 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
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
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. On Wed, Jun 25, 2014 at 11:13 AM, Reid Kleckner <rnk at google.com> wrote:> 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. > > > 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 >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140625/62f2d237/attachment.html>
On Wed, Jun 25, 2014 at 11:13 AM, Reid Kleckner <rnk at google.com> wrote:> 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> > 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 >> > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140625/ab3c2a37/attachment.html>
> 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.