Opened 12 years ago
Last modified 4 years ago
#2157 enhancement new
console support for stdio in Windows
Reported by: | synapsis | Owned by: | khorn |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | windows |
Cc: | manlio_perillo@…, Cory Dodt, teratorn, strank, khorn, Thijs Triemstra, therve, jesstess, Austin Hicks | Branch: |
branches/win-stdio-2157-4
branch-diff, diff-cov, branch-cov, buildbot |
Author: | synapsis, khorn, glyph, therve, thijs |
Description
This patch allows twisted.internet.stdio to work when connected to a console, in Windows.
I have done a small refactoring in twisted.internet._pollingfile. I think pipe support should go in twisted.internet._dumbwin32proc and the console support in a new twisted.internet._conio module.
I have also written a simple test suite.
Console support is still very limited, but now stdiodemo.py and stdin.py examples works.
In the near future I hope to have a working conch.
Attachments (17)
Change History (71)
Changed 12 years ago by
Attachment: | conio.patch added |
---|
comment:1 follow-up: 2 Changed 12 years ago by
Owner: | changed from Glyph to synapsis |
---|
Some comments on the attached code:
- classic to new-style class conversion: good (but unrelated)
- generalization of _PollableReadPipe and _PollableWritePipe - good
- _PollableReadConsole - looks like a good start at the low-levels of dealing with a windows console, but the factoring needs to be cleaned up. Dealing with the win32-specific events should not be handled by the same class which implements echoing and so forth. As a long term goal, providing a win32 implementation of the insults ITerminalTransport/ITerminalProtocol interfaces would seem to make sense (and it may make sense to make some adjustment to these interfaces based on what win32 is capable of). This further suggests splitting the transport logic (ie, WriteConsole calls) from the protocol logic (ie, should an input be echoed?), since insults is part of Twisted Conch, but the core of win32 conio support belongs in Twisted Core.
- test suite - looks good. Adding docstrings explaining the purpose of each test method would be even better.
I don't know much about the Win32 APIs involved here, so I can't comment much on the correctness of most of this code, but it looks like a nice start in the right direction.
comment:2 Changed 12 years ago by
Replying to exarkun:
Some comments on the attached code:
- classic to new-style class conversion: good (but unrelated)
Yes. I have done some modification on _pollingfile for private use, forgetting to revert them.
Shold I remove them?
- generalization of _PollableReadPipe and _PollableWritePipe - good
- _PollableReadConsole - looks like a good start at the low-levels of dealing with a windows console, but the factoring needs to be cleaned up. Dealing with the win32-specific events should not be handled by the same class which implements echoing and so forth.
One possible (but complex) solution is to provide file like objects for console stdin/stdout that are POSIX compatible (like done by Cygwin).
As a long term goal, providing a win32 implementation of the insults ITerminalTransport/ITerminalProtocol interfaces would seem to make sense (and it may make sense to make some adjustment to these interfaces based on what win32 is capable of).
IPython contains a pyreadline package that supports ANSI terminal escape characters.
This further suggests splitting the transport logic (ie, WriteConsole calls) from the protocol logic (ie, should an input be echoed?), since insults is part of Twisted Conch, but the core of win32 conio support belongs in Twisted Core.
- test suite - looks good. Adding docstrings explaining the purpose of each test method would be even better.
I don't know much about the Win32 APIs involved here, so I can't comment much on the correctness of most of this code, but it looks like a nice start in the right direction.
Some comments of what needs to be done:
1) pipe support should (IMHO) be moved from _pollingfile to _dumbwin32proc, console support should be moved to a new _conio module
2) Since a Window process can have only one console and a console can have only on input buffer, the _PollableReadConsole should be a singleton/borg.
The _conio module should provide globals stdin/stdout/stderr objects
3) Since in a child process can happen(?) that stdin is attached to a console and stdout to a pipe (or viceversa) _win32stdio should use isatty on both file descriptors
4) In _PollableReadConsole I convert '\r' to '\r\n'.
I have done so to be able to send empty lines to a LineReceiver protocol. Maybe '\r' should be simply converted to '\n'?
5) I really don't know if _PollableWriteConsole can block.
I have just copied the implementation of _PollableWritePipe, using WriteConsole instead of WriteFile
Can I implement the changes from 1)?
comment:3 follow-up: 5 Changed 12 years ago by
Please make the IO mechanism pluggable. For example, a reactor that can wait on win32 handles (such as win32eventreactor or, potentially, iocpreactor) could turn off mouse and focus events and use WaitForMultipleObjects to find out when ReadConsole wouldn't block.
At the very least, StandardIO should not be inheriting from _PollingTimer.
Also, the standard API for closing a file handle is its loseConnection() method. I don't know why _pollingfile uses close().
comment:4 Changed 12 years ago by
Cc: | Cory Dodt added |
---|
I only have a vague idea about the design details you guys are throwing around here, but I applaud this effort and I hold out hope that the win32 platform will be greatly improved by this patch. So I'm only going to make a simple plea. There seem to be an awful lot of modules and classes that start with underscores. Please review the API at least once and think hard about whether so many things need to be private/untouchable, and which things can be available to applications.
Thanks.
comment:5 Changed 12 years ago by
Replying to PenguinOfDoom:
Please make the IO mechanism pluggable. For example, a reactor that can wait on win32 handles (such as win32eventreactor or, potentially, iocpreactor) could turn off mouse and focus events and use WaitForMultipleObjects to find out when ReadConsole wouldn't block.
This is a good idea. I'm planning to write a file like object that allows a non blocking read from a console.
At the very least, StandardIO should not be inheriting from _PollingTimer.
I'm not sure about this. When running the win32eventreactor we can receive notifications about input available. But what about _PollableWriteConsole?
Moreover I really don't know if WriteConsole can block.
Also, the standard API for closing a file handle is its loseConnection() method. I don't know why _pollingfile uses close().
comment:6 Changed 12 years ago by
This is a new set of patches for a better implementation of console support in twisted.
I have created a separate patch for _pollingfile so that this can be moved to a new ticket.
Notes:
1) I have changed _pollingfile to allow it to be reusable. More things can be done, like moving pipe support to _dumbwin32process and refactoring _PollingTimer to a more reactor like interface
2) implementation of conio needs to be reviewed. I have to do some tests with a real POSIX implementation (on Linux).
3) support for IConsumer and IProducer (in _win32stdio) needs to be reviewed
Changed 12 years ago by
Attachment: | test_conio.2.py added |
---|
test suite for console support, version 2
comment:7 Changed 11 years ago by
Cc: | teratorn added |
---|
comment:8 Changed 11 years ago by
Cc: | strank added |
---|
comment:9 Changed 10 years ago by
Starting with this year I no more use Windows, so I have no more much interest in this patch.
However if this patch is still useful and there is someone that can do a review, I can try to complete it.
comment:11 Changed 9 years ago by
Duplicate of #2135. I kept this one open because it has a lot more stuff on it.
comment:12 Changed 8 years ago by
Cc: | khorn added |
---|
Changed 8 years ago by
Attachment: | win32_console_io.2.patch added |
---|
Unified diff of above patches against r27422
comment:13 Changed 8 years ago by
Consolidated patches into a single unified diff against the current trunk (r27422).
Hopefully this will make it easier to move this forward.
With this patch, tests pass, and the stdiodemo.py example works.
The stdin.py demo does NOT work (no output), at least on my machine (WinXP).
comment:14 Changed 8 years ago by
Found the problem with the stdin.py demo. It uses os.linesep as a delimiter (which on Win32 is '\r\n'), while the stdiodemo.py example uses a hardcoded '\n'.
Changing to a hardcoded '\n' in stdin.py makes it work with this patch. I'm sure there's something deeper going on here which probably needs further investigation.
comment:15 Changed 8 years ago by
Author: | → thijs |
---|---|
Branch: | → branches/win-stdio-2157 |
(In [27426]) Branching to 'win-stdio-2157'
comment:18 Changed 8 years ago by
Author: | thijs → synapsis, khorn, thijs |
---|---|
Cc: | Thijs Triemstra added |
Keywords: | review added |
Owner: | synapsis deleted |
I slapped the patch in a branch and applied some coding standards, up for review.
comment:19 Changed 8 years ago by
Author: | synapsis, khorn, thijs → synapsis, khorn, therve, thijs |
---|---|
Branch: | branches/win-stdio-2157 → branches/win-stdio-2157-2 |
(In [27500]) Branching to 'win-stdio-2157-2'
comment:20 follow-up: 27 Changed 8 years ago by
Keywords: | added; review removed |
---|---|
Owner: | set to khorn |
I don't have a Windows machine here to properly test this code, but here's some comments:
- There's a lot of "XXX" and "TODO" comments in this code. The coding standard says "Comments marked with XXX or TODO must contain a reference to the associated ticket". Hopefully the questions and unhandled conditions marked by these strings will be answered and handled before this branch is merged to trunk, and no tickets will need to be filed. :)
t/i/_pollingfile.py
, line 239 in_PollableReadPipe.checkWork()
: Commented-out code should be fixed if it ought to work, or deleted otherwise.t/i/_win32stdio.py
, line 34 in_PollableReadConsole.checkWork()
: Rather than explain the magic value 2000 as "80x25", why not just write it as "80 * 25"? Also, does this break if the console is a different size (such as 80×50)?t/i/_win32stdio.py
, line 36 in_PollableReadConsole.checkWork()
: If you do get an unexpectedIOError
, it would be more helpful to raise it than to raise an unrelatedAssertionError
.t/i/_win32stdio.py
, in_PollableWriteConsole.checkWork()
: VariablesnumBytesWritten
andnBytesWritten
in the same method? There's got to be better names than these.t/i/_win32stdio.py
, line 90: Is there a reason thatConsoleReader
doesn't have a leading underscore, where_PollableReadConsole
(presumably exactly the same thing but for other reactors) does not?t/i/_win32stdio.py
, line 90: You haveConsoleReader
forwin32eventreactor
and_PollableReadConsole
otherwise; but_PollableWriteConsole
works with all reactors. Why not dropConsoleReader
and just use_PollableReadConsole
all the time, too?t/i/_win32stdio.py
, line 92 inConsoleReader.__init__()
: This is a terrible docstring. It doesn't describe what the class represents or how it's intended to be used.t/i/_win32stdio.py
, line 106 inConsoleReader.doRead()
: This looks exactly like_PollableReadConsole.checkWork()
. MaybeConsoleReader
and_PollableReadConsole
should inherit from some common base class or mixin? Otherwise, all the comments for_PollableReadConsole.checkWork()
apply here too.t/i/_win32stdio.py
, line 169 inStandardIO.__init__()
: You say "for stdfd in range(0, 1, 2)", but I don't think that does what you intend. How about "for stdfd in (0, 1, 2)"?t/i/_win32stdio.py
, line 176 inStandardIO.__init__()
:win32Enabled
is an unfortunate name - it's quite possible to be using Win32 with the select reactor or IOCP reactor. How about calling iteventReactorInstalled
?t/i/_win32stdio.py
, line 210 inStandardIO.__init__()
: I feel it would be more readable to paste "self._addPollableResource(self.stdin)
" into both places where it's required than to do the little if dance afterward.t/i/conio.py
: Since this is just console IO for Windows rather than console IO in general, perhaps this module should be calledwin32conio.py
?t/t/test_conio.py
: Why is this intwisted/test/
and nottwisted/internet/test/
, since it's testing things intwisted/internet/
?t/t/test_conio.py
: This test suite explodes horrible on non-Win32 platforms (or presumably Win32 platforms without the Win32 Extensions package installed) because it importswin32console
. Seet/i/t/test_pollingfile.py
for an example of how to write platform-dependent tests that don't explode on other platforms.
I'm afraid I ran out of time before I could properly review conio.py
and test_conio.py
, but the issues above should be enough to start with.
It looks like khorn has the most involvement with this code at the moment, so assigning back to them to respond to these issues.
comment:21 Changed 8 years ago by
Fair warning: I didn't write any of this code to begin with. I just unified the existing patches and made sure they worked with trunk.
That said, I'm happy to give this one a shot, as long as everyone else is happy to wait long enough for me to more fully understand the code. :)
I'm a little perplexed at how I should proceed though. There are now two branches, and the newer one doesn't seem to have all (any?) of the changes in the patches for this ticket. Should I be submitting patches against one of the branches? Which branch?
comment:23 Changed 8 years ago by
Cc: | therve added |
---|
Unless I'm missing something, the latest branch should have all the changes submitted. Do you see anything missing?
comment:24 Changed 8 years ago by
OK, I think I know what happened now. When I looked at the branches before, I was browsing the code in Trac, and several files were missing from the latest branch. They all seem to be there now. Maybe some sort of caching issue with Trac?
At any rate, seems fine now, my fault for not thinking of caching issues earlier.
Sorry. :)
comment:25 Changed 8 years ago by
comment:26 Changed 8 years ago by
(In [28085]) Address review comments:
- Removed extraneous commented line from _pollingfile.py
- fixed incorrect loop in _win32stdio.py
- renamed win32Enabled to win32EventReactorInstalled
- renamed conio.py to win32conio.py
- fixed test_conio.py so that the tests don't blow up on non-win32 platforms
Refs: #2157
comment:27 Changed 8 years ago by
Keywords: | review added; removed |
---|---|
Owner: | khorn deleted |
Replying to TimAllen:
changes in [28085] address some of the review comments
- There's a lot of "XXX" and "TODO" comments in this code. The coding standard says "Comments marked with XXX or TODO must contain a reference to the associated ticket". Hopefully the questions and unhandled conditions marked by these strings will be answered and handled before this branch is merged to trunk, and no tickets will need to be filed. :)
Let's see if we can fix up the other stuff first...
t/i/_pollingfile.py
, line 239 in_PollableReadPipe.checkWork()
: Commented-out code should be fixed if it ought to work, or deleted otherwise.
Removed the offending line.
t/i/_win32stdio.py
, line 34 in_PollableReadConsole.checkWork()
: Rather than explain the magic value 2000 as "80x25", why not just write it as "80 * 25"? Also, does this break if the console is a different size (such as 80×50)?
Well, it adds one more operation every time the loop runs, so maybe this was a premature optimization?
I currently have no idea if this breaks when/if the console size is different, but I don't see why it would. For that matter I'm not sure why this value was chosen at all...
t/i/_win32stdio.py
, line 36 in_PollableReadConsole.checkWork()
: If you do get an unexpectedIOError
, it would be more helpful to raise it than to raise an unrelatedAssertionError
.
Are we sure that the AssertionError would be raised in all cases? I'm not sure why it does what it's doing, but I also don't really know what the correct behavior should be, so...
t/i/_win32stdio.py
, in_PollableWriteConsole.checkWork()
: VariablesnumBytesWritten
andnBytesWritten
in the same method? There's got to be better names than these.
Probably so, but it follows the example of t.i._pollingfile.py (before this branch), which does the same thing in several places.
t/i/_win32stdio.py
, line 90: Is there a reason thatConsoleReader
doesn't have a leading underscore, where_PollableReadConsole
(presumably exactly the same thing but for other reactors) does not?
t/i/_win32stdio.py
, line 90: You haveConsoleReader
forwin32eventreactor
and_PollableReadConsole
otherwise; but_PollableWriteConsole
works with all reactors. Why not dropConsoleReader
and just use_PollableReadConsole
all the time, too?
t/i/_win32stdio.py
, line 92 inConsoleReader.__init__()
: This is a terrible docstring. It doesn't describe what the class represents or how it's intended to be used.
t/i/_win32stdio.py
, line 106 inConsoleReader.doRead()
: This looks exactly like_PollableReadConsole.checkWork()
. MaybeConsoleReader
and_PollableReadConsole
should inherit from some common base class or mixin? Otherwise, all the comments for_PollableReadConsole.checkWork()
apply here too.
re: 6-9 -- I really am not sure what ConsoleReader was intended to do...if anyone has any ideas, I'd like to hear them.
t/i/_win32stdio.py
, line 169 inStandardIO.__init__()
: You say "for stdfd in range(0, 1, 2)", but I don't think that does what you intend. How about "for stdfd in (0, 1, 2)"?
Yikes! Fixed as suggested.
Not sure why it worked before though...
t/i/_win32stdio.py
, line 176 inStandardIO.__init__()
:win32Enabled
is an unfortunate name - it's quite possible to be using Win32 with the select reactor or IOCP reactor. How about calling iteventReactorInstalled
?
Renamed to win32EventReactorInstalled
.
t/i/_win32stdio.py
, line 210 inStandardIO.__init__()
: I feel it would be more readable to paste "self._addPollableResource(self.stdin)
" into both places where it's required than to do the little if dance afterward.
I don't understand what is meant here. Can you clarify?
t/i/conio.py
: Since this is just console IO for Windows rather than console IO in general, perhaps this module should be calledwin32conio.py
?
Agreed. Fixed.
However, it looks as though this may have been intended to grow into a platform-independent module. See: http://twistedmatrix.com/trac/ticket/2157#comment:6
???
t/t/test_conio.py
: Why is this intwisted/test/
and nottwisted/internet/test/
, since it's testing things intwisted/internet/
?
I'm guessing the original author put it there because test_stdio is also in twisted/test
. Should it be moved? Also, should it be renamed to something like test_win32conio.py
?
t/t/test_conio.py
: This test suite explodes horrible on non-Win32 platforms (or presumably Win32 platforms without the Win32 Extensions package installed) because it importswin32console
. Seet/i/t/test_pollingfile.py
for an example of how to write platform-dependent tests that don't explode on other platforms.
I think this is fixed now, and the tests should be skipped on non-Win32. Please test on non-Win32 platforms to make sure.
I've also noticed something a bit strange when running the tests. Some of the tests in test_conio.py send output to the console which is running trial. So it ends up sending out:
ConInRawTestCase testFlush ... [OK] testRead ... [OK] testReadMultiple ... [OK] testReadWithBuffer ... [OK] testReadWithDelete ... [OK] ConInTestCase testBuffer ... hello [OK] testDeleteBoundary ...w [OK] testDeleteFullBoundary ... hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh w [OK] testFlush ... hello
Somehow I don't think is intentional. :)
I'm not sure if this is because of something trial is doing (or mis-assuming), or if it has to do with the code that writes to the console input buffer.
It seems to happen when WriteConsoleInput() is called.
The tests pass regardless...so is this really a problem with functionality? I don't know.
comment:28 Changed 8 years ago by
Cc: | jesstess added |
---|---|
Keywords: | review removed |
Owner: | set to khorn |
Thanks for all this work, khorn!
Unfortunately there are both testing and markup issues.
If you're interested in running the buildbot tests yourself, a starting point is UsingBuildbot.
comment:29 Changed 8 years ago by
comment:31 Changed 8 years ago by
the only test that's failing on all the windows builders (aside from the glib2reactor and gtkreactor failures, which are unrelated)
[ERROR]: twisted.test.test_conio Traceback (most recent call last): File "C:\twistedbot\winxp32-py2.6-select\Twisted\twisted\trial\runner.py", line 563, in loadPackage module = modinfo.load() File "C:\twistedbot\winxp32-py2.6-select\Twisted\twisted\python\modules.py", line 381, in load return self.pathEntry.pythonPath.moduleLoader(self.name) File "C:\twistedbot\winxp32-py2.6-select\Twisted\twisted\python\reflect.py", line 462, in namedAny topLevelPackage = _importAndCheckStack(trialname) File "C:\twistedbot\winxp32-py2.6-select\Twisted\twisted\python\reflect.py", line 398, in _importAndCheckStack return __import__(importName) File "C:\twistedbot\winxp32-py2.6-select\Twisted\twisted\test\test_conio.py", line 17, in <module> from twisted.internet import win32conio, _win32stdio as stdio File "C:\twistedbot\winxp32-py2.6-select\Twisted\twisted\internet\win32conio.py", line 337, in <module> stdin = ConIn(win32console.GetStdHandle(win32console.STD_INPUT_HANDLE)) File "C:\twistedbot\winxp32-py2.6-select\Twisted\twisted\internet\win32conio.py", line 57, in __init__ defaultMode = handle.GetConsoleMode() pywintypes.error: (6, 'GetConsoleMode', 'The handle is invalid.')
I can't replicate this locally. It looks like it's unable to get a proper handle to the console's std_in buffer. Some possible causes (guessing):
- Running under the buildbot makes it so that there's no handle to get. How does buildbot run it's jobs? In a separate process maybe? Or does something else already have a hold on the stdin buffer?
- Some kind of mismatch between the version of pywin32 on the buildslaves and what's on my local machine?
- Any other ideas?
comment:32 Changed 8 years ago by
Branch: | branches/win-stdio-2157-2 → branches/win-stdio-2157-3 |
---|
(In [29543]) Branching to 'win-stdio-2157-3'
comment:33 Changed 8 years ago by
Merged forward and the tests still pass on my machine...still need to fix buildbot errors.
comment:35 Changed 8 years ago by
After some research, it appears that buildslaves usually (always?) run as Windows Services on Win32. This _might_ mean that the build user would need the "Allow service to interact with desktop" right in order to create a console. Which _might_ have something to do with the above test failure.
Still can't replicate the failure locally.
comment:36 Changed 8 years ago by
Success!
Finally replicated the error using:
>>> from subprocess import Popen >>> outfile = open('outfile.txt', 'w') >>> Popen([r'c:\python26\scripts\trial.bat', 'twisted.test.test_conio'], stdin=outfile)
I get the exact error above. So now I guess it's time to figure out how to build a console or something in the test code...
comment:37 Changed 7 years ago by
Owner: | khorn deleted |
---|
comment:38 Changed 5 years ago by
From MSDN:
GUI applications are initialized without a console. Console applications are initialized with a console, unless they are created as detached processes (by calling the CreateProcess function with the DETACHED_PROCESS flag).
I.e. your process probably doesn't have a console if you run it in the background or start it as a system service. Then all console-related Win32 APIs will fail.
You can call AllocConsole to get yourself a console. It might appear on the user's screen, at least temporarily, but that might be OK for a buildbot. I'm not sure how you could get any input into it in this state, so it might not be useful for testing, but at least you'd get past GetConsoleMode() failing.
comment:39 Changed 5 years ago by
I've been working on improved windows support for StandardIO() and found this branch to be a useful starting point. I've managed to get the (slightly modified) cftp.py and conch.py scripts working and solved the build-bot missing console problem. Improvements include being able to see the console cursor and scroll-back that doesn't keep 'pulling' you back to the active console line. I also tried the imap4client.py example program which worked with no changes.
I had to setup a buildbot-server for testing and have a windows 7 buildbot client (others to follow). I've tested my patches on XP/7/Linux and they seem to work OK. They are against the current trunk 38158.
Unfortunately, the patch is huge (90k) plus 3 new files (8k each) so I'm not sure exactly what to do with it.
For now I'm going to break it up into windows stdio support, tests for that support, conch/cftp changes, tests for that, and finally other unrelated issues I encountered whilst trying to get as many windows tests to pass as possible (6999).
Should I add them here when I'm ready?
comment:40 Changed 5 years ago by
Hi johnnypops. I think it would be best if you raised this on the mailing list. It would probably also be useful if you shared the code at the same time. It's possible someone will be able to take a look at it and give you some ideas about the way to proceed which has the greatest chance of getting your code into Twisted quickly and easily. Thanks!
Changed 5 years ago by
Attachment: | t-internet-t-test.patch added |
---|
_pollingfile, _dumbwin32proc, _win32stdio, test_stdio, test_pollingfile
Changed 5 years ago by
Attachment: | t-conch-t-python.patch added |
---|
patches for cftp, conch scripts plus conch client, ssh, stdio, tap, python.win32
Changed 5 years ago by
Attachment: | t-conch-test.patch added |
---|
lots of minor tweaks to the conch tests
comment:41 Changed 5 years ago by
I foolishly forgot to test with the stdiodemo.py and stdin.py examples mentioned in the original post. Attached fixed versions of win32conio.py and _win32stdio.py.
stdin.py worked as-is but stdiodemo.py needs the delimiter in WebCheckerCommandProtocol() fixing:
class WebCheckerCommandProtocol(basic.LineReceiver): from os import linesep as delimiter def connectionMade(self):
It also revealed that connectionLost() wasn't being called correctly when using the console.
Changed 5 years ago by
Attachment: | t-internet-t-test.2.patch added |
---|
Ignore the attached _win32stdio, this fixed patch includes those changes
comment:42 Changed 4 years ago by
This ticket isn't up for review, but it looks like perhaps it should be. Did you intend to put it up for review?
comment:43 Changed 4 years ago by
Cc: | Austin Hicks added |
---|
comment:44 Changed 4 years ago by
Keywords: | review added |
---|
I suspect that johnnypops may not be paying attention any more, so I'm going to attach the review keyword. (If the author doesn't reply, please consider me the author on this now...)
comment:45 Changed 4 years ago by
Author: | synapsis, khorn, therve, thijs → synapsis, khorn, glyph, therve, thijs |
---|---|
Branch: | branches/win-stdio-2157-3 → branches/win-stdio-2157-4 |
(In [42962]) Branching to win-stdio-2157-4.
comment:46 Changed 4 years ago by
Keywords: | review removed |
---|---|
Owner: | set to John Popplewell |
johnnypops, thanks very much for your contribution. I'm sorry it got lost in the shuffle.
I had a look at the attached patch and attempted to apply it to a branch. It doesn't apply cleanly to trunk any more, and the patch that's there is missing a lot of stuff. I couldn't really tell which files I was supposed to apply and which I wasn't. It also randomly deleted a bunch of docstrings, changed whitespace to violate the coding standard in various places, and added a bunch of methods whose purpose wasn't clear.
All the new classes, Channel
, ChannelReadPipe
, ChannelWritePipe
, ChannelConsole
, _PollableReadConsole
, and so on, need docstrings.
Also I don't quite understand tradeoffs involved in using SetHandleInformation
vs. DuplicateHandle
/CloseHandle
, if that affects the way Twisted will behave at all.
I really hope someone will pick this back up, I'll try to review it more promptly in the future :).
comment:47 Changed 4 years ago by
Owner: | changed from John Popplewell to khorn |
---|
I'll try to give this one another shot.
comment:49 follow-up: 50 Changed 4 years ago by
I've managed to get johnnypops' patches merged, but am having some trouble pushing them back up to SVN. I'll try to get it pushed up tomorrow.
comment:50 Changed 4 years ago by
Replying to khorn:
I've managed to get johnnypops' patches merged, but am having some trouble pushing them back up to SVN. I'll try to get it pushed up tomorrow.
What kinds of issues are you having? When git takes a long time to do things with git-svn, sometimes it's a clue that it's preparing to do some enormously destructive operation that will mangle the whole repository, so I'm very keen to see the output of git svn dcommit -n
or somesuch now :).
comment:51 Changed 4 years ago by
First I had an issue with git-svn finding my ssh binary (plink.exe in this case). This was a matter of setting env vars correctly so it would find it, which sounds simpler than it actually was. :/
then I got something like this:
>git svn dcommit --dry-run Committing to svn+ssh://svn.twistedmatrix.com/svn/Twisted/branches/win-stdio-2157-4 ... diff-tree 2f0571134babb9902c7dc0976fde3913be50c705~1 2f0571134babb9902c7dc0976fde3913be50c705
which I think means it should work fine (the hash is indeed the commit I want to push). When I ran it without the --dry-run flag, though, it just sat there forever, giving me no feedback. I figured it was up to no good, so I killed it after several minutes.
I'll investigate further presently...if you have any suggestions though I'd be glad to hear them.
comment:52 Changed 4 years ago by
comment:53 Changed 4 years ago by
Updated the latest branch with modified patches. Tests seem to pass in my environment, and the stdin.py and stdiodemo.py examples work.
comment:54 Changed 4 years ago by
Thanks again for looking at this, khorn.
Yes, that git-svn output looks correct.
patch for console support