-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Windows Support #30
Windows Support #30
Conversation
…are a proper pull request against pty.js in the nearest future.
…as been committed upstream.
… codes if we fail to start a terminal process.
…t debug environment variable if debug is enabled.
…t debug environment variable if debug is enabled.
/cc @chjj @TooTallNate |
I'm really impressed you got this working on windows, @peters. @TooTallNate, any thoughts? I definitely won't be able to maintain the windows code. If anything changes with the pty.js terminal api, I probably won't be able to change it or test it for windows. @peters, why is |
One could say that having windows support will put severe limitations on what can be achieved on the unix end. Though my initial solution to this is just to throw an exception on methods not supported on windows, such as Terminal.open() and add specific notes in README.md. As a side note, i'm going to use pty.js in a new project in the nearest future, so if any new functionality is required on the windows side, and its possible to implement i can take of that. Breakdown on what happens when a new terminal is forked
It usually takes around 500 ms because winpty-agent.exe also connects to an internal named pipe: https://github.com/peters/pty.js/blob/master/deps/winpty/libwinpty/winpty.cc#L284 And that's why term.ready() is needed because we need to wait for 4) before its safeto assume a terminal was successfully forked. So if you could create a socket and block the node event loop some way until winpty-agent is connected to this.dataPipe then we could eliminate term.ready(). I've tried various solutions but they have all been unsuccessfull, but maybe somebody else knows on how this could be achieved. |
Yeah, that's good. I definitely prefer that.
Sounds alright.
What's wrong with buffering writes to the terminal? |
Buffering data/writes is possible, but what about Terminal.resize/Terminal.kill.. these could be defered until the terminal is ready, but what about the other methods? But that might also have side effects as well, thats why i ended up with term.ready() If you have any genius ideas on how this could be achieved smoothly i'm all ears :) |
I'd really prefer buffering writes and deferring any other method. Obviously the downside is .resize/.kill/etc would not necessarily be sync and might cause some problems, but we could actually do both. Keep the 'ready' event, but also buffer if anything is called before the terminal is ready. I do not want to have to change the unix api for any reason. |
@TooTallNate any luck getting it running now? ;) checkout test.js |
@chjj tested with latest tty.js, works like a charm :) |
Ill try to test it out today. Good work! On Tuesday, February 26, 2013, Peter Rekdal wrote:
|
so... how did it go? |
@peters Hey there! So I just tried your code again on Windows XP. It seems like it's getting farther than it was previously, but I still never see anything get printed to the node application. If I launch the DebugServer first, I see output for the text that I input, but no debug information for anything coming from the cmd.exe:
|
Could you try by specific the absolute path to cmd.exe? I'm having discussion with richard regarding some issues related to Windows XP. Try the following code:
|
@peters Seemingly the same result... I'll try Windows 7 in a bit to see if I get varying results. |
Strange, let me know if you get it working on 7.. I'm testing on Windows XP SP3 and Windows 7 SP 1.. if it does not work, i'll do a fresh install of both OS and see if i can try to reproduce the problem.. because, this is very odd.. |
@peters You know, I get basically the same result on Windows 7 as well... I'm really not sure what I'm doing wrong. Could you maybe make a screencast or something showing all the steps to get it running properly? |
Sure i could do that, but could you try one last thing first?
Tested with node v0.8.21 [tty.js] Listening on port 8080.
Please note that you should install msysgit environment if you want to the first test to work since tty.js I've followed these steps myself, like 10 minutes ago on a fresh Windows 7 installation and it worked great :) |
My test environment is VS 2012 express edition with Windows 7 SP 1 with latest patches installed. |
@peters Doing your steps above worked :) This is very exiting!!! So that said... what's wrong with our standalone test case? I'll have to dig into it I guess. Does running the code you posted in #30 (comment) work for you? It should basically just pass-through everything in the cmd.exe as if there's nothing there. |
Awesome! I do not know, but looking at the assertion errors popping up, the windows version of .fork() outputs console data.. Also there seems to be some issues starting node.exe.. see rprichard/winpty#9 |
@peters I went ahead and merged your code. Partially working windows support is better than no windows support. We can iron out the kinks individually now. Thanks again, this is awesome stuff! |
Awesome! np :) |
Found a problem when environement isn't passed correctly, this possible also cause problem mentioned above(you should specify full path to cmd.exe) since env.path isn't passed correctly. COMSPEC=D:\windows\system32\cmd.exe as you can see PATH, TERM and GENESIS_HOME variables joined into one |
Thank you for reporting the issue, i'll try to find some time this week and commit a patch :) |
Changeset
Added libwinpty in deps/winpty. I've made some minor changes and committed them upstream, but these are not related to the library itself.
Remarks
Only pty.fork() is supported.
Unit tests fails, could somebody care to investigate why?
Demo
And you shall have unicorns!