ninja: disable line-clearing with TERM=dumb

[milahu]

Description of changes

TERM=dumb ninja is 1% prettier than the current solution ninja | cat

cat shows up as a separate process in my process tree
TERM=dumb is less visible

based on #149810 (comment)

ping @pennae

Background

https://github.com/ninja-build/ninja/blob/master/src/line_printer.cc

  const char* term = getenv("TERM");
#ifndef _WIN32
  smart_terminal_ = isatty(1) && term && string(term) != "dumb";

  if (smart_terminal_) {
    printf("\r");  // Print over previous line, if any.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[pennae]

if it works all the same then that's fine. should target staging though because it'll cause a mass rebuild.

[milahu]

lets set TERM=dumb as default?

for example, this ninja gives no output

nixpkgs/pkgs/development/tools/build-managers/gn/generic.nix

Lines 41 to 45 in f72be3a

	buildPhase = ''
	python build/gen.py --no-last-commit-position
	ln -s ${lastCommitPosition} out/last_commit_position.h
	ninja -j $NIX_BUILD_CORES -C out gn
	'';

or lets patch ninja to detect nix-build's "dumb" terminal

[pennae]

nix-build largely passes TERM from the invoking shell to the builder, so patching ninja wouldn't help very much. setting TERM=dumb by default would also affect all other builds and all other commands. that might arguably be a good thing for reproducibility, but it'll mess with builders that can output pretty and useful logs right now :(

[milahu]

ninja could do getenv("NIX_STORE")

or similar

NIX_BINTOOLS=/nix/store/lvg99f3zni6zw4cvlci6wpmzlls0nsn4-binutils-wrapper-2.38
NIX_BINTOOLS_WRAPPER_TARGET_HOST_x86_64_unknown_linux_gnu=1
NIX_BUILD_CORES=3
NIX_BUILD_TOP=/build
NIX_CC=/nix/store/61zfi5pmhb0d91422f186x26v7b52y5k-gcc-wrapper-11.3.0
NIX_CC_WRAPPER_TARGET_HOST_x86_64_unknown_linux_gnu=1
NIX_CFLAGS_COMPILE= -frandom-seed=8cnrgjjflj
NIX_ENFORCE_NO_NATIVE=1
NIX_ENFORCE_PURITY=1
NIX_HARDENING_ENABLE=fortify stackprotector pic strictoverflow format relro bindnow
NIX_INDENT_MAKE=1
NIX_LDFLAGS=-rpath /nix/store/8cnrgjjflj3dyppz299w50l9yydgnqkp-x/lib64 -rpath /nix/store/8cnrgjjflj3dyppz299w50l9yydgnqkp-x/lib 
NIX_LOG_FD=2
NIX_SSL_CERT_FILE=/no-cert-file.crt
NIX_STORE=/nix/store

[pennae]

if we do patch ninja we should do it in a form that can be upstreamed, looking at nix-specific environment variables doesn't sound like that would fit the case. maybe it's as simple as emitting line clearing only when outputting to a tty (which the builders do not)?

[milahu]

if we do patch ninja we should do it in a form that can be upstreamed

why?

nix-build -E 'with import <nixpkgs> {}; pkgs.stdenv.mkDerivation { name = "x"; unpackPhase = "if [ -t 1 ]; then echo term; else echo pipe; fi; exit 1"; }'

says term

and same with [ -t 1 ] | cat says pipe

feel free to post a better idea than getenv("NIX_STORE") ; )

[pennae]

why?

because carrying nix-specific patches to a build system just to make output a little prettier has a pretty bad cost/benefit ratio. this looks benign, but as soon as someone depends ninja doing what it does we'd have to patch that as well. (unlikely, but possible!)

says term

fd 0 is /dev/null though, so [ -t 0 ] says pipe as well. tty checks fd 0 to determine whether something is running on a tty or not as well, which is where our initial assertion came from. ninja checks fd 1 as you noted, a case could be made that it should check 0 instead?

in any case though: patching ninja for slight cosmetic changes is not the solution. we'd be better off patching nix to occasionally flush a line to the log reader once a certain buffer size limit is exceeded.

[milahu]

fd 0 is /dev/null though, so [ -t 0 ] says pipe as well

aah!

patch sent, fingers crossed : )

[milahu]

also when building ninja

building
bootstrapping ninja...
wrote build.ninja.
bootstrap complete.  rebuilding...
[36/36] LINK ninjainja.a.oKKx.o
installing

the 36 lines are line-cleared into one line

[milahu]

upstream said no, so lets just stick with TERM=dumb ninja

as soon as someone depends ninja doing what it does we'd have to patch that as well. (unlikely, but possible!)

yes. its not a bug, its a feature

when people call ninja directly in buildPhase, they will have to use TERM=dumb ninja too