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?
I noticed this because the Win EWS bots have gotten waaaaay behind due to a build failure unrelated to the patches they're processing.
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.
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).
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.
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?
> 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).
<rdar://problem/9509336>
I'm curious how you plan to fix this bug. :)
(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?
> 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).
Created attachment 95237 [details] shot in the dark
Created attachment 95238 [details] tests still need work
I'm running the mac-ews locally using this patch to see whether it explodes.
Created attachment 95254 [details] Rocking and rolling
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?
(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.
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...
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. :)
Comment on attachment 95254 [details] Rocking and rolling Clearing flags on attachment: 95254 Committed r87626: <http://trac.webkit.org/changeset/87626>
All reviewed patches have been landed. Closing bug.