Support pty on windows with ConPty api #109

ghost · 2020-11-26T17:01:08Z

This is an attempt to support pseudo-terminal on windows using ConPty API (should resolve issue #95)

NOTICE:

This pull requests introduces major api change for Open (*os.File -> Pty), but I think it's easy to migrate from the original api since you can convert Pty to os.File on unix platforms easily with simple type cast, but for windows platform, you probably need to handle it differently since the ConPty api requires user provided data streams for io.

Sample wrapper for windows tty: https://github.com/arhat-dev/go-pkg/blob/master/exechelper/tty_windows.go

Known issues:

exit code 0xC0000142 on first command execution (help wanted)
content in a tty window not rendered correctly after serval interaction, but can be resolved after serval Resize

Design decisions:

Currently using go:linkname for os/exec.Cmd compatibility
- need to test with multiple go versions in CI to ensure go stdlib compatibility or
- need to implement new api for windows platform? (discouraged)

Credits goes to https://github.com/ActiveState/termtest, they provided a relatively good example for ConPty in go.

mattcanty · 2021-04-02T16:34:23Z

Interested in seeing this happen. Let me know if I can help in any way...

README.md

creack · 2021-04-05T17:15:30Z

@jeffreystoke I apologize, I still have this in my todo list, will get to it when I can.

@mattcanty This is quite a large change and I haven't find time yet to look into it. Need to test it, make sure it doesn't break existing codebases, doesn't force a large dependency tree, etc.

Ideally, we'd need some automated testing... Checking for various exec modes, daemons, foreground, etc, and ensure that it works in all OS/Arch while staying backward compatible

creack

Sorry again for the delay, I still didn't have time to test it, but here are some comments from reviewing the code

.gitignore

run_windows.s

pty_windows.go

doc.go

go.mod

run_windows.go

util_windows.go

creack · 2021-04-05T20:59:57Z

I managed to try on windows, however, I couldn't get anything to work. Would you have a sample code that would demonstrate how to use this on windows?

All my tests ended up with an error from pty.Start:

CreatePseudoConsole: The operation completed successfully.

A bit strange to have "success" to be an error, but it might be because I do something wrong.

I tried using the lib directly, and I also tried the lib your linked to (arhat.dev/pkg/exechelper), but without any luck.

JustinTimperio · 2021-04-05T22:05:10Z

Hi @creack and @jeffreystoke,

Glad to see some action here. It seems this is one of many libraries looking to make use of this new tool. You may be interested in some work done by @marcomorain in his repo go-conpty. I too have been searching for a Windows PTY solution but have ended up arriving at similar issues.

ghost · 2021-04-06T15:21:41Z

@creack #109 (comment)

Did you run as system admin? If not, can you try again with admin privileges to see whether it's a security issue?

As noted before, I'm not a windows expert, you may have to seek help from others, good luck :)

blasten · 2021-04-28T05:18:39Z

Thanks for working on this. I tried the following on Windows, but got an error:

if err := pty.InheritSize(os.Stdin, ptmx); err != nil {
  log.Printf("error resizing pty: %s", err)
}

produced "error resizing pty: The handle is invalid."

creack

Thank you for the update. I'll try to test it later today.

run_windows.go

winsize_unix.go

winsize_windows.go

doc.go

pty_windows.go

creack · 2021-05-31T08:15:39Z

Would you mind adding an example for windows in the readme? Once merge, this PR will be gone and the links provided in the description forgotten.

creack · 2021-05-31T08:18:19Z

nit: if you could push commits, it would make review easier. Keeping everything in one commit with force push is neat and tidy but makes it difficult to see what changed and requires a full re-review.
We can squash at the end while merging, resulting in a single commit.

ghost · 2021-05-31T08:33:18Z

Would you mind adding an example for windows in the readme? Once merge, this PR will be gone and the links provided in the description forgotten.

The usage is the same with other platforms', so I don't think it's necessary to create a special windows example, but we can note requirements for windows platform, wdyt?

Requirements: https://docs.microsoft.com/en-us/windows/console/createpseudoconsole#requirements

ghost · 2021-05-31T08:35:26Z

nit: if you could push commits, it would make review easier. Keeping everything in one commit with force push is neat and tidy but makes it difficult to see what changed and requires a full re-review.
We can squash at the end while merging, resulting in a single commit.

Ok, I will do that for following changes.

winsize.go

doc.go

ghost · 2021-05-31T10:09:59Z

@blasten The handle invalid error means the provided os.Stdin is not a ConPty handle, we have to add support for old windows console api to make it working, I guess that would happen in a new pull request?

creack · 2021-06-21T02:13:37Z

Sorry, didn't get time to dig into this yet. Something came to my attention though, no context, no detail but saw this:

leokhoa@4fa3b1e

As the lib would now panic if the DLL loading fails, we'll need to figure out want's going on, and whether fix the issue or make sure importing the lib doesn't break unrelated applications if a dll is missing.

ghost · 2021-08-29T09:58:57Z

@creack No idea why that would panic, but never mind, I have just pushed the latest implementation using LazyDLL and LazyProc, it will never panic on dll loading/proc finding failures as noted in my commit message.

ghost · 2021-08-30T13:24:51Z

fyi, go1.17 disabled //go:linkname support for private struct member, so this pull request will probably not be merged

ref: golang/go#46777 (comment)

ghost · 2021-08-30T13:44:57Z

Made it working for go1.17 according to the issue referenced above (also works with go1.16, probably will work for earlier go toolchain)

photostorm · 2021-08-30T16:19:07Z

Is there anything that can be done with "The operation completed successfully." being reported as error when calling CreatePseudoConsole or Resize?

photostorm · 2021-08-30T18:51:48Z

@jeffreystoke I made an attempt to fix the issue I was talking about and seeing if I could fix render bug. I do not work with Windows a lot. I could just been luck when I was testing my code, but it seem to fixed the issue. My copy of your fork: https://github.com/photostorm/pty

ghost · 2021-08-31T12:16:40Z

@photostorm me neither.

re: your fix

Good point, I completely misused the COORD, would you like to merge it on your own or something else?

photostorm · 2021-08-31T12:51:33Z

pty-patch.txt

I created a patch file for you.

Signed-off-by: Jeffrey Stoke <me@arhat.dev>

For backward compatibility Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Remove module dependency golang.org/x/sys/windows Signed-off-by: Jeffrey Stoke <me@arhat.dev>

All platforms share the same GetSize implementation Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Keep it constant with other platforms Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Add missing SmallRect Signed-off-by: Jeffrey Stoke <me@arhat.dev>

winsize: w to ws Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Always call LazyProc.Find() before calling LazyProc.Call() to avoid panic due to compulsory mustFind() call in Call() Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Signed-off-by: Jeffrey Stoke <me@arhat.dev>

* COORD passed as uintptr for pty creation and resizing This should fix unexpected error when creating and resize terminal * Check return value of syscalls to determine error state instead of using err directly This should resolve "The operation completed successfully." problem. photostorm contributed these fixes, I just removed some redundant variable declarations and made this commit. Signed-off-by: Jeffrey Stoke <me@arhat.dev> Co-authored-by: photostorm <3271896+photostorm@users.noreply.github.com> Co-authored-by: Jeffrey Stoke <me@arhat.dev>

bin101 · 2021-10-27T11:27:54Z

Is something still missing here so that this can be merged into main?

jkzilla · 2021-12-10T21:45:17Z

@creack are you available to take a glance to see if this matches and fixes the requested changes?

pete-woods · 2021-12-16T15:12:56Z

Apologies for a non-helpful 👍 comment, but I've been eagerly watching this PR since it was opened, and it would be a great xmas present to see it merged.

creack · 2022-03-27T12:19:53Z

I still haven't had the time to dig into this, I am sorry.

If someone could come up with an easy way to test it, it would be quite helpful as it is not clear to me in which scenario it is supposed to be working.

pete-woods · 2022-03-27T12:28:03Z

What we're hoping to use this for is a Go based SSH server where the terminal is provided by this package. At present the server works great on UNIX platforms, using this package, but has to fall back to plain stdout for Windows.

pete-woods · 2022-03-27T12:28:53Z

We're using this high level SSH server wrapper for the low level Go SSH package: https://github.com/gliderlabs/ssh

creack · 2022-03-27T14:01:01Z

Just tried the pty example from that repository, it doesn't even compile, which is quite bad as even if it is not supported, it shouldn't break compilation.
I'll push a fix for that asap and will keep digging. Still not sure what/how to test though. Once I run the pty example, how do I check that this PR works as expected? What am I supposed to see before/after?

pete-woods · 2022-03-27T14:14:27Z

You should have a working terminal, honestly that's it. Like pressing "up", ctrl-C, stuff like that should work. Previously you literally couldn't make a pty with your package at all on Windows, at the level of the code (intentionally) not compiling there.

creack · 2022-03-27T14:20:46Z

I just cloned the branch, it doesn't compile (go1.18)
Getting:

# github.com/creack/pty
.\cmd_windows.go:24:3: //go:linkname must refer to declared function or variable

pete-woods · 2022-03-27T14:43:58Z

Having never done Windows (or indeed any C/C++) bindings in Go, I couldn't offer better suggestions than you'd find on Google. Does this need to be compiled on a Windows machine?

At any rate, it seems like Windows would have to be added to the CI pipeline for this project in order to keep it building on Windows at very least.

drakkan · 2022-03-28T17:28:33Z

Hi,

I also tried this patch a few months ago. It compiled back then but had several issues with window resizing and other things I don't rember well. I added this feature externally to the library using conpty, unfortunately the code was part of a proprietary project and I cannot share it. I suggest to try conpty to the ones interested in this feature

lonnywong · 2022-05-29T08:21:08Z

Can't build with go1.18.1:

../go/pkg/mod/github.com/donorp/pty@v1.1.12-0.20211004111936-294eccab62ed/cmd_windows.go:24:3: //go:linkname must refer to declared function or variable

photostorm · 2022-07-27T01:35:39Z

I have opened a new pull request for ConPTY if everyone would like to try it.

#155

ghost mentioned this pull request Nov 26, 2020

Add basic CI pipeline for compatibility check #110

Closed

jawnsy mentioned this pull request Nov 29, 2020

Add Windows support coder/wsep#20

Open

ghost mentioned this pull request Nov 30, 2020

Support Windows through conpty #95

Open

mattcanty reviewed Apr 4, 2021

View reviewed changes

README.md Show resolved Hide resolved

creack requested changes Apr 5, 2021

View reviewed changes

JustinTimperio mentioned this pull request Apr 5, 2021

Error when building on windows gliderlabs/ssh#106

Closed

creack added the help wanted label May 4, 2021

creack requested changes May 31, 2021

View reviewed changes

run_windows.go Show resolved Hide resolved

winsize_unix.go Outdated Show resolved Hide resolved

winsize_windows.go Outdated Show resolved Hide resolved

doc.go Outdated Show resolved Hide resolved

pty_windows.go Outdated Show resolved Hide resolved

creack requested changes May 31, 2021

View reviewed changes

winsize.go Show resolved Hide resolved

winsize.go Outdated Show resolved Hide resolved

creack reviewed May 31, 2021

View reviewed changes

doc.go Show resolved Hide resolved

ghost requested a review from creack August 29, 2021 10:13

Jeffrey Stoke and others added 12 commits October 4, 2021 13:19

Unify Winsize and InheritSize for all platfoms

80786e2

Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Pty: Replace io.StringWriter with simple func

e9aa7ec

For backward compatibility Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Embed types and constants from external packages

5cd3e9a

Remove module dependency golang.org/x/sys/windows Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Move GetSize to winsize

f0bc158

All platforms share the same GetSize implementation Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Remove underline prefix for fields in WindowsPty

aee4483

Keep it constant with other platforms Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Use inline error check for Setsize in InheritSize

07b10c5

Signed-off-by: Jeffrey Stoke <me@arhat.dev>

docs: Add windows requirements note

9ac8ea3

Signed-off-by: Jeffrey Stoke <me@arhat.dev>

Fix embedded windows types

295ab95

Add missing SmallRect Signed-off-by: Jeffrey Stoke <me@arhat.dev>

chore: Fix arg name in comment

83b9ce2

winsize: w to ws Signed-off-by: Jeffrey Stoke <me@arhat.dev>

chore: Use LazyDLL and LazyProc

8eb0e58

Always call LazyProc.Find() before calling LazyProc.Call() to avoid panic due to compulsory mustFind() call in Call() Signed-off-by: Jeffrey Stoke <me@arhat.dev>

nit: Workaround go1.17 linkname relocation

f5cfe5c

Signed-off-by: Jeffrey Stoke <me@arhat.dev>

dcantah mentioned this pull request Nov 16, 2021

Add conpty (pseudo console) package microsoft/hcsshim#1228

Merged

ariary mentioned this pull request Mar 5, 2022

For interactive windows pty ariary/tacos#2

Open

anandrkskd mentioned this pull request Apr 22, 2022

Setup interactive testing for windows redhat-developer/odo#5573

Closed

jesseduffield mentioned this pull request Jun 18, 2022

Hold down arrow key to switch file, diff window stop render. jesseduffield/lazygit#1265

Closed

donorp closed this by deleting the head repository Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support pty on windows with ConPty api #109

Support pty on windows with ConPty api #109

ghost commented Nov 26, 2020 •

edited by ghost

Loading

mattcanty commented Apr 2, 2021

creack commented Apr 5, 2021

creack left a comment

creack commented Apr 5, 2021

JustinTimperio commented Apr 5, 2021

ghost commented Apr 6, 2021

blasten commented Apr 28, 2021

creack left a comment

creack commented May 31, 2021

creack commented May 31, 2021

ghost commented May 31, 2021

ghost commented May 31, 2021

ghost commented May 31, 2021

creack commented Jun 21, 2021

ghost commented Aug 29, 2021

ghost commented Aug 30, 2021

ghost commented Aug 30, 2021

photostorm commented Aug 30, 2021 •

edited

Loading

photostorm commented Aug 30, 2021 •

edited

Loading

ghost commented Aug 31, 2021

photostorm commented Aug 31, 2021

bin101 commented Oct 27, 2021

jkzilla commented Dec 10, 2021

pete-woods commented Dec 16, 2021

creack commented Mar 27, 2022

pete-woods commented Mar 27, 2022

pete-woods commented Mar 27, 2022

creack commented Mar 27, 2022

pete-woods commented Mar 27, 2022 •

edited

Loading

creack commented Mar 27, 2022

pete-woods commented Mar 27, 2022

drakkan commented Mar 28, 2022

lonnywong commented May 29, 2022

photostorm commented Jul 27, 2022 •

edited

Loading

Support pty on windows with ConPty api #109

Support pty on windows with ConPty api #109

Conversation

ghost commented Nov 26, 2020 • edited by ghost Loading

mattcanty commented Apr 2, 2021

creack commented Apr 5, 2021

creack left a comment

Choose a reason for hiding this comment

creack commented Apr 5, 2021

JustinTimperio commented Apr 5, 2021

ghost commented Apr 6, 2021

blasten commented Apr 28, 2021

creack left a comment

Choose a reason for hiding this comment

creack commented May 31, 2021

creack commented May 31, 2021

ghost commented May 31, 2021

ghost commented May 31, 2021

ghost commented May 31, 2021

creack commented Jun 21, 2021

ghost commented Aug 29, 2021

ghost commented Aug 30, 2021

ghost commented Aug 30, 2021

photostorm commented Aug 30, 2021 • edited Loading

photostorm commented Aug 30, 2021 • edited Loading

ghost commented Aug 31, 2021

photostorm commented Aug 31, 2021

bin101 commented Oct 27, 2021

jkzilla commented Dec 10, 2021

pete-woods commented Dec 16, 2021

creack commented Mar 27, 2022

pete-woods commented Mar 27, 2022

pete-woods commented Mar 27, 2022

creack commented Mar 27, 2022

pete-woods commented Mar 27, 2022 • edited Loading

creack commented Mar 27, 2022

pete-woods commented Mar 27, 2022

drakkan commented Mar 28, 2022

lonnywong commented May 29, 2022

photostorm commented Jul 27, 2022 • edited Loading

ghost commented Nov 26, 2020 •

edited by ghost

Loading

photostorm commented Aug 30, 2021 •

edited

Loading

photostorm commented Aug 30, 2021 •

edited

Loading

pete-woods commented Mar 27, 2022 •

edited

Loading

photostorm commented Jul 27, 2022 •

edited

Loading