From 292567e0b0a4681eb8ca803c26293d70857fe387 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Sat, 10 Aug 2024 16:02:42 -0700 Subject: [PATCH] fix: check if it is a Real terminal, not just if it is a terminal This will stop printing stuff to dumb terminals that they don't support. I've overall audited usage of isatty and replaced the ones with intent to mean "is a Real terminal" with checking for that. I've also caught a case of carelessly assuming "is a tty" means "should be colour" in nix-env. Change-Id: I6d83725d9a2d932ac94ff2294f92c0a1100d23c9 --- src/libmain/shared.cc | 4 ++-- src/libutil/logging.cc | 10 +++++++++- src/libutil/terminal.cc | 15 ++++++++++----- src/libutil/terminal.hh | 24 +++++++++++++++++++++++- src/nix-env/nix-env.cc | 5 ++--- src/nix/main.cc | 1 + src/nix/prefetch.cc | 6 +++--- 7 files changed, 50 insertions(+), 15 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index a407c647f..018e34509 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -5,9 +5,9 @@ #include "signals.hh" #include "loggers.hh" #include "current-process.hh" +#include "terminal.hh" #include -#include #include #include @@ -347,7 +347,7 @@ int handleExceptions(const std::string & programName, std::function fun) RunPager::RunPager() { - if (!isatty(STDOUT_FILENO)) return; + if (!isOutputARealTerminal(StandardOutputStream::Stdout)) return; char * pager = getenv("NIX_PAGER"); if (!pager) pager = getenv("PAGER"); if (pager && ((std::string) pager == "" || (std::string) pager == "cat")) return; diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index cbeb7aa36..7d9482814 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -37,7 +37,15 @@ void Logger::warn(const std::string & msg) void Logger::writeToStdout(std::string_view s) { - writeFull(STDOUT_FILENO, filterANSIEscapes(s, !shouldANSI(), std::numeric_limits::max(), false)); + writeFull( + STDOUT_FILENO, + filterANSIEscapes( + s, + !shouldANSI(StandardOutputStream::Stdout), + std::numeric_limits::max(), + false + ) + ); writeFull(STDOUT_FILENO, "\n"); } diff --git a/src/libutil/terminal.cc b/src/libutil/terminal.cc index 2ba1cb81b..25e97e599 100644 --- a/src/libutil/terminal.cc +++ b/src/libutil/terminal.cc @@ -7,7 +7,12 @@ namespace nix { -bool shouldANSI() +bool isOutputARealTerminal(StandardOutputStream fileno) +{ + return isatty(int(fileno)) && getEnv("TERM").value_or("dumb") != "dumb"; +} + +bool shouldANSI(StandardOutputStream fileno) { // Implements the behaviour described by https://bixense.com/clicolors/ // As well as https://force-color.org/ for compatibility, since it fits in the same shape. @@ -16,14 +21,14 @@ bool shouldANSI() // unset x set Yes // unset x unset If attached to a terminal // [we choose the "modern" approach of colour-by-default] - auto compute = []() -> bool { + auto compute = [](StandardOutputStream fileno) -> bool { bool mustNotColour = getEnv("NO_COLOR").has_value() || getEnv("NOCOLOR").has_value(); bool shouldForce = getEnv("CLICOLOR_FORCE").has_value() || getEnv("FORCE_COLOR").has_value(); - bool isTerminal = isatty(STDERR_FILENO) && getEnv("TERM").value_or("dumb") != "dumb"; + bool isTerminal = isOutputARealTerminal(fileno); return !mustNotColour && (shouldForce || isTerminal); }; - static bool cached = compute(); - return cached; + static bool cached[2] = {compute(StandardOutputStream::Stdout), compute(StandardOutputStream::Stderr)}; + return cached[int(fileno) - 1]; } // FIXME(jade): replace with TerminalCodeEater. wowie this is evil code. diff --git a/src/libutil/terminal.hh b/src/libutil/terminal.hh index 2c422ecff..28c96c780 100644 --- a/src/libutil/terminal.hh +++ b/src/libutil/terminal.hh @@ -6,6 +6,26 @@ namespace nix { +enum class StandardOutputStream { + Stdout = 1, + Stderr = 2, +}; + +/** + * Determine whether the output is a real terminal (i.e. not dumb, not a pipe). + * + * This is probably not what you want, you may want shouldANSI() or something + * more specific. Think about how the output should work with a pager or + * entirely non-interactive scripting use. + * + * The user may be redirecting the Lix output to a pager, but have stderr + * connected to a terminal. Think about where you are outputting the text when + * deciding whether to use STDERR_FILENO or STDOUT_FILENO. + * + * \param fileno file descriptor number to check if it is a tty + */ +bool isOutputARealTerminal(StandardOutputStream fileno); + /** * Determine whether ANSI escape sequences are appropriate for the * present output. @@ -18,8 +38,10 @@ namespace nix { * - CLICOLOR_FORCE or FORCE_COLOR set -> enable colour * - The output is a tty; TERM != "dumb" -> enable colour * - Otherwise -> disable colour + * + * \param fileno which file descriptor number to consider. Use the one you are outputting to */ -bool shouldANSI(); +bool shouldANSI(StandardOutputStream fileno = StandardOutputStream::Stderr); /** * Truncate a string to 'width' printable characters. If 'filterAll' diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index 6dda34a32..13fadb1d8 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -1,6 +1,7 @@ #include "attr-path.hh" #include "common-eval-args.hh" #include "derivations.hh" +#include "terminal.hh" #include "eval.hh" #include "get-drvs.hh" #include "globals.hh" @@ -17,7 +18,6 @@ #include "legacy.hh" #include "eval-settings.hh" // for defexpr -#include #include #include #include @@ -1092,7 +1092,6 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) return; } - bool tty = isatty(STDOUT_FILENO); RunPager pager; Table table; @@ -1171,7 +1170,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) } } else { auto column = (std::string) "" + ch + " " + version; - if (diff == cvGreater && tty) + if (diff == cvGreater && shouldANSI(StandardOutputStream::Stdout)) column = ANSI_RED + column + ANSI_NORMAL; columns.push_back(column); } diff --git a/src/nix/main.cc b/src/nix/main.cc index 1c1e9df7e..9cbe303ac 100644 --- a/src/nix/main.cc +++ b/src/nix/main.cc @@ -362,6 +362,7 @@ void mainWrapped(int argc, char * * argv) setLogFormat(LogFormat::bar); Finally f([] { logger->pause(); }); settings.verboseBuild = false; + // FIXME: stop messing about with log verbosity depending on if it is interactive use if (isatty(STDERR_FILENO)) { verbosity = lvlNotice; } else { diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc index 0b04a04e6..b99cd5dd0 100644 --- a/src/nix/prefetch.cc +++ b/src/nix/prefetch.cc @@ -4,11 +4,11 @@ #include "shared.hh" #include "store-api.hh" #include "filetransfer.hh" -#include "finally.hh" #include "tarfile.hh" #include "attr-path.hh" -#include "eval-inline.hh" +#include "eval-inline.hh" // IWYU pragma: keep #include "legacy.hh" +#include "terminal.hh" #include @@ -180,7 +180,7 @@ static int main_nix_prefetch_url(int argc, char * * argv) if (args.size() > 2) throw UsageError("too many arguments"); - if (isatty(STDERR_FILENO)) + if (isOutputARealTerminal(StandardOutputStream::Stderr)) setLogFormat(LogFormat::bar); auto store = openStore();