New issue
Advanced search Search tips

Issue 748876 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 737178

Blocking:
issue 707006



Sign in to add a comment

[WPT Export] The exporter merged an upstream PR before its CL had landed

Project Member Reported by jeffcarp@chromium.org, Jul 26 2017

Issue description

I'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.
 
I'm not sure about that hypothesis; on https://github.com/w3c/web-platform-tests/pull/6549 the commit position listed is https://ccrev.com/489569 which is a non-exportable commit (no wpt changes).

So that commit shouldn't have shown up in the list of exportable commits to process...

There's no clear next step or way to reproduce this right now, I think, so I think we'll wait and watch to see if it happens again?
Labels: -Pri-1 Pri-3
That makes total sense. Maybe more verbose logging would help us when we run into situations like this?
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!
Blockedon: 737178
Labels: -Pri-3 Pri-1
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
Cc: -robertma@chromium.org jeffcarp@chromium.org
Owner: robertma@chromium.org
Status: Started (was: Assigned)
Assigning to robertma@ since their CL will resolve this: https://crrev.com/c/598119
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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.
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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