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

fix: set stdout in exec when not tty is available #758

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+5 −1
Open

fix: set stdout in exec when not tty is available #758

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,11 @@ func (p *Program) exec(c ExecCommand, fn ExecCallback) {
}

c.SetStdin(p.input)
c.SetStdout(p.output.TTY())
if p.output.TTY() != nil {
c.SetStdout(p.output.TTY())
} else {
c.SetStdout(p.output)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does setting the output to p.output differ from the (nil)file returned by p.output.TTY()?
If the goal is to output to stdout then would using os.Stdout work similarly to setting Stderr below?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hopefulTex!

As background, I'm doing this from a Wish (SSH) application. I debugged Wish, Bubble Tea, and termenv to see where would be the best place for this fix. With this in mind, let me answer your questions.

How does setting the output to p.output differ from the (nil)file returned by p.output.TTY()?

If the output is nil, the client won't display anything on the screen.

If the goal is to output to stdout then would using os.Stdout work similarly to setting Stderr below?

I'm afraid the goal is not to output to os.Stdout. Doing so would show the output of the execution on the server's screen rather than the client's.

The underlying problem is that Bubble Tea's option WithOutput(...) will convert the output (ssh.Session which implements the io.Writer interface) into a termenv.Output. Then, termenv's output.TTY() will return nil as tty doesn't implement the termenv.File interface (ssh.Session doesn't define Fd() uintptr, which makes sense as it doesn't have a file descriptor).

So, the possible places to fix this issue are:

  1. Wish's Bubble Tea middleware. Wrap the output in a struct that defines a dummy FD() function. But, I would discard this option, as it just instructs to use bubbletea.WithOutput(...).
  2. Bubble Tea's WithOutput() option, check if the output is an ssh.Session. If so, could you do the wrapping defined in 1? But, it doesn't seem to be the right place as it would add a dependency on charmbracelet/ssh. Or, checking if the output is an ssh.Session, and then not creating a termenv.Output (but it would lose termenv's features).
  3. In termenv's output.TTY(), checking if tty is an ssh.Session. However, it has the same issue as in the previous option.
  4. Finally, the last option I can think of would be to define the dummy Fd() uintptr method in (ssh.session)[https://github.com/charmbracelet/ssh/blob/master/session.go]. To be honest, this would be another option, but I'm torn because defining a dummy file descriptor doesn't sound like a good thing to do.

Yet another option would be to create a Bubble that receives the ssh.Session (as I wouldn't have access to the program input/output as they are not exported). But, the bug would still happen if someone else wants to execute a remote command using a Bubble Tea application with Wish.

I created two sample applications that reproduce the issue. One doesn't allocate a pty, so the command (journalctl -f) just shows the output to the client. While the other one starts an interactive bash session: https://gist.github.com/ivanvc/cb4580c02e8bb04d17ef286c9183098b

}
c.SetStderr(os.Stderr)

// Execute system command.
Expand Down