Skip to content
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

Add StartWithAttrs to allow bypassing setsid/setctty #97

Merged
merged 1 commit into from
May 7, 2020

Conversation

creack
Copy link
Owner

@creack creack commented May 5, 2020

Revert #75 but don't change the "old" behavior of pty.Start and pty.StartWithSize.

Introduce pty.StartWithAttr to explicitly set process attributes, which allow for pty to be created without setsid/setctty, fixing the issue faced by aerc.

Fixes error with the new go1.15 behavior introduced in https://go-review.googlesource.com/c/go/+/231638/ ( golang/go#29458 ).

Fixes #96

cc @kr @ddevault @myitcv

Signed-off-by: Guillaume J. Charmes <git+guillaume@charmes.net>
@ddevault
Copy link
Contributor

ddevault commented May 5, 2020

How would aerc need to use this?

@myitcv
Copy link

myitcv commented May 5, 2020

Thanks - 943a9a2 seems to fix the problem I was seeing. Tested against Go +b4ecafc986

@creack
Copy link
Owner Author

creack commented May 5, 2020

@ddevault I submitted a patch here https://lists.sr.ht/~sircmpwn/aerc/patches/10481 it fixes the pty-example case. Please let me know if you find more issues.

@ddevault
Copy link
Contributor

ddevault commented May 5, 2020

Thanks for the patch!

@myitcv
Copy link

myitcv commented May 5, 2020

Is the plan to get this merged soon-ish?

@creack
Copy link
Owner Author

creack commented May 5, 2020

Will merge and publish new tag tomorrow.

@myitcv
Copy link

myitcv commented May 6, 2020

Will merge and publish new tag tomorrow.

Thanks. Just noting Ian's reply in #96 (comment)

@creack
Copy link
Owner Author

creack commented May 6, 2020

I considered passing the tty as ExtraFiles, or even setting Ctty to stdout when stdin is overridden, but as the change is relatively recent and splitting a tty in part if usually a bad idea, I think it is better to revert the main helper to the original behavior.

Edge cases needing to do that can use the new helper or directly call pty.Open.

@creack creack merged commit f272787 into master May 7, 2020
@creack creack deleted the creack/start-with-attrs branch May 7, 2020 13:36
tgulacsi pushed a commit to tgulacsi/aerc that referenced this pull request Aug 20, 2020
Soves an issue with go1.15 not letting ctty be a parent. See
creack/pty#97 for more details.

Signed-off-by: Guillaume J. Charmes <git+guillaume@charmes.net>
notnoop pushed a commit to hashicorp/nomad that referenced this pull request Sep 8, 2020
Starting with golang 1.5, setting Ctty value result in `Setctty set but Ctty not valid in child` error, as part of golang/go#29458 .
This commit lifts the fix in creack/pty#97 .
notnoop pushed a commit to hashicorp/nomad that referenced this pull request Sep 9, 2020
Upgrade to golang 1.15

Starting with golang 1.5, setting Ctty value result in `Setctty set but Ctty not valid in child` error, as part of golang/go#29458 .
This commit lifts the fix in creack/pty#97 .
fredrikhgrelland pushed a commit to fredrikhgrelland/nomad that referenced this pull request Sep 28, 2020
Upgrade to golang 1.15

Starting with golang 1.5, setting Ctty value result in `Setctty set but Ctty not valid in child` error, as part of golang/go#29458 .
This commit lifts the fix in creack/pty#97 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setctty set but Ctty not valid in child with Go +be08e10b3b
3 participants