feat: Add support for executing processes with Windows ConPty #311
Conversation
This prevents a io.ErrShortBuffer from occurring when the byte slice being read is smaller than the chunks sent from the opposite pipe. This makes sense for unordered connections, where transmission is not guarunteed, but does not make sense for TCP-like connections. We use a bufio.Reader when ordered to ensure data isn't lost.
Codecov Report
@@ Coverage Diff @@
## main #311 +/- ##
==========================================
- Coverage 67.76% 67.70% -0.07%
==========================================
Files 135 138 +3
Lines 7161 7256 +95
Branches 74 74
==========================================
+ Hits 4853 4913 +60
- Misses 1815 1841 +26
- Partials 493 502 +9
Continue to review full report at Codecov.
|
| if err != nil { | ||
| return nil, err | ||
| } | ||
| pathPtr, err := windows.UTF16PtrFromString(fullPath) |
There was a problem hiding this comment.
Good call on these 👍 Always confusing that Win32 APIs take UTF16 strings
| // dedupEnvCase is dedupEnv with a case option for testing. | ||
| // If caseInsensitive is true, the case of keys is ignored. | ||
| func dedupEnvCase(caseInsensitive bool, env []string) []string { |
There was a problem hiding this comment.
Oh ya... this is a confusing part of dealing with Windows APIs too (coming from Linux). Thanks for implementing this
| t.Run("Echo", func(t *testing.T) { | ||
| t.Parallel() | ||
| pty := ptytest.Start(t, exec.Command("cmd.exe", "/c", "echo", "test")) | ||
| pty.ExpectMatch("test") | ||
| }) |
There was a problem hiding this comment.
Very cool - thanks for adding a test for this!
|
|
||
| import "os/exec" | ||
|
|
||
| func Start(cmd *exec.Cmd) (PTY, error) { |
There was a problem hiding this comment.
This is a really nice cross-platform abstraction. This should be a go package - would've made our life easier!
There was a problem hiding this comment.
True. It would be a good stdlib addition!
| func (p *ptyWindows) Output() io.ReadWriter { | ||
| return readWriter{ | ||
| Reader: p.outputRead, | ||
| Writer: p.outputWrite, | ||
| } | ||
| } | ||
|
|
||
| func (p *ptyWindows) Input() io.ReadWriter { | ||
| return readWriter{ | ||
| Reader: p.inputRead, | ||
| Writer: p.inputWrite, | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice improvement to the interface! 💯
| func (p *otherPty) Input() io.ReadWriter { | ||
| return readWriter{ | ||
| Reader: p.tty, | ||
| Writer: p.pty, | ||
| } |
There was a problem hiding this comment.
This is so much cleaner than it was before
| pty := ptytest.Start(t, exec.Command("echo", "test")) | ||
| pty.ExpectMatch("test") |
There was a problem hiding this comment.
Thanks for adding this test too. Both of these pty tests here and on Windows add a lot of confidence in this abstraction
| @@ -47,6 +47,7 @@ func startPty(cmd *exec.Cmd) (PTY, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| // Taken from: https://github.com/microsoft/hcsshim/blob/2314362e977aa03b3ed245a4beb12d00422af0e2/internal/winapi/process.go#L6 | |||
There was a problem hiding this comment.
Thanks for adding this too!
|
And nice job figuring out those tricky Win32 API issues - like the magic const, the UTF16 string conversion, the environment variable handling, getting the console hooked up to the process, etc. |
I didn't think we'd need to execute Windows processes inside an emulated TTY, but it's required to make the SSH experience pleasant.
The
consolepackage @bryphe-coder was so extremely helpful to making this work, it's crazy! I minified our usage of theconsolepackage as well to be entirely bare-bones. It's almost unrecognizable.For the
startfunction, I primarily read through the documentation linked above the function to make it concise. The library we were using did a lot of bespoke things that are in the Go stdlib (assuming that's because it's old).I took the liberty of stealing the
ptyname. It felt reasonable, since nothing in our code should use another externalptylibrary, otherwise it will lack cross-platform support.This is the precursor to the workspace agent, which should allow us to complete the entire user-flow.