Skip to content

fix: allow tui to open /dev/tty as fallback#9037

Merged
jedevc merged 2 commits into
dagger:mainfrom
jedevc:try-opening-tty-as-fallback
Nov 22, 2024
Merged

fix: allow tui to open /dev/tty as fallback#9037
jedevc merged 2 commits into
dagger:mainfrom
jedevc:try-opening-tty-as-fallback

Conversation

@jedevc
Copy link
Copy Markdown
Contributor

@jedevc jedevc commented Nov 22, 2024

Some various little things found while investigating TTY magic in the progress output. Original discussion in discord.

It seems like we'll also need a bubbletea fix, as noticed by @vito:

https://github.com/charmbracelet/bubbletea/blob/4ad07926d7ff00bc21a05b2536d82a7cc629225e/tea.go#L738-L740 it does this unconditionally vs. https://github.com/charmbracelet/bubbletea/blob/4ad07926d7ff00bc21a05b2536d82a7cc629225e/tea.go#L582-L586

However, we should be able to cleverly fallback to /dev/tty if it exists, which is what this PR does.

You can simulate /dev/tty not existing by using the setsid wrapper (however, this still breaks until the above-mentioned bubbletea fix can be made). E.g.:

❯ cat /dev/tty
...
❯ setsid cat /dev/tty
cat: /dev/tty: No such device or address

fix: avoid panic if no stdin is available when launching terminal

If it's available, we should try to fallback to /dev/tty as a fallback.
This logic is borrowed from bubbletea, however, we can't just use
WithInputTTY, since this requires that /dev/tty exists - we want to
continue on, even if it doesn't.

This allows us to neatly handle the echo "" | dagger shell case -
stdin is not a TUI, but we can still usually find one at /dev/tty, so
we should try and do that.

Note: we also do a little refactor of findTTYs to directly return
io.Reader/io.Writers - this avoids the horror of typed nils, which
can result in passing a nil os.File into bubbletea (which is does
not like
).


fix: avoid panic if no stdin is available when launching terminal

This really doesn't happen except in truly bizarrely concocted test
cases - it's just nice to not panic if it does happen.

If it's available, we should try to fallback to `/dev/tty` as a fallback.
This logic is *borrowed* from bubbletea, however, we can't just use
`WithInputTTY`, since this requires that `/dev/tty` exists - we want to
continue on, even if it doesn't.

This allows us to neatly handle the `echo "" | dagger shell` case -
stdin is not a TUI, but we can still usually find one at `/dev/tty`, so
we should try and do that.

Note: we also do a little refactor of `findTTYs` to directly return
`io.Reader`/`io.Writer`s - this avoids the horror of typed nils, which
can result in passing a `nil` `os.File` into bubbletea (which is *does
not like*).

Signed-off-by: Justin Chadwell <me@jedevc.com>
This *really* doesn't happen except in truly bizarrely concocted test
cases - it's just nice to not panic if it does happen.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc requested review from helderco and vito November 22, 2024 16:14
@jedevc jedevc merged commit a49c5d3 into dagger:main Nov 22, 2024
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.

3 participants