test-webkitpy crashes with a WindowsError when run under Win32 Python

• Component: Tools / Tests
• Status: REOPENED ---

[Tony Chang]

get test-webkitpy running on win32 python

[Tony Chang]

Created attachment 81312 [details]
Patch

[Tony Chang]

For reference, here's the current bot output:
http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/8789/steps/webkitpy-test/logs/stdio

[Eric Seidel (no email)]

Comment on attachment 81312 [details]
Patch

I guess.  Shell is *evil*.  I suspect this will cause other troubles.

[Eric Seidel (no email)]

Comment on attachment 81312 [details]
Patch

Should we do our own path resolution instead?  I feel like at some other point I needed/wanted a "which" command written in python.  I guess if we don't use a shell, we won't get the environment in which PATH is defined anyway.

[Evan Martin]

FWIW, I would guess eseidel's dislike of going through a shell because of quoting issues, but on on Windows the low-level process creation API takes a string so you need to get quoting right anyway (http://msdn.microsoft.com/en-us/library/ms682425%28VS.85%29.aspx).  Python implicitly converts the array of args to a quoted command line for you on Windows.  Passing shell=True on Windows is pretty standard in Python code I see, though I admit it's all within a small circle of people.

[Tony Chang]

Mid-air conflict while I was writing pretty much what Evan wrote.

The only thing I have to add is that since we expect subprocess.Popen to include the environment on other platforms, it seems to be semantically consistent to for us to use shell=True on Windows.  Although in practice, maybe we don't need the environment to be copied to subprocesses.

*shrug*

[Eric Seidel (no email)]

We've talked about creating our own POpen subclass to fix some of the popen bugs we work-around in Executive.   This might be further (mild) motivation for such.

[Adam Roben (:aroben)]

I don't remember running into this problem back when I was working on getting test-webkitpy working with Win32 Python. But test-webkitpy has undoubtedly changed since then.

[Adam Roben (:aroben)]

The subprocess docs say this:

The only reason you would need to specify shell=True on Windows is where the command you wish to execute is actually built in to the shell, eg dir, copy. You don’t need shell=True to run a batch file, nor to run a console-based executable.

http://docs.python.org/library/subprocess.html#using-the-subprocess-module

So…are the docs wrong? Is there some other reason you'd have to pass shell=True on Windows?

[WebKit Commit Bot]

Comment on attachment 81312 [details]
Patch

Clearing flags on attachment: 81312

Committed r77720: <http://trac.webkit.org/changeset/77720>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 81312 [details]:

http/tests/websocket/tests/send-throw.html bug 53834 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77720 might have broken GTK Linux 64-bit Debug

[Tony Chang]

(In reply to comment #9)
> The subprocess docs say this:
> 
> The only reason you would need to specify shell=True on Windows is where the command you wish to execute is actually built in to the shell, eg dir, copy. You don’t need shell=True to run a batch file, nor to run a console-based executable.
> 
> http://docs.python.org/library/subprocess.html#using-the-subprocess-module
> 
> So…are the docs wrong? Is there some other reason you'd have to pass shell=True on Windows?

The docs are correct, it just doesn't enumerate all the reasons to use the shell on Windows.  We needed it to search the path for things like 'svn' or 'ruby'.  Here's the python bug tracking the difference:
  http://bugs.python.org/issue8557

Anyway, this was rolled out because when shell=True and the command isn't found, we raise ScriptError rather then OSError.

[Eric Seidel (no email)]

ScriptError was originally creaed as part of scm.py to indicate when git/svn returned non-zero.

These days it's used for more than that I fear.

But still we should return OSError when  we can't find a command instead of ScriptError.

[Tony Chang]

(In reply to comment #15)

> But still we should return OSError when  we can't find a command instead of ScriptError.

Hmm, I'm not sure how to do this.  The return code is 1, but that doesn't always mean that the command wasn't found.  I could search the command output to see if the command wasn't found, but I suspect for non-English versions of Windows, the text will differ.

Eric's original suggestion of writing our own path resolution is possible (it's a cross product of PATH and PATHEXT), but I worry that we'll get it wrong.  E.g., is it possible for other environment variables to be in %PATH%?  What are the rules for case insensitivity?