New issue
Advanced search Search tips

Issue 711088 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

webkit-patch rebaseline-cl doesn't work on Windows

Project Member Reported by bsep@chromium.org, Apr 12 2017

Issue description

Running webkit-patch rebaseline-cl crashes on Windows. I investigated and it looks like it's because when Executive.popen tries to run "git rev-parse --is-inside-work-tree" it doesn't pass shell="True" and Windows fails to run the command.

I wrote a small test script that demonstrates the problem more succinctly. Without shell="True" the script fails.

import subprocess
process = subprocess.Popen(["git", "rev-parse", "--is-inside-work-tree"], shell="True")
process.communicate()

Full output of webkit-patch rebaseline-cl:

The current directory (None) is not in a git repo, trying directory C:\src\chrome\src\third_party\WebKit\Tools\Scripts\webkitpy\common\checkout.
Failed to find Git repo for None or C:\src\chrome\src\third_party\WebKit\Tools\Scripts\webkitpy\common\checkout
Traceback (most recent call last):
  File "third_party\WebKit\Tools\Scripts\webkit-patch", line 84, in <module>
    main()
  File "third_party\WebKit\Tools\Scripts\webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "C:\src\chrome\src\third_party\WebKit\Tools\Scripts\webkitpy\tool\webkit_patch.py", line 120, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "C:\src\chrome\src\third_party\WebKit\Tools\Scripts\webkitpy\tool\commands\command.py", line 110, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "C:\src\chrome\src\third_party\WebKit\Tools\Scripts\webkitpy\tool\commands\rebaseline_cl.py", line 56, in execute
    issue_number = self._get_issue_number()
  File "C:\src\chrome\src\third_party\WebKit\Tools\Scripts\webkitpy\tool\commands\rebaseline_cl.py", line 107, in _get_issue_number
    issue = self.git_cl().get_issue_number()
  File "C:\src\chrome\src\third_party\WebKit\Tools\Scripts\webkitpy\common\net\git_cl.py", line 51, in get_issue_number
    return self.run(['issue']).split()[2]
  File "C:\src\chrome\src\third_party\WebKit\Tools\Scripts\webkitpy\common\net\git_cl.py", line 36, in run
    return self._host.executive.run_command(command, cwd=self._cwd)
  File "C:\src\chrome\src\third_party\WebKit\Tools\Scripts\webkitpy\common\system\executive.py", line 321, in run_command
    close_fds=self._should_close_fds())
  File "C:\src\chrome\src\third_party\WebKit\Tools\Scripts\webkitpy\common\system\executive.py", line 389, in popen
    return subprocess.Popen(string_args, **kwargs)
  File "C:\src\depot_tools\python276_bin\lib\subprocess.py", line 709, in __init__
    errread, errwrite)
  File "C:\src\depot_tools\python276_bin\lib\subprocess.py", line 957, in _execute_child
    startupinfo)
WindowsError: [Error 2] The system cannot find the file specified
 
Cc: jeffcarp@chromium.org
Thanks again for filing this --

So, what I found was that in webkitpy/common/checkout/git.py we already have a hack to make git commands work Windows (assuming that the git executable that we're using is actually C:\src\depot_tools\git.bat):
  https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/git.py?l=68

But I didn't copy this hack over to webkitpy/common/net/git_cl.py when I wrote that:
  https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/net/git_cl.py?l=33

The easy solution would be to reuse that Git._init_executable_name to find the right executable name in Windows.

Jeff: Also it's worth noting that anywhere else where we invoke Executable.run_command(["git", ...]) directly is going to be affected by this too, including the stuff in webkitpy/w3c/..., which is one argument for doing issue 676399.
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e5a158db9c3a0720c8390fc89df447b1b52d3d0f

commit e5a158db9c3a0720c8390fc89df447b1b52d3d0f
Author: qyearsley <qyearsley@chromium.org>
Date: Fri Apr 14 00:09:33 2017

Clean up webkitpy.common.checkout.git module.

Purpose: This is done to prepare to reuse the git executable init hack
in git cl so that rebaseline-cl works on Windows.

In this change:
 - Rename _run_git to run
 - Remove completely unused methods
 - Edit comments/docstrings

BUG= 711088 

Review-Url: https://codereview.chromium.org/2815093005
Cr-Commit-Position: refs/heads/master@{#464618}

[modify] https://crrev.com/e5a158db9c3a0720c8390fc89df447b1b52d3d0f/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/git.py
[modify] https://crrev.com/e5a158db9c3a0720c8390fc89df447b1b52d3d0f/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/git_unittest.py

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/699ff2a6613e4dbd2b9357f4845c687e7104f41c

commit 699ff2a6613e4dbd2b9357f4845c687e7104f41c
Author: qyearsley <qyearsley@chromium.org>
Date: Thu Apr 20 17:56:21 2017

Follow-up fix to change in Git module (r465813)

In https://codereview.chromium.org/2818223002, I accidentally
removed a `return` that shouldn't have been removed.

BUG= 711088 

Review-Url: https://codereview.chromium.org/2827953003
Cr-Commit-Position: refs/heads/master@{#466064}

[modify] https://crrev.com/699ff2a6613e4dbd2b9357f4845c687e7104f41c/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/git.py

Status: Fixed (was: Assigned)
Should be fixed now, thanks bsep@ for filing!

Sign in to add a comment