[WPT import] On import, revert exportable-but-not-exported changes and continue |
||||
Issue descriptionIn general, since the beginning, we've always expected that sometimes there will be exportable changes that haven't yet been exported, and we need to avoid undoing these changes on import. The general approach so far has been to abort import when any such changes exist. In the interest of keeping import CLs frequent and small, I think it will be generally better to proceed with import when there are such changes, but try to only import changes that are unrelated to the exportable The simplest approach here would be to call `git checkout HEAD FILE` for each file modified by any exportable commit. In order to avoid re-adding files removed by exportable commits, we probably want to call `rm FILE; git checkout HEAD FILE`. There's might also be some possible case where an exportable commit changes some helper file used by multiple tests, and an upstream commit changes that helper file plus some different set of tests. In this case, reverting the set of files in the exportable commits might not be quite right. Any thoughts? Anyway, after the latest changes in bug 727923 , there is always an exportable-but-not-exported commit and so the importer is blocked (see https://bugs.chromium.org/p/chromium/issues/detail?id=727923#c8). To unblock import now, we have to either do this, or fix the is_exportable function to ignore commits with closed PRs.
,
Jun 16 2017
That's not quite what we want to do. We're trying to keep the changes from the exportable commit and avoid resetting to the previous state before the exportable commit. Simple example: there's a new exportable commit that changes external/wpt/foo/x.html from "version 3" to "version 4". The import removes everything in external/wpt and copies files over again from wpt HEAD. After copying wpt HEAD, external/wpt/foo/x.html is now back to "version 3". We want to get it back to "version 4" (how it is at Chromium HEAD). So, I guess what we kind of want to do is to apply the exportable commits on top of local wpt HEAD before importing. But maybe "re-applying exportable commits on wpt HEAD before import" would practically be the same as just resetting any modified files? One thing I was wondering was: if someone makes an exportable commit in directory foo, and meanwhile there are also other changes upstream in directory foo (maybe to the same or different files). Maybe it would make sense to revert the whole directory to Chromium HEAD state and avoid importing any changes in that directory for now. That's kind of what I've been doing manually when making manual imports when there exportable commits.
,
Jun 19 2017
To fix the immediate problem of import being blocked I think tweaking is_exportable in issue 727923 sounds right. As mentioned in both comments by qyearsley@, there are some tricky things that happen when trying to hold back certain files only, we can't actually tell which files belong together. I think that with any `git checkout HEAD FILE` heuristic we could end up in a state that was never intended in Chromium or in wpt and which introduces failures that we'll reset baselines for and commit. It would eventually stabilize, but that will be noise for any future tooling we build to allow teams to track new failures in wpt. I think that "re-applying exportable commits on wpt HEAD before import" is most promising, although I think it's after, in the sense that we'd try to apply the changes on top of a clean copy (import) of wpt. If all of the exportable-but-not-exported changes apply cleanly, then I think automatically importing the result should be fine. That is, after all, equivalent to what will happen if all of those changes are successfully exported and then imported, which also is automatic with no human supervision. P.S. In my original conception of this, the solution was somewhat different. By forcing export to always be in order, we could have picked the two most recent commits of Chromium and web-platform-tests where all the exportable commits were exported, but no more. However, this would have required rewinding a Chromium checkout as well, and there's a simplicity in avoiding that, as well as other benefits from allowing out-of-order exports.
,
Jun 19 2017
That makes sense! Locally reverting exportable commits and then re-applying them after import (and aborting if they could not be cleanly applied) seems like the least hacky way of doing this.
,
Jul 3 2017
,
Jul 21 2017
,
Jul 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6971b156eb3a4bafb79e58c9755e8fab3d12a74b commit 6971b156eb3a4bafb79e58c9755e8fab3d12a74b Author: Quinten Yearsley <qyearsley@google.com> Date: Sat Jul 22 00:06:06 2017 Apply Chromium patches to local wpt and continue Previously, the importer would check for exportable Chromium commits and abort if any existed. This CL would make it so that instead of aborting, the importer would apply the patch to the local WPT before copying files over. This should make it so importer is no longer blocked on exporter in general, but it should still be the case that exportable Chromium changes aren't clobbered. Bug: 734121 Change-Id: I5ca08df2f38a3afe2c98af1cfa8ef1f1ac44eceb Reviewed-on: https://chromium-review.googlesource.com/580574 Commit-Queue: Quinten Yearsley <qyearsley@chromium.org> Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org> Cr-Commit-Position: refs/heads/master@{#488818} [modify] https://crrev.com/6971b156eb3a4bafb79e58c9755e8fab3d12a74b/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py [modify] https://crrev.com/6971b156eb3a4bafb79e58c9755e8fab3d12a74b/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux_unittest.py [modify] https://crrev.com/6971b156eb3a4bafb79e58c9755e8fab3d12a74b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py [modify] https://crrev.com/6971b156eb3a4bafb79e58c9755e8fab3d12a74b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py [modify] https://crrev.com/6971b156eb3a4bafb79e58c9755e8fab3d12a74b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py [modify] https://crrev.com/6971b156eb3a4bafb79e58c9755e8fab3d12a74b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py
,
Jul 22 2017
Will watch out for the next import where there are exportable commits (and reopen or mark verified)!
,
Jul 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9e5da5e4fb5d6fb7a497cb4c97202a01d27b3ea0 commit 9e5da5e4fb5d6fb7a497cb4c97202a01d27b3ea0 Author: Quinten Yearsley <qyearsley@google.com> Date: Mon Jul 24 23:00:17 2017 Improve commit message when there are no locally-applied commits This is a follow-up to https://crrev.com/c/580574. Bug: 734121 Change-Id: Ia51e2931a928261a621af02541bb5dbe715ca391 Reviewed-on: https://chromium-review.googlesource.com/583299 Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org> Commit-Queue: Quinten Yearsley <qyearsley@chromium.org> Cr-Commit-Position: refs/heads/master@{#489120} [modify] https://crrev.com/9e5da5e4fb5d6fb7a497cb4c97202a01d27b3ea0/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
,
Aug 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3e59fca33cbeb316d58d03583c9a4d2d2656d1a5 commit 3e59fca33cbeb316d58d03583c9a4d2d2656d1a5 Author: Philip Jägenstedt <foolip@chromium.org> Date: Fri Aug 04 09:35:27 2017 Update WPT exporter's comment to drop fixed importer issue Bug: 734121 Change-Id: I3393aeda531cde0b4d12644a362a8ba299e0aafe Reviewed-on: https://chromium-review.googlesource.com/597849 Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Cr-Commit-Position: refs/heads/master@{#491975} [modify] https://crrev.com/3e59fca33cbeb316d58d03583c9a4d2d2656d1a5/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py |
||||
►
Sign in to add a comment |
||||
Comment 1 by jeffcarp@chromium.org
, Jun 16 2017