Script for exporting WPT

third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py

+# Copyright 2016 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import re
+
+WPT_REPO_URL = 'https://chromium.googlesource.com/external/w3c/web-platform-tests.git'
+WPT_TMP_DIR = '/tmp/wpt'
+CR_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/'
+
+
+class LocalWPT(object):
+
+    def __init__(self, host, path=WPT_TMP_DIR, no_fetch=False):
+        self.host = host
+        self.fs = self.host.filesystem
+        self.path = path
+        self.run_command = self.host.executive.run_command

[qyearsley, 2016/11/01 22:57:45]

Here, self.fs and self.run_command are shortcuts, but I'm not sure whether it's
worth it; these two attributes could probably be removed (replacing the usages
below with self.host.filesystem and self.host.executive.run_command) without
making the code much more verbose.

If you think it's more readable with these shortcuts, then what do you think
about marking them as protected with an underscore prefix?

+
+        if no_fetch:
+            self.host.print_('## Stop trying to make fetch happen, it\'s not going to happen')

[foolip, 2016/11/01 22:07:44]

When will this message be seen? :)

[jeffcarp, 2016/11/01 23:21:18]

I was using this flag to avoid fetching the repo every time I tested the script.
I'll change the message to something a little less flippant. 

[foolip, 2016/11/02 13:15:32]

:)

+        else:
+            if self.fs.exists(self.path):
+                self.host.print_('## WPT checkout exists at %s, fetching latest' % (self.path))

[qyearsley, 2016/11/01 22:57:45]

Note, I think that rather than printing messages like this (like what
deps_updater.py does right now), it's actually much nicer to use logs.

Most modules in webkitpy have a global logger _log =
logging.getLogger(__name__), and then printing of messages is done with log
methods (debug, info, warning, error). Logging config can be set in a main
method.

This makes it easy to add a verbose mode, or to change how messages are printed.
I'd like to change deps_updater.py to do this in the future.

[jeffcarp, 2016/11/03 23:54:07]

Changing everything to logging. It feels weird though switching from host.print_
which was inherited to _log which is module-level.

+                self.run(['git', 'checkout', 'master'])
+                self.run(['git', 'fetch', '--all'])
+                self.run(['git', 'merge', '--ff-only', 'origin/master'])

[qyearsley, 2016/11/01 22:57:45]

Possible optional change: We could also make the constructor more of a no-op,
and actually run these git commands in a separate setup method. What do you
think?

[jeffcarp, 2016/11/03 23:54:07]

That works. Since all the class is used for is interacting with a local WPT
checkout, I thought it might be redundant, but it's also not expected behavior
to do a bunch of work in __init__.

+            else:
+                self.host.print_('## Cloning %s into %s' % (WPT_REPO_URL, self.path))
+                self.run_command(['git', 'clone', WPT_REPO_URL, self.path])
+                self.host.print_('## Need to set up remote "github"!')

[foolip, 2016/11/01 22:07:45]

Does this need to be done manually? If so, should the script exit at this point?

[jeffcarp, 2016/11/01 23:21:18]

Yes but I'm not entirely sure how that's going to work at the moment. Right now
I'm using my own personal fork at jeffcarp/web-platform-tests.

[foolip, 2016/11/02 13:15:32]

TODO and early exit then? The script doesn't need to really work when first
landed :)

+
+    def run(self, command, **kwargs):
+        """Runs a command in the local WPT directory."""
+        return self.run_command(command, cwd=self.path, **kwargs)
+
+    def run_to_list(self, command, **kwargs):
+        result = self.run(command, **kwargs)
+        return filter(bool, [s.strip() for s in result.split('\n')])

[foolip, 2016/11/01 22:07:44]

Maybe splitlines() here too, won't comment elsewhere.

+
+    def most_recent_cr_commit(self):
+        """Goes back in WPT commit history and gets the most recent commit
+        that contains 'Cr-Commit-Position:'
+        """
+        sha = self.run(['git', 'rev-list', 'HEAD', '-n', '1', '--grep=Cr-Commit-Position'])
+        if not sha:
+            return None, None
+
+        message = self.run(['git', 'show', '-s', '--format=%B', sha])
+        print 'yerah man'

[foolip, 2016/11/01 22:07:44]

:)

+        print message
+        match = re.search(r'Cr-Commit-Position: (?P<cr_commit>.+)$', message, re.MULTILINE)

[foolip, 2016/11/01 22:07:44]

Throw in a ^ so that it only matches at the beginning of the line? Revert
commits say things like this before the real Cr-Commit-Position:
Also, should this extract out just the number, or does that turn out to not be
necessary yet?

[jeffcarp, 2016/11/01 23:21:18]

Even better, I found `git footers --position [sha]` will give you the value of
Cr-Commit-Position.

https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/d...

I don't think we need to parse out the number. In my testing, git accepts revs
of the format `refs/heads/master@{#428769}`. 

[foolip, 2016/11/02 13:15:32]

Note that this only works if there's a local master branch, and in my testing,
it doesn't actually get the same commit that has `refs/heads/master@{#428769}`
as a footer.

+        return sha, match.groupdict()['cr_commit']
+
+    def clean(self):
+        self.run(['git', 'reset', '--hard', 'HEAD'])

[foolip, 2016/11/01 22:07:45]

Perhaps also git clean -fdx? If this is supposed to be robust against local
changes better go all the way. Or maybe exit with an error if there are local
modifications.

+        self.run(['git', 'checkout', 'master'])
+
+    def all_branches(self):
+        return self.run_to_list(['git', 'branch', '-a'])
+
+    def create_branch_with_patch(self, branch_name, message, patch):
+        self.clean()
+        all_branches = self.all_branches()
+        remote_branch_name = 'remotes/github/%s' % branch_name
+
+        if branch_name in all_branches:
+            self.host.print_('## Local branch %s already exists, deleting' % branch_name)
+            self.run(['git', 'branch', '-D', branch_name])
+
+        if remote_branch_name in all_branches:
+            self.host.print_('## Remote branch %s already exists, deleting' % branch_name)
+            self.run(['git', 'push', 'github', ':%s' % branch_name])

[foolip, 2016/11/01 22:07:44]

This might mess things up if there's an open pull request for it. When should we
expect this will happen? In the beginning, maybe err on the side of failing and
requiring manual intervention when unexpected things happen?

[foolip, 2016/11/02 13:15:32]

TODO for this and the following comments in this file. (It's hard to keep track
of comments in a long review, so err on the side of delaying work to make the
review shorter.)

[jeffcarp, 2016/11/03 23:54:07]

I think it should fail if the remote branch already exists. Adding a TODO to
investigate.

+
+        self.host.print_('## Creating local branch %s' % branch_name)
+        self.run(['git', 'checkout', '-b', branch_name])
+
+        # Remove Chromium WPT directory prefix
+        patch = patch.replace(CR_WPT_DIR, '')

[foolip, 2016/11/01 22:07:45]

Maybe this would also be the place to add "Chromium: ". Although I think having
a way to export a match that can be manually be applied using git am will be
needed for conflicts, so having a method that does it all is probably best.
Could be layered on top of a non-modifying one, of course. (No action required
now.)

+
+        # foolip: Could also use git am -p<n> where n is len(CR_WPT_DIR.split(/'))

[foolip, 2016/11/01 22:07:44]

Yep. Can you make this a TODO?

+        #   or something not off-by-one.
+        self.run(['git', 'apply', '-'], input=patch)
+        self.run(['git', 'commit', '-am', message])
+        self.run(['git', 'push', 'github', branch_name])