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

+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

> -----Original Message-----
> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of
> Philip Reames via llvm-dev
> Sent: Wednesday, February 20, 2019 11:26 PM
> To: Justin Bogner; Zachary Turner via llvm-dev
> Subject: Re: [llvm-dev] Clarification on expectations of buildbot email
> notifications
> 
> +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.  :)
(I haven't noticed you doing tons of reverts either, but maybe I just
haven't been looking for them.)

This is why my crew has a staging branch for merging in upstream stuff,
in proper Theory of Constraints "buffer" style.  Between upstream
breaks
and merge issues, it's actually kind of rare to have a clean automated
merge onto our master branch.  We usually end up looking for a "mostly
harmless" merge point and manually bringing it in, then fix up the issues 
later, just so we can stay sort-of caught up.

But, it does give us some breathing room to engage authors and give them
a reasonable period to fix things up themselves, before it materially
affects our actual work branches.
--paulr
> 
> 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
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Wed, Feb 20, 2019 at 8:26 PM Philip Reames via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> +1 to Justin's comment
>
> The only standard for revert should be: it's broken, and here's a
> reproducer.  Nothing else should matter.
>
+1 here.

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)

> ... 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.
>
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).

- Dave

>
> 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
> _______________________________________________
> 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/20190221/46602c0b/attachment.html>

+1 to all of Dave's comments (and agreements)

On Thu, Feb 21, 2019 at 9:07 AM David Blaikie via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
>
>
> On Wed, Feb 20, 2019 at 8:26 PM Philip Reames via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>>
>> +1 to Justin's comment
>>
>> The only standard for revert should be: it's broken, and here's
a
>> reproducer.  Nothing else should matter.
>
>
> +1 here.
>
> 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)
>
>>
>> ... 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.
>
>
> 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).
>
> - Dave
>
>>
>>
>> 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
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev