(original) (raw)
On Wed, Feb 20, 2019 at 8:26 PM Philip Reames via llvm-dev <llvm-dev@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@lists.llvm.org> writes:
\>> On Wed, Feb 20, 2019 at 9:13 AM Tom Stellard <tstellar@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@lists.llvm.org
\> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_
LLVM Developers mailing list
llvm-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev