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+

Description Aakash Jain 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.
Comment 1 Aakash Jain 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
Comment 2 Jonathan Bedard 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.
Comment 3 Aakash Jain 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.
Comment 4 Aakash Jain 2019-06-20 09:21:12 PDT 
Created attachment 372560 [details]
Patch
Comment 5 EWS Watchlist 2019-06-20 09:23:40 PDT Comment hidden (obsolete)
Comment 6 Aakash Jain 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
Comment 7 Jonathan Bedard 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).
Comment 8 Aakash Jain 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.
Comment 9 Jonathan Bedard 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.
Comment 10 Aakash Jain 2019-06-20 13:08:40 PDT 
Created attachment 372579 [details]
Patch
Comment 11 EWS Watchlist 2019-06-20 13:13:38 PDT Comment hidden (obsolete)
Comment 12 WebKit Commit Bot 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
Comment 13 Aakash Jain 2019-06-20 13:36:01 PDT 
Committed r246651: <https://trac.webkit.org/changeset/246651>
Comment 14 Radar WebKit Bug Importer 2019-06-20 13:37:12 PDT 
<rdar://problem/51957653>
Comment 15 Radar WebKit Bug Importer 2019-06-20 13:37:12 PDT 
<rdar://problem/51957654>