Do not start a new console when spawning a new process on Windows #697
Conversation
Codecov Report
@@ Coverage Diff @@
## trunk #697 +/- ##
==========================================
- Coverage 91.27% 91.26% -0.02%
==========================================
Files 844 843 -1
Lines 147472 147298 -174
Branches 13047 13021 -26
==========================================
- Hits 134612 134431 -181
- Misses 10623 10629 +6
- Partials 2237 2238 +1Continue to review full report at Codecov.
|
opalmer
left a comment
There was a problem hiding this comment.
Looks good.
I initially thought DETACHED_PROCESS might be better suited here but that completely detaches the process from the original console which is probably not the behavior most people would expect. Eventually exposing the process creation flags is probably a better approach as that would allow someone more explicit control over the behavior around process creation. Of course that's a bigger change and separate from this PR...
glyph
left a comment
There was a problem hiding this comment.
This looks great, modulo the minor tweaks I suggested. Thanks for noticing and fixing!
src/twisted/test/test_process.py
Outdated
| def test_flags(self): | ||
| """ | ||
| Verify that the flags passed to win32process.CreateProcess() prevent a | ||
| new console window from being created. See http://tm.tl/5726 |
There was a problem hiding this comment.
I think that including the script inline (minimized as much as possible) would be better than linking to the closed ticket.
| threadAttributes, bInheritHandles, creationFlags, | ||
| newEnvironment, currentDirectory, startupinfo): | ||
| """ | ||
| See the Windows API documentation for I{CreateProcess} for further details. |
There was a problem hiding this comment.
twistedchecker shouldn't be checking a callback like this, and this documentation is all duplicative with the existing docs, so this docstring should probably be elided.
|
@alecu thanks for the submission. Sorry it took 5 years to get this fix in. |
http://twistedmatrix.com/trac/ticket/5726
Moving patch submitted by @alecu to GitHub