Bug 198289

Summary:

[ews-build] Triggered builds should use same revision as parent build

Product:

WebKit

Reporter:

Aakash Jain <aakash_jain>

Component:

Tools / Tests

Assignee:

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:

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	Details Formatted Diff Diff
Patch (7.08 KB, patch) 2019-06-20 13:08 PDT, Aakash Jain	jbedard: review+	Details Formatted Diff Diff
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)

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.

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)

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.

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.