Bug 55585 – EWS builds patches that fail to build twice, which seems useless and slows down the bots

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Roben (:aroben)]

EWS builds patches that fail to build twice. See this code in AbstractEarlyWarningSystem.review_patch: <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py?rev=76008#L91>

91	        if not self._build(patch, first_run=True):
92	            if not self._can_build():
93	                return False
94	            self._build(patch)

It doesn't seem helpful to build failing patches twice. What would change between the first attempt to build and the second attempt to build?

[Adam Roben (:aroben)]

I noticed this because the Win EWS bots have gotten waaaaay behind due to a build  failure unrelated to the patches they're processing.

[Adam Roben (:aroben)]

I guess we really end up doing *three* builds:

1) Try to build with the patch
2) Do a clean build without the patch
3) Try to build with the patch again

I guess this is trying to figure out when the build is broken due to the patch and when it's already broken due to something else. But I still don't see why (3) is useful. Even (2) seems dubious.

[Adam Roben (:aroben)]

In the best case, all three of these builds could be quite quick (e.g., if only a single .cpp file is changed). But in the worst case they could be three full rebuilds (e.g., if Platform.h is touched).

[Eric Seidel (no email)]

The Win EWS bots used to (and I believe still) do a clean build every 20 builds or so.

It was part of their startup script. It would delete the build directory before buildling.

Lucas admins all the EWS bots these days, so he would know exacxtly how they're set up.  All non-EWS bots use this script:
http://trac.webkit.org/browser/trunk/Tools/EWSTools/start-queue.sh

which does not to a force clean.  We could make all bots do a force clean if that's desired.

[Adam Roben (:aroben)]

OK, it could be that there's something wrong with the script that's causing the clean builds not to happen. We should look into that.

But I think this bug as filed is still valid. Eric, can you explain what benefit there is to building the patch twice?

[Adam Barth]

> But I think this bug as filed is still valid. Eric, can you explain what benefit there is to building the patch twice?

We used to do this because we didn't have a way of capturing the build failure output for upload before.  We re-ran the build to generate the output again.  Now we have a way of keeping the output, so we can remove the final build (we just need to save the build error in case it's caused by the patch).

[Adam Roben (:aroben)]

<rdar://problem/9509336>

[Adam Barth]

I'm curious how you plan to fix this bug.  :)

[Adam Roben (:aroben)]

(In reply to comment #8)
> I'm curious how you plan to fix this bug.  :)

Didn't you already outline a fix in comment 6?

[Adam Barth]

> Didn't you already outline a fix in comment 6?

Very true!  Currently there are two base classes for EWS bots:

* AbstractEarlyWarningSystem
* AbstractTestingEWS

It turns out that AbstractTestingEWS (the new hotness) does exactly that.  AbstractTestingEWS is probably stable enough that we can just switch all the bots to use it as a base class now.

AbstractTestingEWS also runs the tests, but we could switch off that functionality if you don't want to run tests on Windows (or we could leave it on if you do want to run the tests).

[Adam Barth]

Created attachment 95237 [details]
shot in the dark

[Adam Barth]

Created attachment 95238 [details]
tests still need work

[Adam Barth]

I'm running the mac-ews locally using this patch to see whether it explodes.

[Adam Barth]

Created attachment 95254 [details]
Rocking and rolling

[Eric Seidel (no email)]

Comment on attachment 95254 [details]
Rocking and rolling

View in context: https://bugs.webkit.org/attachment.cgi?id=95254&action=review

I don't think you meant to delete all those tests.  Otherwise this looks fine.

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:121
> +    def build_style(self):
> +        return self._build_style

I'm not quite sure why we're doing so much to pipe this through.

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:39
>  class AbstractEarlyWarningSystemTest(QueuesTest):

Why are you deleting all these tests?

[Adam Barth]

(In reply to comment #15)
> (From update of attachment 95254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95254&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:121
> > +    def build_style(self):
> > +        return self._build_style
> 
> I'm not quite sure why we're doing so much to pipe this through.

Yeah, that was probably a bit of overkill.

> > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:39
> >  class AbstractEarlyWarningSystemTest(QueuesTest):
> 
> Why are you deleting all these tests?

They test the code I deleted.  In any case, they're redundant with the other tests for PatchAnalysisTask.

[Adam Barth]

Comment on attachment 95254 [details]
Rocking and rolling

View in context: https://bugs.webkit.org/attachment.cgi?id=95254&action=review

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:-50
> -        OutputCapture().assert_outputs(self, queue._can_build, [], expected_stderr=expected_stderr)

queue._can_build doesn't exist anymore.

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:-57
> -        OutputCapture().assert_outputs(self, queue._can_build, [], expected_stderr=expected_stderr)

ditto

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:-72
> -    def test_post_reject_message_on_bug(self):

This one we might be able to keep...

[Eric Seidel (no email)]

Comment on attachment 95254 [details]
Rocking and rolling

View in context: https://bugs.webkit.org/attachment.cgi?id=95254&action=review

>>> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:39
>>>  class AbstractEarlyWarningSystemTest(QueuesTest):
>> 
>> Why are you deleting all these tests?
> 
> They test the code I deleted.  In any case, they're redundant with the other tests for PatchAnalysisTask.

OK.  I'm not sure I'm 100% convinced they're dupes, but I trust you. :)

[Adam Barth]

Comment on attachment 95254 [details]
Rocking and rolling

Clearing flags on attachment: 95254

Committed r87626: <http://trac.webkit.org/changeset/87626>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.