reuse same code for setting pipes in run/exec#12659
reuse same code for setting pipes in run/exec#12659crosbymichael merged 1 commit intomoby:masterfrom
Conversation
|
LGTM |
|
@cpuguy83 posted some test code here https://gist.github.com/cpuguy83/b98f4da1207b9b4ed82b and this has not passed his test yet. I'm still finding out why. |
dqminh
force-pushed
the
exec-interactive-hang
branch
from
281935a to
2e823ce
Compare
There was a problem hiding this comment.
This is causing a failure on Windows... but seems like something we should be testing on Windows too.
There was a problem hiding this comment.
We extracted tests that has kr/pty into _unix.go files b/c pty does a lot of unix stuff to control the terminal and none of those stuff exists on windows (pkg doesnt even compile on windows). I don't think we can easily run kr/pty on windows @cpuguy83 but folks working on engine may come up with a solution there.
There was a problem hiding this comment.
If we don't have a way to do this today (even not using kr/pty), then yeah, let's move to _unix.go
There was a problem hiding this comment.
i will move this to _unix.go first, just to make sure that it passes the current CI tests.
There was a problem hiding this comment.
It looks to me like this test can be written without kr/pty using exec.Cmd.
dqminh
force-pushed
the
exec-interactive-hang
branch
from
2e823ce to
d3883e4
Compare
This also moves `exec -i` test to _unix_test.go because it seems to need a pty to reliably reproduce the behavior. Signed-off-by: Daniel, Dao Quang Minh <[email protected]>
dqminh
force-pushed
the
exec-interactive-hang
branch
from
d3883e4 to
ade8146
Compare
|
LGTM |
1 similar comment
|
LGTM |
reuse same code for setting pipes in run/exec
7 participants
Fixes #12546
The command's stdin pipe has to be an *os.File, otherwise bad things happen. This tries to reuse some code of
runto setup the pipes becauseruncode path already took care of that.