-
-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set Ctty in SysProcAttr #75
Conversation
Is there any chance there are programs that rely on the current behavior (and would therefore be broken by this change)? |
It seems very unlikely that there would be. It would basically require a program using kr/pty to have a child process which explicitly wants /dev/tty to be broken, but process the child's stdout on a pty. That would be very weird indeed. |
That makes sense. Thanks for the patch! |
Cheers! |
I think this is a bug in golang if that is the case. It is utterly wrong for this package to behave in the manner prior to the patch - perhaps half of the use-cases would stop working. Programs like less, vipe, fzf, and more would cease to be useful. My downstream is aerc, which relies on this feature heavily, for example by piping emails into less. |
I just started digginginto this after a while, I am a bit rusty, but the Golang patch seems to make sense. As pty.Start creates a detached process in a new session, the controlling terminal is not the parent's try, but the local one, i.e. 0. I'll look into |
Here's a more-or-less minimal reproducable example: https://git.sr.ht/~sircmpwn/pty-example/tree/master/main.go Note that the go.mod in this repo has a replacement directive which is necessary for this to build right, so don't just copy+paste this code. It writes some text into /bin/less's stdin and runs it in a pty, then displays it in aerc's interactive terminal emulator. Most of the relevant pty code lives in aerc, but there you have it. |
Thank you for the sample. I did reproduce. After digging into the aerc code, I think I understand the problem. aerc uses lib-vterm and runs a pty process inside of it while expected vterm to be the controlling terminal. As vterm is already the "head" of the session, the process could simply inherit it instead of creating a new one. I managed to run the example properly by setting I'll do some more digging, but at first glance, it looks like the 1.15 is the expected behavior. |
Without this change, the controlling TTY is not set, so processes like less cannot have their stdin redirected. This was partially fixed many years ago, but it seemed the last piece of the puzzle was never put in.