creack
Another attempt of support pty on windows with ConPTY api
This is an another attempt to support pseudo-terminal on windows using ConPty API (should resolve issue https://github.com/creack/pty/issues/95)
This pull request does introduce major api change for Start method (*os.File -> Pty),
I have tested this code with remote terminal application that I wrote. It would be great to get some additional testing with other use cases.
This looks very promising, I will try it out to see if it works in our system
I just tried this out, and works perfectly for my use case (an SSH server that must work on UNIX and Windows) 👍
Trying this now. Would be really nice if this works on go 1.18 too :) considering repo is still pointing to 1.13 :D
It just passed our ci too. Worked like a charm too :)
On Thu, Jul 28, 2022, 7:49 PM Pete Woods @.***> wrote:
It worked on Go 1.18 for me
— Reply to this email directly, view it on GitHub https://github.com/creack/pty/pull/155#issuecomment-1198395129, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYTREGNBTBOT5HCXFTO3ATVWK23LANCNFSM54T43GUQ . You are receiving this because you commented.Message ID: @.***>
@creack if you have time to look at this, it would be most appreciated, I found the API break to be extremely unintrusive (in that I didn't need to change any code)
I’ll take a look over the weekend. Quite a few tickets in backlog, sorry about that.
On Thu, Jul 28, 2022 at 10:51 AM Pete Woods @.***> wrote:
@CrEaK https://github.com/CrEaK if you have time to look at this, it would be most appreciated, I found the API break to be extremely unintrusive (in that I didn't need to change any code)
— Reply to this email directly, view it on GitHub https://github.com/creack/pty/pull/155#issuecomment-1198396721, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD7LZMFPY36MECEUL42SDDVWK3CBANCNFSM54T43GUQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>
--
Guillaume J. Charmes
Please help to remove wild forks!
@creack Is there anything we can do to help with this PR getting reviewed? Windows support is a really big deal for a bunch of users of your library.
I'm happy this PR exists. However, it introduces a breaking change: *os.File -> Pty. The func Start and StartWithAttrs return *os.File currently, but the PR changes this to type Pty.
FWIW I agree there should be a high bar to justify incompatible changes to the interface. It's not immediately clear to me why that would be necessary to add Windows support.
There should be high bar to justify incompatible changes to the interface. Due to the return type of Start, I do not see a way to pass the handles. I think this API change would make it possible for other systems in future to be added.
Would it be possible to use a Unix socket on Windows instead of a pair of pipes? Each end of a socket is full-duplex, which might mean the exported interface can be a single os.File object.
https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
@kr I do not think I can used that. there is window handle that I need access to also.
Ultimately we will have to defer to @creack, it's his library.
My feeling is, it might be worth breaking the API, if that's absolutely necessary to support Windows. Even in that case, I think any new interface should undergo careful design review (which I could participate in).
But we haven't exhausted all the strategies to maintain compatibility. For instance, maybe the window handle could be stored on the side in a global lookup table, and the *os.File pointer or a file descriptor or something could be used to look up the window handle when it's needed.
var (
windowHandles = map[*os.File]windows.Handle{}
windowHandlesMu sync.Mutex
)
Choosing a suitable value to use as the map key could be tricky. The fd might not work if the client code calls dup or something. I don't have great knowledge of the Windows API and its semantics so I don't know offhand if there's a good value to use. Is there a stable way to identify a pty in Windows? Maybe something available via os.FileInfo.Sys?
Also, this would probably need to use a finalizer to delete map entries.
What do you think?
That seems like a plausible strategy to me, but without input from @creack I don't thing we can realistically progress
@creack have you gotten the chance to review this PR? My team could definitely use this. At the moment we are relying on @photostorm's fork (which has worked perfectly so far)
The code looks good! really hoping this gets merged. This will help my project so much!
Seems like the author has abandoned the project, maybe it's better to create another fork?
I am still around, but indeed real life has been on the way lately. I have quite a lot of pry pr/issues pinned, I’ll address them as soon as I can. This kind of thing require quite a lot of time.
On Wed, Jun 7, 2023 at 10:50 PM Charles Chiu @.***> wrote:
Seems like the author has abandoned the project, maybe it's better to create another fork?
— Reply to this email directly, view it on GitHub https://github.com/creack/pty/pull/155#issuecomment-1581808414, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD7LZOSHUEFIYWVM2JYF2LXKE4Y3ANCNFSM54T43GUQ . You are receiving this because you were mentioned.Message ID: @.***>
--
Guillaume J. Charmes
@photostorm ~since this PR has yet to be merged (eagerly awaiting the day!) and since your repo has issues turned off the best place I can think to report an issue is on this thread.~
Contents of comment moved: https://github.com/photostorm/pty/issues/2
