<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 20, 2019 at 8:26 PM Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+1 to Justin's comment<br>
<br>
The only standard for revert should be: it's broken, and here's a <br>
reproducer.  Nothing else should matter.<br></blockquote><div><br>+1 here.<br><br>With only some wiggle room for "is this regression only in some esoteric mode that only this person/group cares about & it's unreasonable to expect other people to care about/be able to reproduce" (even something like: I can only reproduce this while running on certain hardware, where it's unreasonable to expect someone else to have that hardware - then it gets into weird cases where the esoteric hardware owner can reasonable slow down the rest of the project by having a patch reverted while they investigate, or if they should take the cost of having their branch broken while they investigate & the project continues on regardless, to some degree)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">... says the person w/a ton of internal regression testing which <br>
regularly finds crashes upstream, and is terrified of the implied effort <br>
of having to engage each author of a broken patch individually while <br>
being unable to ship or see what other breakage is coming in.  <br>
Admittedly, self serving perspective.  :)<br>
<br>
Philip<br>
<br>
p.s. Speaking personally, I actually *prefer* having a patch of mine <br>
simply reverted w/o discussion when there's a problem.  This way there's <br>
no expectation of timely response on my side, and if I happen to be out <br>
of office, or on vacation, I can get to it when I get back and have <br>
time.  I do expect to be *informed* of the revert of course.<br></blockquote><div><br>Yeah, I'm of two minds about this personally - I personally get frustrated (but honestly, it's probably as much frustrattion with myself than anyone else) when a patch is reverted for relatively trivial reasons - like a mistyped CHECK line (oh, this should have "requires: asserts"? OK). I'd personally appreciate some courtesy from folks - and tend to extend the same in return (you broke the -Werror build? Oh, I'll just add that cast-to-void to fix your unused variable warning in non-asserts builds, etc).<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 2/20/19 1:35 PM, Justin Bogner via llvm-dev wrote:<br>
> Zachary Turner via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> writes:<br>
>> On Wed, Feb 20, 2019 at 9:13 AM Tom Stellard <<a href="mailto:tstellar@redhat.com" target="_blank">tstellar@redhat.com</a>> wrote:<br>
>>> On 02/20/2019 07:39 AM, via llvm-dev wrote:<br>
>>>> Reid said:<br>
>>>><br>
>>>>> I don't think whether a buildbot sends email should have anything to do<br>
>>>>> with whether we revert to green or not. Very often, developers commit<br>
>>>>> patches that cause regressions not caught by our buildbots. If the<br>
>>>>> regression is severe enough, then I think community members have the<br>
>>>>> right, and perhaps responsibility, to revert the change that caused it.<br>
>>>>> Our team maintains bots that build chrome with trunk versions of clang,<br>
>>>>> and we identify many regressions this way and end up doing many reverts<br>
>>>>> as a result. I think it's important to continue this practice so that<br>
>>>>> we don't let multiple regressions pile up.<br>
>>>> My team also has internal bots and we see breakages way more often than<br>
>>>> we'd like.  We are a bit reluctant to just go revert something, though,<br>
>>>> and typically try to engage the patch author first.<br>
>>>><br>
>>>> Engaging the author has a couple of up-sides: it respects the author's<br>
>>>> contribution and attention to the process; and once you've had to fix<br>
>>>> a particular problem yourself (rather than someone else cleaning up<br>
>>>> after your mess) you are less likely to repeat that mistake.<br>
>>>><br>
>>> In my opinion engaging the author first is the best approach for internal<br>
>>> bots, and I think this should be captured in some way in the developer<br>
>>> guide.<br>
>>><br>
>>> We don't want to send the message that is OK to revert patches at any<br>
>>> time just because one of your internal tests failed.  In my experience<br>
>>> most community members do engage with the author first, like Paul<br>
>>> describes,<br>
>>> so this isn't usually a problem, but new contributors may not be familiar<br>
>>> with this precedent.<br>
>>><br>
>>> -Tom<br>
>> This is kind of what I was getting at with my original email, so thank you<br>
>> for wording it better than I did.<br>
>><br>
>> If we can agree that "contact the author first for internal bots" is better<br>
>> than "revert automatically, even for internal bots" (which may not be the<br>
>> case, I don't want to speak for others), then the problem becomes one of<br>
>> defining what an "internal bot" is.  In my opinion, an internal bot it is<br>
>> one that does does not satisfy both conditions: a) on the public waterfall,<br>
>> b) automatically sends notifications, but perhaps not everyone agrees with<br>
>> this definition.<br>
> I would argue that "internal vs external" is the wrong division here.<br>
> It does come up that internal bots or weird local configurations find<br>
> significant problems in practice sometimes, and reverting to green can<br>
> be completely reasonable for these cases. Obviously some discretion is<br>
> necessary, but reverting more or less any change that causes issues<br>
> severe enough to legitimately block you or seriously hamper your ability<br>
> to notice further issues is fair game in my eyes.<br>
><br>
> Like Reid says, the important part about reverting is the contract<br>
> between the person doing the revert and the original committer. When a<br>
> patch is reverted, the reverter has a responsibility for making sure the<br>
> originator has the means to fix whatever problem they found. Any revert<br>
> that isn't tied to a public bot absolutely must come with reasonable<br>
> instructions to reproduce the problem, and the reverter needs to<br>
> actively engage with the originator to get the patch back in in a form<br>
> that doesn't hit the problem.<br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>