Bug 53822

Created attachment 81312 [details] Patch

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

Comment on attachment 81312 [details] Patch I guess. Shell is *evil*. I suspect this will cause other troubles.

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.

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.

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*

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.

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.

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?

Comment on attachment 81312 [details] Patch Clearing flags on attachment: 81312 Committed r77720: <http://trac.webkit.org/changeset/77720>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 81312 [details]: http/tests/websocket/tests/send-throw.html bug 53834 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.

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

(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.

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.

(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?

no problem with Windows Python nowadays.

<rdar://problem/136549155>