All,
I wanted to pass this one around before opening an issue on it. When running the unit test for popen via rt.bat (in PCBuild8), I received the following error:
=== BEGIN ERROR ===
C:\Documents and Settings\joe\Desktop\Development\Python\trunk\PCbuild8>rt test_popen Deleting .pyc/.pyo files ... 43 .pyc deleted, 0 .pyo deleted
C:\Documents and Settings\joe\Desktop\Development\Python\trunk\PCbuild8>win32Release\python.exe -E -tt ../lib/test/regrtest.py te st_popen test_popen test test_popen failed -- Traceback (most recent call last): File "C:\Documents and Settings\joe\Desktop\Development\Python\trunk\lib\test\test_popen.py", line 31, in test_popen ["foo", "bar"] File "C:\Documents and Settings\joe\Desktop\Development\Python\trunk\lib\test\test_popen.py", line 24, in _do_test_commandline got = eval(data)[1:] # strip off argv[0] File "<string>", line 0
^
SyntaxError: unexpected EOF while parsing
1 test failed: test_popen
=== END ERROR ===
Only naturally, I looked into what was causing it and noticed the following: Line 23 of the test_popen.py appears to be returning '' and assigning this to data.
data = os.popen(cmd).read()
The problem with is, the next line (24) assumes the previous line will work and goes on to perform the following strip and assert:
got = eval(data)[1:] # strip off argv[0] self.assertEqual(got, expected)
So, in a perfect world, ['-c','foo','bar']\n is what data Should be. I put some quick debug statements after line 23 in test_popen.py to verify this and I observed the following:
data= cmd= "C:\Documents and Settings\joe\Desktop\Development\Python\trunk\PCbuild8\win32Release\python.exe" -c "import sys;print sys.argv" foo bar
Now, on to the 'interesting' part. From the command line, observe the following:
C:\Documents and Settings\joe\Desktop\Development\Python\trunk\PCbuild8\win32release>python -c "import sys; print sys.argv" foo bar ['-c', 'foo', 'bar']
Outside of the popen call failing. I am wondering if an appropriate assert should be performed on the data object, prior to line 24.
In addition, if you debug into the posixmodule, this is the scoop:
- breakpoint set in posixmodule at the start of posix_popen
- i run in debug
- run the following:
import os tmp = os.popen('"C:/Documents and Settings/joe/Desktop/Development/Python/trunk/PCbuild8/win32Release/python.exe" -c "import sys;print sys.argv" foo bar')
call enters posixmodule posix_popen and follows path: f = _PyPopen(cmdstring, tm | _O_TEXT, POPEN_1);
enters posixmodule: _PyPopen
enters posixmodule: _PyPopenCreateProcess
enters posixmodule linen 4920 where the CreateProcess is... s2 checks out as: "C:\WINDOWS\system32\cmd.exe /c "C:/Documents and Settings/joe/Desktop/Development/Python/trunk/PCbuild8/win32Release/python.exe" -c "import sys;print sys.argv" foo bar"
this call returns nonzero, which means it "succeeded". see: [ http://msdn2.microsoft.com/en-us/library/ms682425.aspx ]
On another note, I ran across CreateProcessW and am interested in questioning whether or not this has a place in posixmodule?
Any on yet another note, when I ran test_popen.py straight from /lib (using my std::Python25 install, I obtained the following debug output in the same statement of interest)
data=['-c', 'foo', 'bar'] cmd=c:\python25\python.exe -c "import sys;print sys.argv" foo bar
Your thoughts ?
Joseph Armbruster
All,
I wanted to pass this one around before opening an issue on it. When running the unit test for popen via rt.bat (in PCBuild8), I received the following error:
=== BEGIN ERROR ===
C:\Documents and Settings\joe\Desktop\Development\Python\trunk\PCbuild8>rt test_popen Deleting .pyc/.pyo files ... 43 .pyc deleted, 0 .pyo deleted
C:\Documents and Settings\joe\Desktop\Development\Python\trunk\PCbuild8>win32Re lease\python.exe -E -tt ../lib/test/regrtest.py test_popen test_popen test test_popen failed -- Traceback (most recent call last): File "C:\Documents and Settings\joe\Desktop\Development\Python...
I can't reproduce this. I expect you will find it is due to the space in the filename of your Python directory, via cmd.exe's documented behaviour with quote characters. A patch that allows the test suite to work in such an environment would be welcome, but I think you might end up needing access to GetShortPathName() rather than CreateProcess().
Cheers,
Mark
Mark,
Sounds good, I will get patching tonight. Any thoughts on CreateProcessW ?
Joseph Armbruster
On 6/4/07, Mark Hammond mhammond@skippinet.com.au wrote: >
All,
I wanted to pass this one around before opening an issue on it. When running the unit test for popen via rt.bat (in PCBuild8), I received the following error:
=== BEGIN ERROR ===
C:\Documents and Settings\joe\Desktop\Development\Python\trunk\PCbuild8>rt test_popen Deleting .pyc/.pyo files ... 43 .pyc deleted, 0 .pyo deleted
C:\Documents and Settings\joe\Desktop\Development\Python\trunk\PCbuild8>win32Re lease\python.exe -E -tt ../lib/test/regrtest.py test_popen test_popen test test_popen failed -- Traceback (most recent call last): File "C:\Documents and Settings\joe\Desktop\Development\Python...
I can't reproduce this. I expect you will find it is due to the space in the filename of your Python directory, via cmd.exe's documented behaviour with quote characters. A patch that allows the test suite to work in such an environment would be welcome, but I think you might end up needing access to GetShortPathName() rather than CreateProcess().
Cheers,
Mark
Mark,
My apologies for being a day late, got working on some other things. So here's the scoop as it relates to the issue at hand:
If you run rt.bat from the trunk as-is and place it in a path that contains an empty space, you receive the error outlined in resultwithspace.txt.
If you run rt.bat from the trunk as-is and place it in a path that does not contain an empty space, you receive no errors as outlined in resultwithoutspace.txt.
If you run rt.bat with the patch, on Windows XP, you receive no errors as outlined in resultafterpatch.txt.
The patch is attached. Probably my biggest question now is the use of GetVersion as opposed to GetVersionEx. According to the MSDN, it doesn't appear to be all that undesirable:
http://msdn2.microsoft.com/en-us/library/ms724451.aspx
Your thoughts?
Joseph Armbruster
Joseph Armbruster wrote:
Mark,
Sounds good, I will get patching tonight. Any thoughts on CreateProcessW ?
Joseph Armbruster
On 6/4/07, Mark Hammond < mhammond@skippinet.com.au
mailto:mhammond@skippinet.com.au> wrote:
> All,
>
> I wanted to pass this one around before opening an issue on it.
> When running the unit test for popen via rt.bat (in PCBuild8),
> I received the following error:
>
> === BEGIN ERROR ===
>
> C:\Documents and
> Settings\joe\Desktop\Development\Python\trunk\PCbuild8>rt test_popen
> Deleting .pyc/.pyo files ...
> 43 .pyc deleted, 0 .pyo deleted
>
> C:\Documents and
> Settings\joe\Desktop\Development\Python\trunk\PCbuild8>win32Re
> lease\python.exe -E -tt ../lib/test/regrtest.py test_popen
> test_popen
> test test_popen failed -- Traceback (most recent call last):
> File "C:\Documents and Settings\joe\Desktop\Development\Python\...
I can't reproduce this. I expect you will find it is due to the
space in
the filename of your Python directory, via cmd.exe's documented
behaviour
with quote characters. A patch that allows the test suite to work
in such
an environment would be welcome, but I think you might end up
needing access
to GetShortPathName() rather than CreateProcess().
Cheers,
Mark
Index: posixmodule.c
--- posixmodule.c (revision 55784) +++ posixmodule.c (working copy) @@ -4793,6 +4793,9 @@ PROCESS_INFORMATION piProcInfo; STARTUPINFO siStartInfo; DWORD dwProcessFlags = 0; / no NEW_CONSOLE by default for Ctrl+C handling /
- DWORD dwVersion = 0;
- DWORD dwMajorVersion = 0;
DWORD dwMinorVersion = 0; char s1,s2, s3 = " /c "; const char szConsoleSpawn = "w9xpopen.exe"; int i; @@ -4814,8 +4817,20 @@
--comshell; ++comshell;if (GetVersion() < 0x80000000 &&
- _stricmp(comshell, "command.com") != 0) {
- dwVersion = GetVersion();
- dwMajorVersion = (DWORD)(LOBYTE(LOWORD(dwVersion)));
- dwMinorVersion = (DWORD)(HIBYTE(LOWORD(dwVersion))); +
- if (dwMajorVersion >= 5)
- {
- / Current, XP + Vista? /
- x = i + strlen(s3) + strlen(cmdstring) + 1;
- s2 = (char *)alloca(x);
- ZeroMemory(s2, x);
- PyOS_snprintf(s2, x, "%s", cmdstring);
- }
- else if (dwMajorVersion == 4 &&
- _stricmp(comshell, "command.com") != 0) { / NT/2000 and not using command.com. / x = i + strlen(s3) + strlen(cmdstring) + 1; s2 = (char )alloca(x); @@ -4836,23 +4851,23 @@ modulepath[x] = '\0'; / Create the full-name to w9xpopen, so we can test it exists */ strncat(modulepath,
- szConsoleSpawn,
- (sizeof(modulepath)/sizeof(modulepath[0]))
- -strlen(modulepath));
- szConsoleSpawn,
- (sizeof(modulepath)/sizeof(modulepath[0]))
- -strlen(modulepath)); if (stat(modulepath, &statinfo) != 0) { size_t mplen = sizeof(modulepath)/sizeof(modulepath[0]); / Eeek - file-not-found - possibly an embedding situation - see if we can locate it in sys.prefix/ strncpy(modulepath,
- Py_GetExecPrefix(),
- mplen);
- Py_GetExecPrefix(),
- mplen); modulepath[mplen-1] = '\0'; if (modulepath[strlen(modulepath)-1] != '\') strcat(modulepath, "\"); strncat(modulepath,
- szConsoleSpawn,
- mplen-strlen(modulepath));
- szConsoleSpawn,
- mplen-strlen(modulepath)); / No where else to look - raise an easily identifiable error, rather than leaving Windows to report "file not found" - as the user is probably blissfully @@ -4861,10 +4876,10 @@/ if (stat(modulepath, &statinfo) != 0) { PyErr_Format(PyExc_RuntimeError,
- "Can not locate '%s' which is needed "
- "for popen to work with your shell "
- "or platform.",
- szConsoleSpawn);
- "Can not locate '%s' which is needed "
- "for popen to work with your shell "
- "or platform.",
- szConsoleSpawn); return FALSE; } } @@ -4970,20 +4985,26 @@ FALSE, DUPLICATE_SAME_ACCESS); if (!fSuccess)
- {
return win32_error("DuplicateHandle", NULL); }
/ Close the inheritable version of ChildStdin that we're using. / CloseHandle(hChildStdinWr);
if (!CreatePipe(&hChildStdoutRd, &hChildStdoutWr, &saAttr, 0))
- {
return win32_error("CreatePipe", NULL); }
fSuccess = DuplicateHandle(GetCurrentProcess(), hChildStdoutRd,
GetCurrentProcess(), &hChildStdoutRdDup, 0, FALSE, DUPLICATE_SAME_ACCESS);if (!fSuccess)
- {
return win32_error("DuplicateHandle", NULL); }
/ Close the inheritable version of ChildStdout that we're using. / @@ -4991,14 +5012,18 @@
if (n != POPEN_4) {
if (!CreatePipe(&hChildStderrRd, &hChildStderrWr, &saAttr, 0))- {
return win32_error("CreatePipe", NULL); - }
fSuccess = DuplicateHandle(GetCurrentProcess(),
if (!fSuccess)hChildStderrRd, GetCurrentProcess(), &hChildStderrRdDup, 0, FALSE, DUPLICATE_SAME_ACCESS); - {
return win32_error("DuplicateHandle", NULL); - } / Close the inheritable version of ChildStdErr that we're using. / CloseHandle(hChildStderrRd); }
C:\Documents and Settings\joe\Desktop\Development\Python\trunk\PCbuild8>win32Release\python.exe -E -tt ../lib/test/regrtest.py te st_popen test_popen 1 test OK. About to run again without deleting .pyc/.pyo first: Press any key to continue . . .
C:\trunk\PCbuild8>win32Release\python.exe -E -tt ../lib/test/regrtest.py test_popen test_popen 1 test OK. About to run again without deleting .pyc/.pyo first: Press any key to continue . . .
C:\trunk\PCbuild8>win32Release\python.exe -E -tt ../lib/test/regrtest.py test_popen test_popen 1 test OK.
C:\Documents and Settings\joe\Desktop\Development\Python\trunk\PCbuild8>win32Release\python.exe -E -tt ../lib/test/regrtest.py te st_popen test_popen test test_popen failed -- Traceback (most recent call last): File "C:\Documents and Settings\joe\Desktop\Development\Python\trunk\lib\test\test_popen.py", line 31, in test_popen ["foo", "bar"] File "C:\Documents and Settings\joe\Desktop\Development\Python\trunk\lib\test\test_popen.py", line 24, in _do_test_commandline got = eval(data)[1:] # strip off argv[0] File "<string>", line 0
^ SyntaxError: unexpected EOF while parsing
1 test failed: test_popen About to run again without deleting .pyc/.pyo first: Press any key to continue . . .
My apologies for being a day late, got working on some other things. So here's the scoop as it relates to the issue at hand:
If you run rt.bat from the trunk as-is and place it in a path that contains an empty space, you receive the error outlined in resultwithspace.txt.
If you run rt.bat from the trunk as-is and place it in a path that does not contain an empty space, you receive no errors as outlined in resultwithoutspace.txt.
If you run rt.bat with the patch, on Windows XP, you receive no errors as outlined in resultafterpatch.txt.
In that last step, you failed to indicate if the path had a space or not. ie, on Windows XP I get that behaviour now without needing to apply the patch.
The patch is attached.
The vast majority of the patch is insignificant - it is either adding braces where they are not necessary, or changing whitespace inappropriately (the spaces you replaced are so the lines all line up regardless of the tab width.) It seems there is only one significant block in your patch, and its not clear to me what the intent of the patch is - I admit I didn't apply it and look at it in-place, but a couple of comments indicating exactly what you are trying to do would be good, especially as I'm not aware of this behaviour change from Win2K -> WinXP.
Probably my biggest question now is the use of GetVersion as opposed to GetVersionEx.
The existing code explicitly checks if it is the 9x or NT family, which your patch no longer does. It seems to me that Windows ME will also qualify - although in general the strcmp for command.com will succeed, if an alternative shell is installed on a ME box it will do the wrong thing. If you need to check anything more than the high-bit of GetVersion(), IMO it should be replaced with GetVersionEx().
Cheers,
Mark
My apologies for being a day late, got working on some other things. So here's the scoop as it relates to the issue at hand:
Something else I meant to mention: your problem is that the test suite fails in some circumstances, but these circumstances are not met for most core developers or when running the python test suite from the directory it is built in, but your proposed fix is a patch to os.popen(). There would also need to be new test cases added to demonstrate this bug in a "normal" test run.
Cheers,
Mark
-
Joseph Armbruster -
Mark Hammond