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

TERM=dumb fixes #10131

Merged
merged 4 commits into from
Mar 29, 2024
Merged

TERM=dumb fixes #10131

merged 4 commits into from
Mar 29, 2024

Conversation

intelfx
Copy link
Contributor

@intelfx intelfx commented Mar 2, 2024

  • libmain/progress-bar: try harder to avoid escape sequences if !isTTY
  • treewide: replace usages of isatty(STDERR_FILENO) with shouldANSI()
  • treewide: shouldANSI() -> isTTY()
  • libutil/terminal: cache isTTY()

Motivation

Currently, progress bar code emits control sequences (at least \r and CSI K)
regardless of whether nix allows other escape sequences in its output (as
determined by nix::shouldANSI() function.

Rework progress bar code such that !isTTY disables emission of all control
sequences, including \r.

Additionally, rename nix::shouldANSI() into nix::isTTY() to better reflect
the semantics of this function.

Context

Running nix copy with TERM=dumb and/or standard streams redirected into
a pipe still causes nix to emit several control sequences, interfering with
programmatic parsing of its output:

    "msg": [
        "",
        "\u001b[KWarning: Permanently added '<redacted>' (ED25519) to the list of known hosts.",
        "",
        "\u001b[Kcopying path '/nix/store/xdrsf841kdlf95rxd3qy6yhi57s25krr-dns-root-data-2023-11-27' from 'https://cache.nixos.org'...",
        "copying path '/nix/store/l3lhzwrfgzncjj1z4wyq6kkrp6czx5qp-gcc-12.3.0-libgcc' from 'https://cache.nixos.org'...",
        <...>

Priorities and Process

Add 👍 to pull requests you find important.

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

@intelfx intelfx requested a review from edolstra as a code owner March 2, 2024 16:58
@github-actions github-actions bot added the new-cli Relating to the "nix" command label Mar 2, 2024
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Thanks!

@intelfx
Copy link
Contributor Author

intelfx commented Mar 8, 2024

  • I don't think 8c1eeb4 is worthwhile (shouldANSI feels like a better name to me)

It's just a bit of a misnomer, because semantically it also controls whether to emit things like \r which are not ANSI control codes (thus it does not answer the question of "whether our terminal is ANSI X3.64 capable", but rather "is the thing attached to our stderr a terminal at all").

Specifically, this hunk is needed to achieve reasonable UX (we should log individual actions if and only if the progress bar is not available):

https://github.com/NixOS/nix/pull/10131/files#diff-20a8b5b2a231db80eab27840bd32ac0214aa0c4e9e923e649d3d741c3da77b48R379-R381

...but if I saw a line like if (nix::shouldANSI()) verbosity = ... in an unfamiliar codebase I'd get slightly confused because there is no obvious link between ANSI capabilities and verbosity.


That said, I do not feel strongly about it, if you think it's not worthwhile, I'll drop it :)

@intelfx
Copy link
Contributor Author

intelfx commented Mar 26, 2024

@thufschmitt gentle ping — does the reasoning above make any additional sense for you, or do you still think I should drop the rename?

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @intelfx

Nah, let's keep it like that. Will merge as soon as the CI is OK

@thufschmitt thufschmitt merged commit 90f5189 into NixOS:master Mar 29, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants