llvm dev - Feb 2019 - Clarification on expectations of buildbot email notifications

This is kind of what I was getting at with my original email, so thank you
for wording it better than I did.

If we can agree that "contact the author first for internal bots" is
better
than "revert automatically, even for internal bots" (which may not be
the
case, I don't want to speak for others), then the problem becomes one of
defining what an "internal bot" is.  In my opinion, an internal bot it
is
one that does does not satisfy both conditions: a) on the public waterfall,
b) automatically sends notifications, but perhaps not everyone agrees with
this definition.

On Wed, Feb 20, 2019 at 9:13 AM Tom Stellard <tstellar at redhat.com>
wrote:
> On 02/20/2019 07:39 AM, via llvm-dev wrote:
> > Reid said:
> >
> >> I don't think whether a buildbot sends email should have
anything to do
> >> with whether we revert to green or not. Very often, developers
commit
> >> patches that cause regressions not caught by our buildbots. If the
> >> regression is severe enough, then I think community members have
the
> >> right, and perhaps responsibility, to revert the change that
caused it.
> >> Our team maintains bots that build chrome with trunk versions of
clang,
> >> and we identify many regressions this way and end up doing many
reverts
> >> as a result. I think it's important to continue this practice
so that
> >> we don't let multiple regressions pile up.
> >
> > My team also has internal bots and we see breakages way more often
than
> > we'd like.  We are a bit reluctant to just go revert something,
though,
> > and typically try to engage the patch author first.
> >
> > Engaging the author has a couple of up-sides: it respects the
author's
> > contribution and attention to the process; and once you've had to
fix
> > a particular problem yourself (rather than someone else cleaning up
> > after your mess) you are less likely to repeat that mistake.
> >
>
> In my opinion engaging the author first is the best approach for internal
> bots, and I think this should be captured in some way in the developer
> guide.
>
> We don't want to send the message that is OK to revert patches at any
> time just because one of your internal tests failed.  In my experience
> most community members do engage with the author first, like Paul
> describes,
> so this isn't usually a problem, but new contributors may not be
familiar
> with this precedent.
>
> -Tom
>
> >> I think what's important, and what we should, after this
discussion
> >> concludes, put in the developer policy, is that the person doing
the
> >> revert has the responsibility to do their best to help the patch
author
> >> reproduce the problem or at least understand the bug.
> >>
> >> This can take many forms. They can link directly to an LLVM
buildbot,
> >> which should be self-explanatory as far as reproduction goes. It
can be
> >> an unreduced crash report. If they're nice, they can use
CReduce to make
> >> it smaller. But, a reverter can't just say "Revert rNNN,
breaks
> >> $RANDOM_PROJECT on x86_64-linux-gu". If they add,
"reduction
> forthcoming"
> >> and they deliver on that promise, I think we should support that.
> >>
> >> In other words, the bar to revert should be low, so we can do it
fast
> >> and save downstream consumers time and effort. If someone
isn't making
> >> a good faith effort to follow up after a revert, then authors have
a
> >> right to push back.
> >
> > We have been on the wrong side of a revert where it was "this
broke us"
> > and then nothing. I was inclined to just re-apply the patch, but
that's
> > my "Mr Grumpy" avatar talking. How do we address failure to
conform to
> the
> > community norms?
> >
> >> I agree with Paul that we should remove the text about checking
nightly
> >> builders. That suggestion seems a bit dated.
> >
> > That was Tom Stellard, not me, but I agree with him.
> > --paulr
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190220/d17b21cf/attachment.html>

Zachary Turner via llvm-dev <llvm-dev at lists.llvm.org>
writes:
> On Wed, Feb 20, 2019 at 9:13 AM Tom Stellard <tstellar at redhat.com>
wrote:
>> On 02/20/2019 07:39 AM, via llvm-dev wrote:
>> > Reid said:
>> >
>> >> I don't think whether a buildbot sends email should have
anything to do
>> >> with whether we revert to green or not. Very often, developers
commit
>> >> patches that cause regressions not caught by our buildbots. If
the
>> >> regression is severe enough, then I think community members
have the
>> >> right, and perhaps responsibility, to revert the change that
caused it.
>> >> Our team maintains bots that build chrome with trunk versions
of clang,
>> >> and we identify many regressions this way and end up doing
many reverts
>> >> as a result. I think it's important to continue this
practice so that
>> >> we don't let multiple regressions pile up.
>> >
>> > My team also has internal bots and we see breakages way more often
than
>> > we'd like.  We are a bit reluctant to just go revert
something, though,
>> > and typically try to engage the patch author first.
>> >
>> > Engaging the author has a couple of up-sides: it respects the
author's
>> > contribution and attention to the process; and once you've had
to fix
>> > a particular problem yourself (rather than someone else cleaning
up
>> > after your mess) you are less likely to repeat that mistake.
>> >
>>
>> In my opinion engaging the author first is the best approach for
internal
>> bots, and I think this should be captured in some way in the developer
>> guide.
>>
>> We don't want to send the message that is OK to revert patches at
any
>> time just because one of your internal tests failed.  In my experience
>> most community members do engage with the author first, like Paul
>> describes,
>> so this isn't usually a problem, but new contributors may not be
familiar
>> with this precedent.
>>
>> -Tom
>
> This is kind of what I was getting at with my original email, so thank you
> for wording it better than I did.
>
> If we can agree that "contact the author first for internal bots"
is better
> than "revert automatically, even for internal bots" (which may
not be the
> case, I don't want to speak for others), then the problem becomes one
of
> defining what an "internal bot" is.  In my opinion, an internal
bot it is
> one that does does not satisfy both conditions: a) on the public waterfall,
> b) automatically sends notifications, but perhaps not everyone agrees with
> this definition.
I would argue that "internal vs external" is the wrong division here.
It does come up that internal bots or weird local configurations find
significant problems in practice sometimes, and reverting to green can
be completely reasonable for these cases. Obviously some discretion is
necessary, but reverting more or less any change that causes issues
severe enough to legitimately block you or seriously hamper your ability
to notice further issues is fair game in my eyes.

