Bug 169462

Created attachment 304035 [details] Patch

Current patch is creating the branch in the GitHub personal repo. Adding PR creation from that should be straightforward.

Script might be useful for those changing imported/w3c/web-platform-tests tests. Only working for those using git.

Created attachment 305076 [details] Patch

wpt_github.py is a copy of the corresponding chromium file.

Created attachment 309348 [details] Patch

Attachment 309348 [details] did not pass style-queue: ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:46: expected 2 blank lines, found 1 [pep8/E302] [5] ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:43: Bad option value 'unused-argument' [pylint/E0012] [5] ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:43: Bad option value 'unused-argument' [pylint/E0012] [5] ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:44: [MockWeb.request] Instance of 'MockWeb' has no 'responses' member [pylint/E1101] [5] ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:96: [TestExporterTest.test_export] Instance of 'WPTGitHub' has no 'calls' member (but some types could not be inferred) [pylint/E1103] [5] ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:112: [TestExporterTest.test_export] Instance of 'Bugzilla' has no 'calls' member (but some types could not be inferred) [pylint/E1103] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:176: too many blank lines (3) [pep8/E303] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:19: Bad option value 'unused-argument' [pylint/E0012] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:19: Bad option value 'unused-argument' [pylint/E0012] [5] Total errors found: 9 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 309348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309348&action=review > Tools/Scripts/webkitpy/w3c/wpt_github.py:3 > +# 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. We don't have this LICENSE file in our repository. I'm not sure what to do with this license, but we can't just put this in as this patch exists now.

Created attachment 309359 [details] Adding URL to Chromium license

