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.

Sign up for GitHub

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

Jump to bottom

tty in local_resource kills tilt #6378

Closed
nicks opened this issue May 20, 2024 · 2 comments · Fixed by #6382 or #6390
Closed

tty in local_resource kills tilt #6378

nicks opened this issue May 20, 2024 · 2 comments · Fixed by #6382 or #6390
Labels
bug Something isn't working

Comments

@nicks
Copy link
Member

nicks commented May 20, 2024 •
edited
Loading

Steps to Reproduce

Tiltfile:

local_resource('test', ['./test.sh'])

test.sh:

#!/bin/bash

zsh -i -l -c 'exit;'
  1. Run tilt up and wait a little while
  2. Hit space

Expected Behavior

Tilt should successfully run the command

Current Behavior

On macOS, tilt crashes with suspended (tty input)
On Linux, the terminal gets mysteriously detached from the tilt process
@nicks nicks added the bug Something isn't working label May 20, 2024
@nicks
Copy link
Member Author

nicks commented May 21, 2024

i was able to get something working by attaching a pty to stdin of the process...

i'm not sure if that's a good general solution (since some stuff may behave differently when it's interacting with a pty) but i guess we can make it an option

nicks added a commit that referenced this issue May 23, 2024
@nicks
tiltfile: add stdin_mode='pty' to local_resouce()
bb2e634 
Allows users to attach a pty,
for interactive commands that try
to grab the "main" tty

fixes #6378

Signed-off-by: Nick Santos <nick.santos@docker.com>
nicks added a commit that referenced this issue May 24, 2024
@nicks
tiltfile: add stdin_mode='pty' to local_resouce()
353d130 
Allows users to attach a pty,
for interactive commands that try
to grab the "main" tty

fixes #6378

Signed-off-by: Nick Santos <nick.santos@docker.com>
@nicks nicks mentioned this issue May 24, 2024
Merged
nicks added a commit that referenced this issue May 24, 2024
@nicks
tiltfile: add stdin_mode='pty' to local_resouce()
405400d 
Allows users to attach a pty,
for interactive commands that try
to grab the "main" tty

fixes #6378

Signed-off-by: Nick Santos <nick.santos@docker.com>
nicks added a commit that referenced this issue May 24, 2024
@nicks
tiltfile: add stdin_mode='pty' to local_resouce()
1a73ffa 
Allows users to attach a pty,
for interactive commands that try
to grab the "main" tty

fixes #6378

Signed-off-by: Nick Santos <nick.santos@docker.com>
nicks added a commit that referenced this issue May 24, 2024
@nicks
tiltfile: add stdin_mode='pty' to local_resouce() (#6382)
20ab76b 
Allows users to attach a pty,
for interactive commands that try
to grab the "main" tty

fixes #6378

Signed-off-by: Nick Santos <nick.santos@docker.com>
@nicks nicks mentioned this issue May 31, 2024
Closed
@nicks nicks reopened this May 31, 2024
@nicks
Copy link
Member Author

nicks commented May 31, 2024

the pr was rolled back

nicks added a commit that referenced this issue May 31, 2024
@nicks
procutil: use setsid instead of setpgid
0999672 
this starts to stretch my knowledge
of low-level unix process apis, but
my current understanding is:
- every process has a process group id and a session id
- all processes in a group belong to the same session
- setpgid creates a new process group id in the same session
- setsid creates a new sessions and new process group
- process groups are used to send signals to sets of processes
- sessions are used to attach a process to a controlling terminal

using a new session here ensures
that the subprocess can't run something
that closes the terminal controlling tilt,
or put tilt in the background.

fixes #6378
fixes #6387

Signed-off-by: Nick Santos <nick.santos@docker.com>
@nicks nicks mentioned this issue May 31, 2024
Merged
nicks added a commit that referenced this issue Jun 3, 2024
@nicks
procutil: use setsid instead of setpgid (#6390)
c38f0fd 
this starts to stretch my knowledge
of low-level unix process apis, but
my current understanding is:
- every process has a process group id and a session id
- all processes in a group belong to the same session
- setpgid creates a new process group id in the same session
- setsid creates a new sessions and new process group
- process groups are used to send signals to sets of processes
- sessions are used to attach a process to a controlling terminal

using a new session here ensures
that the subprocess can't run something
that closes the terminal controlling tilt,
or put tilt in the background.

fixes #6378
fixes #6387

Signed-off-by: Nick Santos <nick.santos@docker.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
1 participant
@nicks