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

fix(libutil/terminal): require both stderr/stdout to be tty to output color #11994

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

Conversation

xokdvium
Copy link
Contributor

Motivation

Fixes #11583
Fixes #4848

This is a bit more conservative and safe for cli composability. A common use-case for nix cli is to filter/transform output with grep/ripgrep/jq from stdout. Those tools usually have their own colored output. It's best not to step on their toes.

Once example of such command is: nix registry list | rg nixpkgs

Note that this is more of a band-aid solution. Nix generally handles ANSI colors in an ad-hoc way and there might be some other unexpected bugs related to control characters.

Context


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

… color

This is a bit more conservative and safe for cli composability. A common use-case
for nix cli is to filter/transform output with grep/ripgrep/jq from stdout. Those
tools usually have their own colored output. It's best not to step on their toes.

Once example of such command is: `nix registry list | rg nixpkgs`

Note that this is more of a band-aid solution. Nix generally handles ANSI colors
in an ad-hoc way and there might be some other unexpected bugs related to control
characters.
@xokdvium xokdvium requested a review from edolstra as a code owner November 29, 2024 20:05
@xokdvium
Copy link
Contributor Author

My linux-jitsu is too weak to implement regression tests for this. Is there some way to redirect output to a file but pretending that's a tty? Not sure if using strace would be a great and portable approach either.

@xokdvium
Copy link
Contributor Author

@Ericson2314, I'm not very familiar with portability for windows. Are we okay with generally using POSIX isatty with STDOUT_FILENO? Should this be cleaned up or is it fine as-is?

@Mic92
Copy link
Member

Mic92 commented Dec 1, 2024

My linux-jitsu is too weak to implement regression tests for this. Is there some way to redirect output to a file but pretending that's a tty? Not sure if using strace would be a great and portable approach either.

You can use the script command for this.

 faketty () {
     script -qefc "$(printf "%q " "$@")" /dev/null
 }

@@ -18,6 +18,7 @@ bool isTTY()
{
static const bool tty =
isatty(STDERR_FILENO)
&& isatty(STDOUT_FILENO)
Copy link
Member

@Mic92 Mic92 Dec 1, 2024

Choose a reason for hiding this comment

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

Only caveat I see just now is that we have tooling like cachix (maybe also attic?), will no have less fancy builds logs:

nix-build | cachix push

Also was nix-build output maybe not relying on this.

@Mic92
Copy link
Member

Mic92 commented Dec 1, 2024

Do the said commands actually profit from colors much?
We could just unconditionally apply filterANSIEscapes and call it a day.

@edolstra
Copy link
Member

edolstra commented Dec 2, 2024

I'm not sure about this. It's pretty reasonable to do something like nix some-command --json > out and still expect to get the progress bar and colors in interactive terminals.

@roberth
Copy link
Member

roberth commented Dec 2, 2024

Linux has atomic write behavior for single syscalls up to 4k or page size or something, so if both commands use a single write syscall per line, then the result would be mostly not scrambled, especially if Nix keeps an empty line above the progress bar, and puts the cursor there after its refresh (otherwise the progress bar sort of becomes a background).
I know it's not a complete solution, but it might help for some of the cases where it works, or where some mild scrambling isn't the end of the world.

I'm not looking forward to bug reports like "nix some-command --json >out hangs" where it turns out to be a slow network or something, so I'm leaning towards @edolstra's skepticism here.

@Mic92
Copy link
Member

Mic92 commented Dec 3, 2024

So the question is, how can we make stdout not containing colour when the output is not a tty?

@edolstra
Copy link
Member

edolstra commented Dec 3, 2024

So the question is, how can we make stdout not containing colour when the output is not a tty?

Our stdout output doesn't contain color AFAIK. For example, the submitter's example (nix registry list) doesn't have color.

I think the real issue is that the progress bar doesn't play nice with other commands when piped into them because it does raw terminal I/O (like moving the cursor).

@Mic92
Copy link
Member

Mic92 commented Dec 3, 2024

Than is this issue invalid? #11583 I was also trying to reproduce this issue locally but couldn't find any colors in the output. I haven't looked for non-visible ansi characters.

@roberth
Copy link
Member

roberth commented Dec 3, 2024

raw terminal I/O

Doesn't have to be "raw". Basically everything can be ANSI bytes, and those don't have to be chunked in any particular way.

Than is this issue invalid? #11583

Probably valid. The progress bar is happy to appear, disappear, and leave the cursor at the start of the line.

Value printing in the repl is supposed to be streaming, but apparently also "permits" progress bar updates or something while it's doing that.
To fix that without compromising the streaming (laziness) property, the repl needs to become a proper TUI. I hope C++ has a good library for combining TUI-like behaviors and log streaming, because that interaction is not at all trivial to get right.

@edolstra
Copy link
Member

edolstra commented Dec 4, 2024

Value printing in the repl is supposed to be streaming, but apparently also "permits" progress bar updates or something while it's doing that.

That's because it needs to call logger->pause() before starting printValue().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants