API test should not update the working directory to ToT for retrying without patch. It should use the same revision to create archive from clean-tree from which the archive-with-patch was created. Currently before run-api-tests-without-patch step, 'unapply-patch' step is run (which use 'Tools/Scripts/clean-webkit'), which not only un-apply the patch, but also updates to ToT. This can result in wrong results in some cases, especially when an api test failure is introduced or rolled-back between the first api-test run and the unapply-patch step. e.g.: In https://ews-build.webkit.org/#/builders/9/builds/1847, when the patch processing started there was a failure (TestWebKitAPI.Challenge.ClientCertificate) in the tree, but by the time unapply-patch step run, failure was fixed (rolled-back https://bugs.webkit.org/show_bug.cgi?id=197887). clean-webkit not only cleaned the working directory, but also updated it, and so the new updated ToT did not had this failure, and so the run-api-tests-without-patch passed, and EWS blamed the test failure (TestWebKitAPI.Challenge.ClientCertificate) on the patch.
Note that 'unapply-patch' step doesn't update to ToT, this was already fixed in http://trac.webkit.org/changeset/240107/webkit . Instead, the 'clean-and-update-working-directory' step updates to ToT (as it uses alwaysUseLatest=True). The proper fix would be: a) parent build should pass the revision (as a build property) to the triggered build b) triggered build should (have a build step to) checkout that particular revision
Won't layout tests have this same problem? I suppose we haven't started running them on new EWS yet.
(In reply to Jonathan Bedard from comment #2) > Won't layout tests have this same problem? I suppose we haven't started running them on new EWS yet. Yup, that's applicable to all the triggered builds. That's why I changed the bug title to make it generic. Fix coming soon.
Created attachment 372560 [details] Patch
Attachment 372560 [details] did not pass style-queue: ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:858: [TestCheckOutSpecificRevision.test_success] Passing unexpected keyword argument 'state_string' in function call [pylint/E1123] [5] ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:858: [TestCheckOutSpecificRevision.test_success] No value passed for parameter 'status_text' in function call [pylint/E1120] [5] ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:873: [TestCheckOutSpecificRevision.test_failure] Passing unexpected keyword argument 'state_string' in function call [pylint/E1123] [5] ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:873: [TestCheckOutSpecificRevision.test_failure] No value passed for parameter 'status_text' in function call [pylint/E1120] [5] ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:879: [TestCheckOutSpecificRevision.test_skip] Passing unexpected keyword argument 'state_string' in function call [pylint/E1123] [5] ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:879: [TestCheckOutSpecificRevision.test_skip] No value passed for parameter 'status_text' in function call [pylint/E1120] [5] Total errors found: 6 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Sample runs: Triggered build: https://ews-build.webkit-uat.org/#/builders/19/builds/2792 Parent build (skipped this step): https://ews-build.webkit-uat.org/#/builders/33/builds/4209
Comment on attachment 372560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372560&action=review > Tools/BuildSlaveSupport/ews-build/factories.py:43 > self.addStep(CheckOutSource()) Feels weird to have both CheckOutSource and CheckOutSpecificRevision...looking at the code in CheckOutSource, by default, it sets alwaysUseLatest to true. That means that in EWS, we will update to latest, then downgrade to the specific revision, then apply the patch....is there a reasonable way to skip the update-to-latest bit? Clearly we don't want our own 'CheckOutSource' step (buildbot implements that for us, if I'm reading the code correctly).
> Feels weird to have both CheckOutSource and CheckOutSpecificRevision... Agree. > looking at the code in CheckOutSource, by default, it sets alwaysUseLatest to true. Yes, if we don't use alwaysUseLatest=True, Buildbot will automatically apply the patch to the repo, and that doesn't handle ChangeLogs well (we use our own svn-apply script for that). So, we need to use alwaysUseLatest=True. See https://bugs.webkit.org/show_bug.cgi?id=193138 > That means that in EWS, we will update to latest, then downgrade to the specific revision, Other way to think about it is that git will fetch the repository and then switch to specific revision. > then apply the patch....is there a reasonable way to skip the update-to-latest bit? I couldn't find a better way in Buildbot to do that. If we were not using alwaysUseLatest, then we could have overridden computeSourceRevision() method. (https://github.com/buildbot/buildbot/blob/master/master/buildbot/steps/source/base.py#L262). (Side note: In case of non try builders (e.g: SVNPoller/GitPoller), Buildbot finds the revision from the commit/sourcestamp) I don't see a way for Buildbot to update to a specific revision while using alwaysUseLatest=True (which might make sense as per definition of alwaysUseLatest). In https://github.com/buildbot/buildbot/blob/master/master/buildbot/steps/source/git.py#L74 __init__ doesn't take a revision argument either. > Clearly we don't want our own 'CheckOutSource' step (buildbot implements that for us, if I'm reading the code correctly). Yes.
(In reply to Aakash Jain from comment #8) > ... > > Yes, if we don't use alwaysUseLatest=True, Buildbot will automatically apply > the patch to the repo, and that doesn't handle ChangeLogs well (we use our > own svn-apply script for that). So, we need to use alwaysUseLatest=True. See > https://bugs.webkit.org/show_bug.cgi?id=193138 > ... This is worth a comment, because it's not obvious. This basically explains why we have the duplicate call.
Created attachment 372579 [details] Patch
Attachment 372579 [details] did not pass style-queue: ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:858: [TestCheckOutSpecificRevision.test_success] Passing unexpected keyword argument 'state_string' in function call [pylint/E1123] [5] ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:858: [TestCheckOutSpecificRevision.test_success] No value passed for parameter 'status_text' in function call [pylint/E1120] [5] ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:873: [TestCheckOutSpecificRevision.test_failure] Passing unexpected keyword argument 'state_string' in function call [pylint/E1123] [5] ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:873: [TestCheckOutSpecificRevision.test_failure] No value passed for parameter 'status_text' in function call [pylint/E1120] [5] ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:879: [TestCheckOutSpecificRevision.test_skip] Passing unexpected keyword argument 'state_string' in function call [pylint/E1123] [5] ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:879: [TestCheckOutSpecificRevision.test_skip] No value passed for parameter 'status_text' in function call [pylint/E1120] [5] Total errors found: 6 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 372579 [details] Patch Rejecting attachment 372579 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 372579, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=372579&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=198289&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Updating working directory Processing patch 372579 from bug 198289. Fetching: https://bugs.webkit.org/attachment.cgi?id=372579 Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Jonathan Bedard']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Parsed 4 diffs from patch file(s). patching file Tools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Tools/BuildSlaveSupport/ews-build/factories.py patching file Tools/BuildSlaveSupport/ews-build/steps.py patching file Tools/BuildSlaveSupport/ews-build/steps_unittest.py Hunk #1 FAILED at 35. Hunk #2 succeeded at 870 (offset 34 lines). 1 out of 2 hunks FAILED -- saving rejects to file Tools/BuildSlaveSupport/ews-build/steps_unittest.py.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Jonathan Bedard']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Parsed 4 diffs from patch file(s). patching file Tools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Tools/BuildSlaveSupport/ews-build/factories.py patching file Tools/BuildSlaveSupport/ews-build/steps.py patching file Tools/BuildSlaveSupport/ews-build/steps_unittest.py Hunk #1 FAILED at 35. Hunk #2 succeeded at 870 (offset 34 lines). 1 out of 2 hunks FAILED -- saving rejects to file Tools/BuildSlaveSupport/ews-build/steps_unittest.py.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Jonathan Bedard']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource From https://git.webkit.org/git/WebKit abe2c22ad5f..8cdd4e6ef0c master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 246649 = abe2c22ad5f2dd20c9d3a9dcdec38d9c51cb1835 r246650 = 8cdd4e6ef0c1f0a33512f3d92f7e5d254e7b1c30 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: https://webkit-queues.webkit.org/results/12534172
Committed r246651: <https://trac.webkit.org/changeset/246651>
<rdar://problem/51957653>
<rdar://problem/51957654>