(In reply to Alex Christensen from comment #8) > Comment on attachment 309348 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309348&action=review > > > Tools/Scripts/webkitpy/w3c/wpt_github.py:3 > > +# 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. > > We don't have this LICENSE file in our repository. I'm not sure what to do > with this license, but we can't just put this in as this patch exists now. Good point. I updated the patch to add the chromium repository LICENSE url

Attachment 309359 [details] did not pass style-queue: ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:44: Bad option value 'unused-argument' [pylint/E0012] [5] ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:44: Bad option value 'unused-argument' [pylint/E0012] [5] ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:96: [TestExporterTest.test_export] Instance of 'WPTGitHub' has no 'calls' member (but some types could not be inferred) [pylint/E1103] [5] ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:112: [TestExporterTest.test_export] Instance of 'Bugzilla' has no 'calls' member (but some types could not be inferred) [pylint/E1103] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:180: too many blank lines (3) [pep8/E303] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:23: Bad option value 'unused-argument' [pylint/E0012] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:23: Bad option value 'unused-argument' [pylint/E0012] [5] Total errors found: 7 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ping review?

Comment on attachment 309359 [details] Adding URL to Chromium license View in context: https://bugs.webkit.org/attachment.cgi?id=309359&action=review > Tools/Scripts/webkitpy/w3c/wpt_github.py:7 > +# The LICENSE file referenced above can be found at > +# https://chromium.googlesource.com/chromium/src.git/+/master/LICENSE I think we should include the LICENSE file in this repository to avoid problems if google changes how this server responds, the server location, the location on the server, etc. or if someone doesn't have access to their server for some reason.

(In reply to Alex Christensen from comment #13) > Comment on attachment 309359 [details] > Adding URL to Chromium license > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309359&action=review > > > Tools/Scripts/webkitpy/w3c/wpt_github.py:7 > > +# The LICENSE file referenced above can be found at > > +# https://chromium.googlesource.com/chromium/src.git/+/master/LICENSE > > I think we should include the LICENSE file in this repository to avoid > problems if google changes how this server responds, the server location, > the location on the server, etc. or if someone doesn't have access to their > server for some reason. Talking with Geoff about it, the idea would be to inline the LICENSE in each file. I'll do that in a follow-up or landing patch.

Comment on attachment 309359 [details] Adding URL to Chromium license View in context: https://bugs.webkit.org/attachment.cgi?id=309359&action=review It's exciting to get this script working. By the way, the question of where to put tests to be exported hasn't been settled. e.g. Maciej in https://lists.webkit.org/pipermail/webkit-dev/2017-May/029022.html says: " think it would be cleaner to have a separate directory of tests intended for [export], separate from imported tests. It could be right next to imported/w3c/web-platform-tests. I think mixing tests that are imported from upstream and tests intended for eventual upstreaming is more confusing than helpful." > Tools/Scripts/webkitpy/common/checkout/scm/git.py:587 > + def reset_hard(self, revision): Instead of just using the Git lingo, we should clarify what it does. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:588 > + return self._run_git(['reset', '--hard', revision]) It's strange that we sometimes call this function with revision being a Git hashish but other times it's a branch name. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:590 > + def am(self, options): Please give a more descriptive name like "apply_mail_patch" or "apply_git_patch". > Tools/Scripts/webkitpy/common/checkout/scm/git.py:593 > + args = ['am'] > + args.extend(options) > + return self._run_git(args) More idiomatic way to write this in Python would be self._run_git(['am'] + options) > Tools/Scripts/webkitpy/common/checkout/scm/git.py:595 > + def commit(self, options): I'd call this local_commit or commit_local since scm interface is shared with SVN. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:602 > + def format_patch(self, options): > + args = ['format-patch'] > + args.extend(options) We should just use scm().create_patch(git_commit=commit_id) instead. r- because of this feature duplication. > Tools/Scripts/webkitpy/w3c/test_exporter.py:51 > + self.bugzilla = bugzillaClass() These instance variables should be prefixed with _ since they're more or less private to this class. If they're intended to be publicly accessible variable, then we have too many of those in this class. > Tools/Scripts/webkitpy/w3c/test_exporter.py:52 > + self.webKitGit = gitClass('.', None, executive=self._host.executive, filesystem=self._filesystem) In general, I don't really like tools relying on the current working directory like this. It's fine to use the current working directory in the case no option was specified, but it's better if the directory name of the WPT checkout was explicitly specifiable in an option so that it's easier to call this function from another script for example. Also, why are we assuming that WebKit checkout is a Git checkout? Why can't we just let the host object automatically detect the SCM type? > Tools/Scripts/webkitpy/w3c/test_exporter.py:60 > + self.repository_directory = options.repository_directory It's not obvious at all whether this repository refers to WebKit checkout to WPT checkout. We should probably prefix each and every instance variable in this class by _webkit_ or _wpt_. > Tools/Scripts/webkitpy/w3c/test_exporter.py:66 > + self.username = options.username > + if not self.username: > + self.username = os.environ.get('GITHUB_WPT_USER') > + if not self.username: > + raise ValueError("Missing GitHub username.") We should also get it from git config user.name and keychain. See webkitpy/common/net/credentials.py r- because of this. > Tools/Scripts/webkitpy/w3c/test_exporter.py:78 > + self.branch_name = "webkit-" + str(self.bug_id) > + self.bugzilla_url = "https://bugs.webkit.org/show_bug.cgi?id=" + str(self.bug_id) > + self.commit_title = 'WebKit export of ' + self.bugzilla_url We should use % or .format. I also think "webkit-169462" is not exotic enough not to conflict with user-created branch name. It should be something more unique like "wpt-export-for-webkit-169462" In addition, it's ambiguous as to what commit_title means in this context. It's the commit message / PR description in WPT, so I think a better name would be self._wpt_commit_message or self._wpt_pr_title. Finally, "WebKit export of 169462" is a strange title for a PR request. I would have phrased it like "Test updates for WebKit bug 169462". > Tools/Scripts/webkitpy/w3c/test_exporter.py:82 > + if not self.repository_remote: > + self.repository_remote = self.username What's the point of this code? Why do we need to push to a specific user's repo? I've never had to specify repository name like this since I usually only have one remote in my GitHub fork. > Tools/Scripts/webkitpy/w3c/test_exporter.py:84 > + self.repository_origin_url = "https://" + self.username + "@github.com/" + self.username + "/web-platform-tests.git" We should just run git config --get remote.origin.url instead. > Tools/Scripts/webkitpy/w3c/test_exporter.py:85 > + self.repository_public_url = "https://github.com/" + self.username + "/web-platform-tests.git" if not options.repository_public_url else options.repository_public_url This instance variable is never used other than the line below so it should be a local variable. > Tools/Scripts/webkitpy/w3c/test_exporter.py:86 > + self.repository_public_tree_url = self.repository_public_url[:-4] + "/tree/" Are we truncating ".git" by [:-4]? That is not obvious at all. Also, it looks like this is only used in logging. If we used "git config --get" above (please add a new method on Git class), then we should be able to parse that result and construct this URL instead. > Tools/Scripts/webkitpy/w3c/test_exporter.py:95 > + def _init_repository(self, url, repository_directory, gitClass): I think we should call this _ensure_wpt_checkout or something since we're not init'ing a new git repository. > Tools/Scripts/webkitpy/w3c/test_exporter.py:109 > + patchOptions = ["--no-update", "--no-clean", "--local-commit"] Nit: should be patch_options. > Tools/Scripts/webkitpy/w3c/test_exporter.py:121 > + # webKitPatch = WebKitPatch(self._filesystem.join(self._filesystem.abspath(__file__), "..", "..")) > + #(options, args) = webKitPatch.parse_args(patchOptions) > + #return webKitPatch.check_arguments_and_execute(options, args) Please remove this commented out code. r- because of this. > Tools/Scripts/webkitpy/w3c/test_exporter.py:128 > + self.git.reset_hard('origin/master') > + if not self.options.git_commit: > + self.webKitGit.reset_hard('HEAD~1') It's confusing that we're calling reset_hard with a branch name or hashish. > Tools/Scripts/webkitpy/w3c/test_exporter.py:138 > + _log.info("Retrying to create the branch") > + self.git.delete_branch(self.branch_name) > + self.git.checkout_new_branch(self.branch_name) I don't think we should automatically delete a branch created by an user like this. We should ask the user to confirm it's okay to delete the branch at minimum. We can also add a flag to automatically delete the branch if there is a conflicting one but doing so by default without giving any warning to the user is dangerous. r- because of this. > Tools/Scripts/webkitpy/w3c/test_exporter.py:141 > + try: > + self.git.am([patch, '--exclude', '*-expected.txt']) > + except Exception as e: It's not a good idea to capture any exception like this. Also, capturing the exception should probably happen in git.apply_patch (or whatever its name will be) instead. That way, that function can take a patch name and files to exclude as arguments instead o an arbitrary list of options. > Tools/Scripts/webkitpy/w3c/test_exporter.py:148 > + def push_to_public_repository(self): This is a very vague name. It should be something like push_to_remote_wpt_fork. > Tools/Scripts/webkitpy/w3c/test_exporter.py:157 > + response = self.gitHub.create_pr(self.repository_remote + ':' + self.branch_name, self.commit_title, description) It's not obvious at all that the first argument is a branch name. I think we should use the name arguments as in: create_pr(remote_branch_name=self.repository_remote + ':' + self.branch_name). > Tools/Scripts/webkitpy/w3c/test_exporter.py:170 > + patch_data = self.webKitGit.format_patch([git_commit, 'LayoutTests/imported/w3c/web-platform-tests', '--stdout']) Again, we should be using scm.create_patch(git_commit=commit_id) instead. > Tools/Scripts/webkitpy/w3c/test_exporter.py:175 > + patch_data = patch_data.replace('LayoutTests/imported/w3c/web-platform-tests/', '') It seems a little risky to do a global replaccement like this but I suppose text like this would never appear in the tests themselves. > Tools/Scripts/webkitpy/w3c/test_exporter.py:184 > + _log.info("Will push branch to " + self.git.remote(["get-url", self.repository_remote]).strip()) > + return > + _log.info("Will push branch to " + self.repository_origin_url) Again, it's not obvious what repository_remote and repository_origin_url refer to. > Tools/Scripts/webkitpy/w3c/test_exporter.py:222 > + - USERNAME may be set using the environment variable GITHUB_WPT_USER or as a command line option. > + - Github credential may be set using the environment variable GITHUB_WPT_AUTH or as a command line option. (Please provide a valid GitHub 'Personal access token' with 'repo' as scope) We should support pulling username & password from the keychain. Having to do this extra step is really annoying. Even asking the user the password manually is better than requiring the user to get this extra token thing. Also, if you have SSH key setup with GitHub, it should just work without having to do any authentication.

Comment on attachment 309359 [details] Adding URL to Chromium license View in context: https://bugs.webkit.org/attachment.cgi?id=309359&action=review Added some comments on the Git additions. >> Tools/Scripts/webkitpy/common/checkout/scm/git.py:587 >> + def reset_hard(self, revision): > > Instead of just using the Git lingo, we should clarify what it does. Maybe a docstring can do this? I find it written as-is to be the ideal way to write it as someone who's used git for some time. The same way for an SVN function that calls 'svn revert' I would expect it to be named 'revert' on an SVN class. >> Tools/Scripts/webkitpy/common/checkout/scm/git.py:588 >> + return self._run_git(['reset', '--hard', revision]) > > It's strange that we sometimes call this function with revision being a Git hashish but other times it's a branch name. Maybe a better name for revision would be commit_identifier? Or commit_reference? >> Tools/Scripts/webkitpy/common/checkout/scm/git.py:590 >> + def am(self, options): > > Please give a more descriptive name like "apply_mail_patch" or "apply_git_patch". Given we're using the `am` argument, and this `Git` class is supposed to be representative of the `git` binary (as I understand it), am seems correct. That being said, I have not used this command before, so it may be worth writing a docstring explaining what the function is expected to do. >> Tools/Scripts/webkitpy/common/checkout/scm/git.py:593 >> + return self._run_git(args) > > More idiomatic way to write this in Python would be self._run_git(['am'] + options) +1. >> Tools/Scripts/webkitpy/common/checkout/scm/git.py:595 >> + def commit(self, options): > > I'd call this local_commit or commit_local since scm interface is shared with SVN. Maybe we should have commit do a few things as to adhere to the SVN interface (or maybe this is a terrible idea): 1. Add a push=True default argument to the `commit` function definition, which adheres to the SVN interface, and therefore unifies the SCM interface(s). 2. locally commit. 3. update the repository and push if push=True. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:625 > if quiet: There should be Git unit tests near this file; can you please add some for the new functionality and ensure existing tests still pass?

Thanks for the review. I'll try to provide a new version of the patch soon. > By the way, the question of where to put tests to be exported hasn't been > settled. > e.g. Maciej in > https://lists.webkit.org/pipermail/webkit-dev/2017-May/029022.html says: > " think it would be cleaner to have a separate directory of tests intended > for [export], separate from imported tests. It could be right next to > imported/w3c/web-platform-tests. I think mixing tests that are imported from > upstream and tests intended for eventual upstreaming is more confusing than > helpful." That is how all other browsers are doing. Splitting the folders will make it very hard to export tests given that some links to common resources will no longer work. > What's the point of this code? Why do we need to push to a specific user's > repo? > I've never had to specify repository name like this since I usually only > have one remote in my GitHub fork. The point here is that one pushes the commit to a GitHub repository. Then a PR request is made from that repository to the WPT repository. The local WPT repo will have these two remotes, one for getting updates from WPT and one to push changes. Ideally, it should be a bot account/bot repository since the bot should run this script. This should also work with whoever wants to run the script on its own. > > Tools/Scripts/webkitpy/w3c/test_exporter.py:222 > > + - USERNAME may be set using the environment variable GITHUB_WPT_USER or as a command line option. > > + - Github credential may be set using the environment variable GITHUB_WPT_AUTH or as a command line option. (Please provide a valid GitHub 'Personal access token' with 'repo' as scope) > > We should support pulling username & password from the keychain. > Having to do this extra step is really annoying. > Even asking the user the password manually is better than requiring the user > to get this extra token thing. > Also, if you have SSH key setup with GitHub, it should just work without > having to do any authentication. The token is used for creating the PR not for pushing to the user repo.

Hi everyone, I'm the current maintainer of the Chromium import/export codebase. Glad to see the code is useful to you! Also happy to see WebKit setting up auto export! More contributors, better WPT :) I notice the patch is a few months old. In the past few months, we've made some major cleanups and fixed a few bugs/TODOs in wpt_github.py. Most notably, the API and error handling are much more consistent now. Instead of swallowing the error and returning None, an exception is thrown for all unexpected API errors. It'd be great if you could pull in the newest version, Youenn. The API indeed changed a bit in the process. Looks like the only change needed in this patch is the use of WPTGitHub.create_pr. Instead of returning the full response dict, a PR issue number (int) is now returned. Hence, you need to construct the PR URL from a string template rather than using `response['html_url']`. (And you might want to catch WPTGitHub.GitHubError and retry if you want to make the script more robust against server errors.) Let me know if you have any questions! Thanks again!

(In reply to youenn fablet from comment #17) > Thanks for the review. > I'll try to provide a new version of the patch soon. > > > By the way, the question of where to put tests to be exported hasn't been > > settled. > > e.g. Maciej in > > https://lists.webkit.org/pipermail/webkit-dev/2017-May/029022.html says: > > " think it would be cleaner to have a separate directory of tests intended > > for [export], separate from imported tests. It could be right next to > > imported/w3c/web-platform-tests. I think mixing tests that are imported from > > upstream and tests intended for eventual upstreaming is more confusing than > > helpful." > > That is how all other browsers are doing. I don't think that's a good enough reason to take that approach. You need to get the community consensus before you can take that approach. > Splitting the folders will make it very hard to export tests given that some > links to common resources will no longer work. Why? We can just make a copy of resources or superimpose files to be exported on top of existing files when running tests. > > > Tools/Scripts/webkitpy/w3c/test_exporter.py:222 > > > + - USERNAME may be set using the environment variable GITHUB_WPT_USER or as a command line option. > > > + - Github credential may be set using the environment variable GITHUB_WPT_AUTH or as a command line option. (Please provide a valid GitHub 'Personal access token' with 'repo' as scope) > > > > We should support pulling username & password from the keychain. > > Having to do this extra step is really annoying. > > Even asking the user the password manually is better than requiring the user > > to get this extra token thing. > > Also, if you have SSH key setup with GitHub, it should just work without > > having to do any authentication. > > The token is used for creating the PR not for pushing to the user repo. Regardless, it's not okay to require users to set random environmental variables like these.

Created attachment 323815 [details] Patch

Attachment 323815 [details] did not pass style-queue: Traceback (most recent call last): File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module> main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main WebKitPatch(os.path.abspath(__file__)).main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 54, in run args.extend(self._changed_files(state)) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 44, in _changed_files return self.cached_lookup(state, "changed_files") File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 62, in cached_lookup state[key] = promise(self, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 51, in <lambda> "changed_files": lambda self, state: self._tool.scm().changed_files(self._options.git_commit), File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 235, in changed_files status_command = [self.executable_name, 'diff', '-r', '--name-status', "--no-renames", "--no-ext-diff", "--full-index", self.merge_base(git_commit), '--'] File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 215, in merge_base return self.remote_merge_base() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 517, in remote_merge_base return self._run_git(['merge-base', self.remote_branch_ref(), 'HEAD']).strip() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 524, in remote_branch_ref if not self._branch_ref_exists(remote_master_ref): AttributeError: 'Git' object has no attribute '_branch_ref_exists' If any of these errors are false positives, please file a bug against check-webkit-style.

Let's start with small steps. The proposed script would be used by WebKit developers to make contributions to WPT by directly making edits to LayoutTests/imported/w3c/web-platform-tests. The usual rules (commit WPT changes to WebKit when merged in WPT, merge WPT PR if r+ed in WebKit) remain unchanged.

Comment on attachment 323815 [details] Patch Again, I don't think we should take this approach until such a consensus is reached on webkit-dev. People had varying opinions of how this should work, but one most obvious feedback was that we didn't want to modify imported tests directly.

> Again, I don't think we should take this approach until such a consensus is > reached on webkit-dev. I'll be happy to ping webkit-dev but I am not sure how much it will help for this particular patch. This script is allowing to author changes to existing WPT tests/create new WPT tests in a WebKit checkout instead of a separate WPT checkout. Coupling this authoring with WebKit patch authoring has obvious benefits, as well as coupling bugzilla patch/WPT PR processes. It may also be seen as a step towards one solution for which I am fine discussing more the details. But it is useful on its own. > People had varying opinions of how this should work, but one most obvious > feedback was that we didn't want to modify imported tests directly. The concern I heard was about WebKit repository containing changes in LayoutTests/imported/w3c/web-platform-tests that would not be in WPT GitHub repository. I do not see how this particular script goes against this valid concern. Are there other concerns?

(In reply to youenn fablet from comment #24) > > Again, I don't think we should take this approach until such a consensus is > > reached on webkit-dev. > > I'll be happy to ping webkit-dev but I am not sure how much it will help for > this particular patch. > > This script is allowing to author changes to existing WPT tests/create new > WPT tests in a WebKit checkout instead of a separate WPT checkout. Coupling > this authoring with WebKit patch authoring has obvious benefits, as well as > coupling bugzilla patch/WPT PR processes. > > It may also be seen as a step towards one solution for which I am fine > discussing more the details. > But it is useful on its own. The problem here is that the previous webkit-dev discussion resulted in people opposing to the workflow of modifying imported scripts. This script will encourage that workflow.

Oops, I probably should not have done review+ given the discussion above.

We discussed this with Ryosuke and agreed with this first step. There was no push back on WebKit dev either. I’ll wait to land the patch for a few days in case I misunderstood things.

(In reply to Robert Ma from comment #18) > Hi everyone, I'm the current maintainer of the Chromium import/export > codebase. Glad to see the code is useful to you! Also happy to see WebKit > setting up auto export! More contributors, better WPT :) > > I notice the patch is a few months old. In the past few months, we've made > some major cleanups and fixed a few bugs/TODOs in wpt_github.py. Most > notably, the API and error handling are much more consistent now. Instead of > swallowing the error and returning None, an exception is thrown for all > unexpected API errors. It'd be great if you could pull in the newest > version, Youenn. > > The API indeed changed a bit in the process. Looks like the only change > needed in this patch is the use of WPTGitHub.create_pr. Instead of returning > the full response dict, a PR issue number (int) is now returned. Hence, you > need to construct the PR URL from a string template rather than using > `response['html_url']`. (And you might want to catch WPTGitHub.GitHubError > and retry if you want to make the script more robust against server errors.) > > Let me know if you have any questions! Thanks again! Thanks for the information, the patch now uses the latest wpt_github.py now.

Created attachment 328944 [details] Patch for landing

Attachment 328944 [details] did not pass style-queue: Traceback (most recent call last): File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module> main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main WebKitPatch(os.path.abspath(__file__)).main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 54, in run args.extend(self._changed_files(state)) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 44, in _changed_files return self.cached_lookup(state, "changed_files") File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 62, in cached_lookup state[key] = promise(self, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 51, in <lambda> "changed_files": lambda self, state: self._tool.scm().changed_files(self._options.git_commit), File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 235, in changed_files status_command = [self.executable_name, 'diff', '-r', '--name-status', "--no-renames", "--no-ext-diff", "--full-index", self.merge_base(git_commit), '--'] File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 215, in merge_base return self.remote_merge_base() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 517, in remote_merge_base return self._run_git(['merge-base', self.remote_branch_ref(), 'HEAD']).strip() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 524, in remote_branch_ref if not self._branch_ref_exists(remote_master_ref): AttributeError: 'Git' object has no attribute '_branch_ref_exists' If any of these errors are false positives, please file a bug against check-webkit-style.

Updated the patch to add a --remote-url option so that ssh GitHub configurations can be supported. It might be beneficial to split the configuration of the WPT clone in its own dedicated script.

Created attachment 328945 [details] Patch for landing

Attachment 328945 [details] did not pass style-queue: Can't find a branch to diff against. svn-remote.svn.fetch is not in the git config and refs/remotes/origin/master does not exist If any of these errors are false positives, please file a bug against check-webkit-style.

The commit-queue encountered the following flaky tests while processing attachment 328945 [details]: media/modern-media-controls/seek-backward-support/seek-backward-support.html bug 174916 (authors: graouts@apple.com, mcatanzaro@igalia.com, and ryanhaddad@apple.com) The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 328945 [details]: svg/animations/svgtransform-animation-1.html bug 179354 (authors: ap@webkit.org, arv@chromium.org, krit@webkit.org, mark.lam@apple.com, and zimmermann@kde.org) The commit-queue is continuing to process your patch.

Comment on attachment 328945 [details] Patch for landing Clearing flags on attachment: 328945 Committed r225737: <https://trac.webkit.org/changeset/225737>

All reviewed patches have been landed. Closing bug.

<rdar://problem/35960387>

Comment on attachment 328945 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=328945&action=review > Tools/Scripts/webkitpy/common/checkout/scm/git.py:524 > - if not self._branch_ref_exists(remote_master_ref): > + if not self.branch_ref_exists(remote_master_ref): This breaks uploads from a Git clone of the repository, since now `git show-ref` is verifying the 'refs/heads/refs/remotes/origin/master' ref, which doesn't exist, while before it verified the 'refs/remotes/origin/master' ref.

(In reply to Build Bot from comment #33) > Attachment 328945 [details] did not pass style-queue: > > Can't find a branch to diff against. svn-remote.svn.fetch is not in the git > config and refs/remotes/origin/master does not exist > > > If any of these errors are false positives, please file a bug against > check-webkit-style. This is an example of that error. All EWSs seem to be affected, so I'll roll this out. Otherwise, a temporary workaround for this would be adding a svn-remote.svn.fetch entry: $ git config --add svn-remote.svn.fetch "refs/remotes/origin/master"

Reverted r225737 for reason: Breaks Committed r225742: <https://trac.webkit.org/changeset/225742>

Created attachment 329587 [details] Fixing refs/heads regression

Attachment 329587 [details] did not pass style-queue: ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:44: Bad option value 'unused-argument' [pylint/E0012] [5] ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:44: Bad option value 'unused-argument' [pylint/E0012] [5] ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:130: [TestExporterTest.test_export] Instance of 'Bugzilla' has no 'calls' member (but some types could not be inferred) [pylint/E1103] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:91: whitespace before ')' [pep8/E202] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:159: whitespace before ')' [pep8/E202] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:178: whitespace before ')' [pep8/E202] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:228: whitespace before ')' [pep8/E202] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:260: whitespace before ')' [pep8/E202] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:280: whitespace before ')' [pep8/E202] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:305: whitespace before ')' [pep8/E202] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:330: whitespace before ')' [pep8/E202] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:409: whitespace before ')' [pep8/E202] [5] ERROR: Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:35: Bad option value 'unused-argument' [pylint/E0012] [5] Total errors found: 13 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to Zan Dobersek from comment #41) > Reverted r225737 for reason: > > Breaks > > Committed r225742: <https://trac.webkit.org/changeset/225742> Thanks Zan for reverting it. I fixed the patch and the style checker seems happy now.

Comment on attachment 329587 [details] Fixing refs/heads regression Clearing flags on attachment: 329587 Committed r226009: <https://trac.webkit.org/changeset/226009>

All reviewed patches have been landed. Closing bug.