spawnProcess opens an unwanted console

[alecu]

When spawning a process under Windows to capture its standard output, if the controlling process is not a console application a new console is wrongly opened.

Here's a sample program that shows the bug:

# should_not_open_console.pyw

from twisted.internet import reactor, utils

def write_result(result):
    open("output.log", "w").write(repr(result))
    reactor.stop()

PING_EXE = r"c:\windows\system32\ping.exe"
d = utils.getProcessOutput(PING_EXE, ["slashdot.org"])
d.addCallbacks(write_result)
reactor.run()

Make sure to run it with the pythonw executable:

c:\python27\pythonw.exe should_not_open_console.pyw

When running the above command, the program starts in the background, and the wrong behavior shows up: a new console is opened, with the path to ping.exe as the title, and it remains blank while ping runs.

The expected behavior is: no new console is opened.

[alecu]

This issue has already been mentioned in the past:

•  http://twistedmatrix.com/pipermail/twisted-python/2006-August/013814.html
•  http://twistedmatrix.com/pipermail/twisted-python/2007-February/014731.html

[alecu]

Use the CREATE_NO_WINDOW flag to call CreateProcess

[alecu]

Following one of the solutions proposed in the threads above, I've submitted a fix that uses the CREATE_NO_WINDOW flag to call CreateProcess, as described in  Process Creation Flags.

[thijs]

Putting it up for review as described on ReviewProcess#Authors:Howtogetyourchangereviewed.

[amacleod]

I reviewed the patch and it mostly looks good. Things still to do:

1. Needs a news fragment.
2. In the test (test_process.py:2304) it might be worth explaining that index 5 is the positional argument for flags.
3. It would be nice to capture the steps to reproduce this behavior in Twisted source somewhere. I'm guessing that making tests that require user interaction (a la  https://launchpad.net/game) would be inappropriate to the Twisted suite, and I'm not even sure if there is a way to reliably force repro from trial. Anyway, if you can think of a way to explain and capture the issue in source, that would be cool. If not, I think that's ok too.

I verified that the test passes on Windows 7 64-bit running Python 2.7.3 32-bit with Twisted 12.1.0 source patched with your diff. I also verified that the test fails if I revert _dumbwin32proc.py

I also observed that, pre-patch, the extraneous window shows up if I run the .pyw in the ticket description and post-patch the program runs without any extra windows appearing.

[amacleod]

Oh, one more thing. I heard some murmuring in #twisted-dev that sibpath ought to be deprecated, and you should use FilePath(x).sibling("y") instead.

[alecu]

Fixes issues requested on the first review

[alecu]

I've attached a new patch with the fixes requested on the review:

• added a News fragment
• makes more explicit the parameter being checked in the tests
• improves the docstring for the test, and includes this issue number
• uses FilePath(x).sibling instead of sibpath

[amacleod]

Fixes look good. I will see about getting these changes committed.

[amacleod]

(In [35150]) Branching to 'windows-spawnprocess-nowindow-5726'

[amacleod]

As exarkun was coaching me through pre-commit steps he pointed out some issues with the patch:

1. The new test is in twisted/test/, to which we ought to avoid adding new things. It might be more appropriate to make a new TestCase dedicated to Win32 process stuff. JP suggested a new module in twisted/internet/test/.
2. There are naming convention mistakes in the test. The one I can spot from a brief scan is that Win32ProcessFlagsTest's sibling classes seem to end in "TestCase".
3. We ought not to be using @inlineCallbacks on test methods.

One question I think worth considering is how much merit there is in a unit test for behavior that's easy for humans to observe but hard for trial to observe. I think the test you wrote is worthy, since it at least makes sure that the right flags are being passed to and fro. I would suggest altering its docstring a little, though, to clearly state that it's just testing that flags get set properly and the function call doesn't throw an exception.

So, short of coming up with a magical Python library that can detect the presence and absence of windows, I think moving the current test code to its own module and fixing up any naming issues should be sufficient. ( http://twistedmatrix.com/documents/current/core/development/naming.html)