Another attempt of support pty on windows with ConPTY api #155
Conversation
|
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 worked on Go 1.18 for me |
|
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
<#155 (comment)>, 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
<#155 (comment)>, 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
|
|
|
|
|
|
Worked for me on 1.18 and 1.17.1, thanks @photostorm 👏👏👏 |
|
@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: |
|
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/ |
|
ok, I will look into it. Thanks for your feedback. @kr |
|
@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) |
|
Great, if we're going the v2 route 🎉 |
|
@photostorm I can't get Setsize to work with this PR, when using it, it sets cols/rows to 1, regardless of the value I attempt to set. This results in InheritSize not working either and any children spawn up to have either size 0/0 or 1/1. Any idea? Note that before calling Setsize, Getsize returns the expected value. Sample code: package main
import (
"log"
pty "github.com/creack/pty/v2"
)
func assertSize(ptm pty.Pty, expected *pty.Winsize) {
childSize, err := pty.GetsizeFull(ptm)
if err != nil {
log.Fatalf("Error getting the child size: %s.", err)
}
if childSize.Cols != expected.Cols || childSize.Rows != expected.Rows {
log.Fatalf("Unexpected ptm size. %v != %v.", childSize, expected)
}
}
func main() {
a, _, err := pty.Open()
if err != nil {
panic(err)
}
if err := pty.Setsize(a, &pty.Winsize{Rows: 10, Cols: 10}); err != nil {
log.Fatalf("Error setsize: %s.", err)
}
assertSize(a, &pty.Winsize{Rows: 10, Cols: 10})
log.Printf("Success.")
}go get github.com/creack/pty/v2@photostorm-master-creack
GOOS=windows go build -o /tmp/foo.exeResult |
|
Actually, taking a step back, I can't even get write/read to work. I may be doing something wrong though, would you have any example showing a working behavior? When I try to write or read from the tty, I always get an "already closed" error. When reading on the pty, it blocks forever regardless of what I write. package main
import (
"log"
pty "github.com/creack/pty/v2"
)
func main() {
ptmA, _, err := pty.Open()
if err != nil {
panic(err)
}
if _, err := ptmA.Write([]byte("hello\n")); err != nil {
panic(err)
}
bufA := make([]byte, 5)
if _, err := ptmA.Read(bufA); err != nil {
panic(err)
}
if string(bufA) != "hello" {
log.Fatalf("Unexpected bufA: %q.", bufA)
}
} |
|
I will take a look when I get some time later this week. |
|
Minor-ish issue: closing the pty twice causes a heap violation resulting in the parent process to die (exit status 0xc0000374). The case happens often when dealing with network where multiple thing can cause the server to want to kill the process / close the pty. package main
import (
"os/exec"
"runtime"
"github.com/creack/pty/v2"
)
func main() {
binName := "sh"
if runtime.GOOS == "windows" {
binName = "cmd"
}
cmd := exec.Command(binName)
ptm, err := pty.Start(cmd)
if err != nil {
panic(err)
}
_ = ptm.Close()
_ = ptm.Close() // Causes exit 0xc0000374.
println("not reached on windows")
}Note that it also happens when closing only once but when the process is already gone. package main
import (
"os/exec"
"runtime"
"time"
"github.com/creack/pty/v2"
)
func main() {
binName := "sh"
if runtime.GOOS == "windows" {
binName = "cmd"
}
cmd := exec.Command(binName)
ptm, err := pty.Start(cmd)
if err != nil {
panic(err)
}
_, _ = ptm.WriteString("exit\r\n")
time.Sleep(200e6) // Small delay to let the process die.
_ = ptm.Close() // Causes exit 0xc0000374.
println("not reached on windows")
} |
|
Another issue is that I can't get a shell to properly work :(, it seems to be working at first, but as soon as I reach ~50% of the terminal, the cursor starts to jump around randomly. https://asciinema.org/a/uNCRfSYmuuwYqoqu9Q4a7SEOn A regular cmd or pwsh without the lib wrapping it works as expected. EDIT: Now that I post this, I realize it is likely related to the SetSize issue mention before, thinks it has the wrong size, it moves back the cursor to where it thinks is the last line. I need to run, I'll dig more into this later. |
|
confirmed, when manually setting the size, it works fine, with one caveat, still having an issue with Some programs (like vim) use that as upper bound, which results in the wrong size. Example open/close vim: PS C:\> $Host.UI.RawUI
ForegroundColor : Gray
BackgroundColor : Black
CursorPosition : 0,11
WindowPosition : 0,0
CursorSize : 25
BufferSize : 292,69
WindowSize : 292,69
MaxWindowSize : 292,69
MaxPhysicalWindowSize : 1008,45
KeyAvailable : True
WindowTitle : Administrator: C:\Program Files\PowerShell\7\pwsh.exe
PS C:\> vim
PS C:\> $Host.UI.RawUI
ForegroundColor : Gray
BackgroundColor : Black
CursorPosition : 0,27
WindowPosition : 0,0
CursorSize : 25
BufferSize : 292,45
WindowSize : 292,45
MaxWindowSize : 292,45
MaxPhysicalWindowSize : 1008,45
KeyAvailable : True
WindowTitle : Administrator: C:\Program Files\PowerShell\7\pwsh.exe |
|
As OpenSSH suffers from the same It would be nice if we could get a working example from windows to windows as well. For now, I only have been able to get something working when using OSX terminal/iTerm2. Using VSCode terminal, everything breaks, using windows console, everything breaks. |
|
I have a fix for the close issue. I am currently investigating sizing behavior. It is interesting that the setSize function seems to report a success, but when using the GetConsoleScreenBufferInfo method, it reports an invalid handle error. |
Thankyou for continuing to take time on this issue |
|
Any updates? Thank you for dedicating your time to this issue |
Right now, I'm still working on fixing the invalid handle issue with get size. It seemed I was using the wrong type of handle for the console screen buffer info. It should be the stdout handle. However, that also seems to be reporting an invalid handle. so I just been researching to try to figure out the correct way of handling the situation. Any help would be appreciation. |
pty_windows.go closed consoleW and consoleR in the initial open(), with the comment "These pipes can be closed here without any worry." This isn't actually true. If you interact with the terminal via Go code (i.e. don't immediately fork a new process, but use (for example) github.com/mvdan/sh), you need those pipes to stay open, and to close them yourself.
Don't close consoleR & consoleW in Windows
|
Thankyou for continuing to plug away at this |
|
I don't mean to disrupt the discussion about this PR, but perhaps you could take a look at aiopty/pty/conpty/conpty_windows.go and aiopty/term/term_windows.go. |
|
thank you all very much for implementation tty on windows! |
|
Sorry for the delay, I have been swamped recently, I'll take a look as soon as I can. |
|
Great news! 👍 |
|
Getting an error when running the tests (go1.22.3) |
|
When commenting out the SetDeadline section which cause the build to fail, I get test errors: Running the tests as Administrator yields the same result, including Access is denied. |
|
I still working on fixing invalid handle with get size, but just been busy with work. |
This is an another attempt to support pseudo-terminal on windows using ConPty API (should resolve issue #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.