WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
198289
[ews-build] Triggered builds should use same revision as parent build
https://bugs.webkit.org/show_bug.cgi?id=198289
Summary
[ews-build] Triggered builds should use same revision as parent build
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.
Top of Page
Format For Printing
XML
Clone This Bug