Like Reid says, the important part about reverting is the contract
between the person doing the revert and the original committer. When a
patch is reverted, the reverter has a responsibility for making sure the
originator has the means to fix whatever problem they found. Any revert
that isn't tied to a public bot absolutely must come with reasonable
instructions to reproduce the problem, and the reverter needs to
actively engage with the originator to get the patch back in in a form
that doesn't hit the problem.

+1 to Justin's comment

The only standard for revert should be: it's broken, and here's a 
reproducer.  Nothing else should matter.

... says the person w/a ton of internal regression testing which 
regularly finds crashes upstream, and is terrified of the implied effort 
of having to engage each author of a broken patch individually while 
being unable to ship or see what other breakage is coming in.  
Admittedly, self serving perspective.  :)

Philip

p.s. Speaking personally, I actually *prefer* having a patch of mine 
simply reverted w/o discussion when there's a problem.  This way there's
no expectation of timely response on my side, and if I happen to be out 
of office, or on vacation, I can get to it when I get back and have 
time.  I do expect to be *informed* of the revert of course.

On 2/20/19 1:35 PM, Justin Bogner via llvm-dev wrote:
> Zachary Turner via llvm-dev <llvm-dev at lists.llvm.org> writes:
>> On Wed, Feb 20, 2019 at 9:13 AM Tom Stellard <tstellar at
redhat.com> wrote:
>>> On 02/20/2019 07:39 AM, via llvm-dev wrote:
>>>> Reid said:
>>>>
>>>>> I don't think whether a buildbot sends email should
have anything to do
>>>>> with whether we revert to green or not. Very often,
developers commit
>>>>> patches that cause regressions not caught by our buildbots.
If the
>>>>> regression is severe enough, then I think community members
have the
>>>>> right, and perhaps responsibility, to revert the change
that caused it.
>>>>> Our team maintains bots that build chrome with trunk
versions of clang,
>>>>> and we identify many regressions this way and end up doing
many reverts
>>>>> as a result. I think it's important to continue this
practice so that
>>>>> we don't let multiple regressions pile up.
>>>> My team also has internal bots and we see breakages way more
often than
>>>> we'd like.  We are a bit reluctant to just go revert
something, though,
>>>> and typically try to engage the patch author first.
>>>>
>>>> Engaging the author has a couple of up-sides: it respects the
author's
>>>> contribution and attention to the process; and once you've
had to fix
>>>> a particular problem yourself (rather than someone else
cleaning up
>>>> after your mess) you are less likely to repeat that mistake.
>>>>
>>> In my opinion engaging the author first is the best approach for
internal
>>> bots, and I think this should be captured in some way in the
developer
>>> guide.
>>>
>>> We don't want to send the message that is OK to revert patches
at any
>>> time just because one of your internal tests failed.  In my
experience
>>> most community members do engage with the author first, like Paul
>>> describes,
>>> so this isn't usually a problem, but new contributors may not
be familiar
>>> with this precedent.
>>>
>>> -Tom
>> This is kind of what I was getting at with my original email, so thank
you
>> for wording it better than I did.
>>
>> If we can agree that "contact the author first for internal
bots" is better
>> than "revert automatically, even for internal bots" (which
may not be the
>> case, I don't want to speak for others), then the problem becomes
one of
>> defining what an "internal bot" is.  In my opinion, an
internal bot it is
>> one that does does not satisfy both conditions: a) on the public
waterfall,
>> b) automatically sends notifications, but perhaps not everyone agrees
with
>> this definition.
> I would argue that "internal vs external" is the wrong division
here.
> It does come up that internal bots or weird local configurations find
> significant problems in practice sometimes, and reverting to green can
> be completely reasonable for these cases. Obviously some discretion is
> necessary, but reverting more or less any change that causes issues
> severe enough to legitimately block you or seriously hamper your ability
> to notice further issues is fair game in my eyes.
>
> Like Reid says, the important part about reverting is the contract
> between the person doing the revert and the original committer. When a
> patch is reverted, the reverter has a responsibility for making sure the
> originator has the means to fix whatever problem they found. Any revert
> that isn't tied to a public bot absolutely must come with reasonable
> instructions to reproduce the problem, and the reverter needs to
> actively engage with the originator to get the patch back in in a form
> that doesn't hit the problem.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev