[WPT Export] The exporter merged an upstream PR before its CL had landed |
|||||
Issue descriptionI'm not sure why but the exporter randomly thought it was time to merge the PR for this CL upstream when it clearly wasn't: CL: https://chromium-review.googlesource.com/c/571249/ PR: https://github.com/w3c/web-platform-tests/pull/6549 My best guess is that since the provisional PR gets assigned a bogus Commit-Position, a given commit on Chromium master could be pointing to it and cause the exporter to think it's time to merge it. I'll look into removing Commit-Position from PR descriptions.
,
Aug 1 2017
That makes total sense. Maybe more verbose logging would help us when we run into situations like this?
,
Aug 1 2017
Sorry I'm a bit confused here, so trying to clarify the hypothesis: 1. Somehow crrev.com/489569 was considered as exportable during an export. 2. The exporter tried to find its corresponding PR: the change ID was not found in any PRs, so the exporter fell back to the commit position, which unfortunately matched https://github.com/w3c/web-platform-tests/pull/6549. So it went ahead and merged that PR. So this bug has two root causes indeed: first we need to fix issue 737178 now that the fallback is no longer needed and commit positions are not reliable when a CL is under review; second we'd better figure out why crrev.com/489569 was considered exportable. Please correct me if I misunderstood anything!
,
Aug 2 2017
Yes, in common.is_exportable, wpt_github.pr_for_chromium_commit is called before wpt_github.pr_for_chromium_commit is called[1]. Therefore, even if a commit doesn't have any exportable changes, if its commit position points to an open PR with that commit position it will interpret that PR as the correct upstream PR and attempt to merge it. As robertma suggested, this will be resolved by fixing bug 737178 . Bumping the priority back up since there is still a real risk of this happening. [1] https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common.py#82
,
Aug 3 2017
Assigning to robertma@ since their CL will resolve this: https://crrev.com/c/598119
,
Aug 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/041a5e276880155037326ea61f66c6f663f96551 commit 041a5e276880155037326ea61f66c6f663f96551 Author: Robert Ma <robertma@chromium.org> Date: Fri Aug 04 14:28:07 2017 Strip Cr-Commit-Position from PRs along with all the usage Cr-{Original-}Commit-Position footers are now stripped from the commit message before being sent to GitHub. The motivations are: 1. We have fully migrated to Gerrit and can rely on Change ID. 2. Commit positions of in-process CLs are not reliable, leading to confusion and other issues (e.g. crbug.com/748876 ). All the usages of commit positions are removed as well, including most_recent_chromium_commit and export_first_exportable_commit that are no longer in use. Since change ID is the only way to look up corresponding PR now, WPTGitHub.pr_for_chromium_commit is removed and all previous users now interact with pr_with_change_id directly. Bug: 737178 , 748876 Change-Id: I32b395c069ddb66ed1ff2412d2cbe2572e7678c4 Reviewed-on: https://chromium-review.googlesource.com/598119 Commit-Queue: Robert Ma <robertma@chromium.org> Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org> Cr-Commit-Position: refs/heads/master@{#492024} [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common_unittest.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit_unittest.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_mock.py [modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_unittest.py
,
Aug 4 2017
We believe bug 737178 is the root cause, which has been fixed by the change above. Marking this bug as fixed for now, unless we see any new incidents of this issue.
,
Aug 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8f1d3ec4530580dabed2db5b681b74005b23236c commit 8f1d3ec4530580dabed2db5b681b74005b23236c Author: Robert Ma <robertma@chromium.org> Date: Fri Aug 04 19:52:10 2017 Revert "Strip Cr-Commit-Position from PRs along with all the usage" This reverts commit 041a5e276880155037326ea61f66c6f663f96551. Reason for revert: Rietvield has not been completely deprecated. Existing CLs can still be landed via Rietveld, so we need to keep the fallback mechanism a little longer. Original change's description: > Strip Cr-Commit-Position from PRs along with all the usage > > Cr-{Original-}Commit-Position footers are now stripped from the commit > message before being sent to GitHub. The motivations are: > 1. We have fully migrated to Gerrit and can rely on Change ID. > 2. Commit positions of in-process CLs are not reliable, leading to > confusion and other issues (e.g. crbug.com/748876 ). > > All the usages of commit positions are removed as well, including > most_recent_chromium_commit and export_first_exportable_commit that are > no longer in use. > > Since change ID is the only way to look up corresponding PR now, > WPTGitHub.pr_for_chromium_commit is removed and all previous users now > interact with pr_with_change_id directly. > > Bug: 737178 , 748876 > Change-Id: I32b395c069ddb66ed1ff2412d2cbe2572e7678c4 > Reviewed-on: https://chromium-review.googlesource.com/598119 > Commit-Queue: Robert Ma <robertma@chromium.org> > Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> > Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org> > Cr-Commit-Position: refs/heads/master@{#492024} TBR=qyearsley@chromium.org,jeffcarp@chromium.org,robertma@chromium.org Change-Id: If92c76683417d795e82f68e1f46ba25210577a86 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 737178 , 748876 Reviewed-on: https://chromium-review.googlesource.com/602787 Commit-Queue: Jeff Carpenter <jeffcarp@chromium.org> Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org> Cr-Commit-Position: refs/heads/master@{#492106} [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common_unittest.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit_unittest.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_mock.py [modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_unittest.py
,
Aug 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78df37d1618c8db03c47d950b355fb3e3fa9a384 commit 78df37d1618c8db03c47d950b355fb3e3fa9a384 Author: Robert Ma <robertma@chromium.org> Date: Tue Aug 08 19:20:52 2017 Strip Cr-Commit-Position from provisional PRs Cr-{Original-}Commit-Position footers are now stripped from the description of provisional CLs (those created from Gerrit in-flight CLs). The motivations are: 1. All Gerrit CLs have change IDs, which act sufficiently as unique IDs. 2. Commit positions of in-flight CLs are not reliable (ToT+1), leading to confusion and other issues (e.g. crbug.com/748876 ). Also remove most_recent_chromium_commit and export_first_exportable_commit that are no longer in use. This is a rework of I32b395c069ddb66ed1ff2412d2cbe2572e7678c4 which was reverted in If92c76683417d795e82f68e1f46ba25210577a86 due to some issues. Bug: 737178 , 748876 Change-Id: I33588ef09b3bd6d8a22870eaf222a2f17080d4a6 Reviewed-on: https://chromium-review.googlesource.com/602617 Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org> Commit-Queue: Robert Ma <robertma@chromium.org> Cr-Commit-Position: refs/heads/master@{#492723} [modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py [modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit.py [modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit_unittest.py [modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py [modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py [modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py [modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py [modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_unittest.py |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by qyears...@chromium.org
, Jul 26 2017