From ba8ed092989c05c30e2b190bb8a7e94853148757 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Tue, 30 Nov 2021 21:26:22 +0100 Subject: [PATCH] main: always put BUP_FORCE_TTY into the environment Nix reports that progress output from midx/bloom while save is running is no longer printed. The reason turns out to be somewhat complicated: * midx/bloom progress output is printed directly from the C code, if stderr is known to be a tty from isatty(2) or the environment variable BUP_FORCE_TTY * save is a built-in command now, and so are midx/bloom (but the latter is less relevant) * a process inherits the environment that the parent has, unless otherwise specified So with the _old_ setup (with save not a module but process), when save is started, main would set both BUP_FORCE_TTY (and BUP_TTY_WIDTH) in the environment for the subprocess, causing save to obtain it, and further midx/bloom to inherit it from save. One of the downsides of this setup is that the size of the window is now fixed, so resizing doesn't update. With the _new_ setup, where save is a module, we don't set BUP_FORCE_TTY or BUP_TTY_WIDTH in the environment since the redirection and fixing etc. all happens in the main, and the code is directly accessing stdout/stderr via the filtering. The problem now is the following: 1) We create the filter thread, so that stdout/stderr are no longer pointing to the real tty fd, so that isatty() returns false. This is fine for save, or when we start bloom/midx directly, as the _helpers.c istty2 has been evaluated already before we redirect the fds. 2) As described, we don't set the environment variables when we run the save code, since it's a module. However, as a result, when save starts midx/bloom as a new subprocess, they inherit the redirected fds, so that they're not writing to the tty themselves, but also don't get the environment variable BUP_FORCE_TTY since they're started by save and that was a module. The solution then is fairly simple: set both BUP_FORCE_TTY and BUP_TTY_WIDTH in the environment unconditionally. The latter is necessary so that options._tty_width() works in the module-based commands as well. This didn't just affect save -> midx/bloom, but also the ssh calls that pass BUP_FORCE_TTY/BUP_TTY_WIDTH to the remote process, so potentially the server's debug output was also affected. Nix reported that the client debug output was not shown ("Receiving index from server: %d/%d\r"), but I don't have an explanation for that related to this commit, as that code is actually running in the process of the save, thus should be shown depending on helpers.istty2, which was always correct in the main process. Also, I'll note that we now always put BUP_FORCE_TTY into the environment, which we avoided previously for some calls. However, prior to the change to have most things be modules, we were also doing that, since everything we called (git and other tools like par2) would be started by a subprocess, so this doesn't really seem to pose any danger. Reported-by: Nix Fixes: 5dd0172dddb9 ("bup: filter stdout/stderr via thread/pipe for internal subcommands") Reviewed-by: Rob Browning [rlb@defaultvalue.org: remove now unused merge_dict from imports] Signed-off-by: Rob Browning Tested-by: Rob Browning --- lib/bup/main.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/bup/main.py b/lib/bup/main.py index 0c21c88..746fce4 100755 --- a/lib/bup/main.py +++ b/lib/bup/main.py @@ -31,7 +31,6 @@ from bup.helpers import ( columnate, handle_ctrl_c, log, - merge_dict, tty_width ) from bup.git import close_catpipes @@ -198,13 +197,9 @@ fix_stdout = not already_fixed and os.isatty(1) fix_stderr = not already_fixed and os.isatty(2) if fix_stdout or fix_stderr: - tty_env = merge_dict(environ, - {b'BUP_FORCE_TTY': (b'%d' - % ((fix_stdout and 1 or 0) - + (fix_stderr and 2 or 0))), - b'BUP_TTY_WIDTH': b'%d' % _tty_width(), }) -else: - tty_env = environ + _ttymask = (fix_stdout and 1 or 0) + (fix_stderr and 2 or 0) + environ[b'BUP_FORCE_TTY'] = b'%d' % _ttymask + environ[b'BUP_TTY_WIDTH'] = b'%d' % _tty_width() sep_rx = re.compile(br'([\r\n])') @@ -382,7 +377,7 @@ def run_subproc_cmd(args): p = subprocess.Popen(c, stdout=PIPE if fix_stdout else out, stderr=PIPE if fix_stderr else err, - env=tty_env, bufsize=4096, close_fds=True) + bufsize=4096, close_fds=True) # Assume p will receive these signals and quit, which will # then cause us to quit. for sig in (signal.SIGINT, signal.SIGTERM, signal.SIGQUIT): -- 2.39.2