Another attempt of support pty on windows with ConPTY api

[photostorm]

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.

[pete-woods]

This looks very promising, I will try it out to see if it works in our system

[pete-woods]

I just tried this out, and works perfectly for my use case (an SSH server that must work on UNIX and Windows) 👍

[mjudeikis]

Trying this now. Would be really nice if this works on go 1.18 too :) considering repo is still pointing to 1.13 :D

[pete-woods]

It worked on Go 1.18 for me

[mjudeikis]

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: ***@***.***>

[pete-woods]

@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)

[creack]

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

[pete-woods]

[mjudeikis]

Please help to remove wild forks!

[gabemarshall]

Worked for me on 1.18 and 1.17.1, thanks @photostorm 👏👏👏

[pete-woods]

@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.

[mohammed90]

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.

[kr]

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.

[photostorm]

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.

[kr]

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/

[photostorm]

ok, I will look into it. Thanks for your feedback. @kr

[photostorm]

@kr I do not think I can used that. there is window handle that I need access to also.

[kr]

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?

[pete-woods]

That seems like a plausible strategy to me, but without input from @creack I don't thing we can realistically progress

[sabattle]

@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)

[adrivn]

Any updates? Thank you for dedicating your time to this issue

[photostorm]

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.

[pete-woods]

Thankyou for continuing to plug away at this

[iyzyi]

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.

[kcmvp]

thank you all very much for implementation tty on windows!
any update on this?

[creack]

Sorry for the delay, I have been swamped recently, I'll take a look as soon as I can.

[pidgeon777]

Great news! 👍

[creack]

Getting an error when running the tests (go1.22.3)

.\io_test.go:33:32: cannot use int(ptmx.Fd()) (value of type int) as syscall.Handle value in argument to syscall.SetNonblock
.\io_test.go:69:32: cannot use int(ptmx.Fd()) (value of type int) as syscall.Handle value in argument to syscall.SetNonblock 

[creack]

When commenting out the SetDeadline section which cause the build to fail, I get test errors:

--- FAIL: TestSetsize (0.01s)e.
    doc_test.go:102: Unexpected Getsize X result after Setsize: 1 != 0.
    doc_test.go:103: Unexpected Getsize Y result after Setsize: 1 != 0.
    doc_test.go:104: Unexpected Getsize Rows result after Setsize: 2 != 1.
    doc_test.go:105: Unexpected Getsize Cols result after Setsize: 2 != 1.
    helpers_test.go:24: Unexpected error from pty Close: Access is denied..
--- FAIL: TestOpen (0.01s)
    helpers_test.go:24: Unexpected error from pty Close: Access is denied..
--- FAIL: TestGetsizeFull (0.02s)
    helpers_test.go:24: Unexpected error from pty Close: Access is denied..
--- FAIL: TestOpenByName (0.02s)
    doc_test.go:38: Failed to open tty file: open |0: The filename, directory name, or volume label syntax is incorrect..
    helpers_test.go:24: Unexpected error from pty Close: Access is denied..
--- FAIL: TestName (0.02s)
    helpers_test.go:24: Unexpected error from pty Close: Access is denied..
--- FAIL: TestGetsize (0.02s)
    helpers_test.go:24: Unexpected error from pty Close: Access is denied..
exit status 0xc0000374
FAIL    github.com/creack/pty/v2        0.357s

Running the tests as Administrator yields the same result, including Access is denied.

[photostorm]

I still working on fixing invalid handle with get size, but just been busy with work.

[kareemmahlees]

Any update on this ?

[GitMurf]

Really waiting for this to merge so lazygit pagers will be usable for windows

jesseduffield/lazygit#1453

This is why I am here too 😢 Any updates? It seems without this, LazyGit cannot allow using a custom pager like delta for git diffing things. Would be super useful. Thanks so much for everyone's work on this!

[pidgeon777]

Hope is still high for this PR

[photostorm]

I am still here. Just been busy with my work. I hope to get back to soon. If any one can help me with looking into invalid handle problem that would be great.