Bug 198289

Summary: [ews-build] Triggered builds should use same revision as parent build
Product: WebKit Reporter: Aakash Jain <aakash_jain>
Component: Tools / TestsAssignee: Aakash Jain <aakash_jain>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, commit-queue, ews-watchlist, jbedard, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch jbedard: review+

Aakash Jain
Reported 2019-05-28 03:13:22 PDT
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.
Attachments
Patch (6.73 KB, patch)
2019-06-20 09:21 PDT, Aakash Jain 		no flags
DetailsFormatted DiffDiff
Patch (7.08 KB, patch)
2019-06-20 13:08 PDT, Aakash Jain 		jbedard: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Aakash Jain
Comment 1 2019-06-19 20:57:53 PDT
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
Jonathan Bedard
Comment 2 2019-06-20 07:30:02 PDT
Won't layout tests have this same problem? I suppose we haven't started running them on new EWS yet.
Aakash Jain
Comment 3 2019-06-20 07:51:29 PDT
(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.
Aakash Jain
Comment 4 2019-06-20 09:21:12 PDT
Created attachment 372560 [details] Patch
EWS Watchlist
Comment 5 2019-06-20 09:23:40 PDT Comment hidden (obsolete)
Aakash Jain
Comment 6 2019-06-20 09:24:55 PDT
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
Jonathan Bedard
Comment 7 2019-06-20 10:34:21 PDT
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).
Aakash Jain
Comment 8 2019-06-20 12:04:21 PDT
> 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.
Jonathan Bedard
Comment 9 2019-06-20 12:53:49 PDT
(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.
Aakash Jain
Comment 10 2019-06-20 13:08:40 PDT
Created attachment 372579 [details] Patch
EWS Watchlist
Comment 11 2019-06-20 13:13:38 PDT Comment hidden (obsolete)
WebKit Commit Bot
Comment 12 2019-06-20 13:31:37 PDT
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
Aakash Jain
Comment 13 2019-06-20 13:36:01 PDT
Committed r246651: <https://trac.webkit.org/changeset/246651>
Radar WebKit Bug Importer
Comment 14 2019-06-20 13:37:12 PDT
<rdar://problem/51957653>
Radar WebKit Bug Importer
Comment 15 2019-06-20 13:37:12 PDT
<rdar://problem/51957654>
Note You need to log in before you can comment on or make changes to this bug.