| Message ID | 6a78ee837fe579d6b1beaeda54beddba3c05fd2e.1678290800.git.mirai@makinata.eu |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#62056] guix: Only issue erase-current-line sequence for ttys. | expand |
Hi, Bruno Victal <mirai@makinata.eu> skribis: > * guix/progress.scm (erase-current-line): Only issue erase-current-line sequence for ttys. > --- > > Avoids cluttering log lines with �[K when output is logged to a file. +1! > (define (erase-current-line port) > - "Write an ANSI erase-current-line sequence to PORT to erase the whole line and > -move the cursor to the beginning of the line." > - (display "\r\x1b[K" port)) > + "When @var{port} is interactive, write an ANSI erase-current-line sequence > +to erase the whole line and move the cursor to the beginning of the line, > +otherwise write a newline." > + (if (isatty? port) > + (display "\r\x1b[K" port) > + (newline port))) We should avoid calling ‘isatty?’ every time, it’s too costly, which is why there’s also ‘isatty?*’ somewhere that memoizes things. However, it seems up to the caller to check that before calling ‘erase-current-line’. That seems to be the case within progress.scm and in (guix status). Could you see which use of ‘erase-current-line’ is causing problems? TIA, Ludo’.
Hi Ludo’, On 2023-03-16 21:30, Ludovic Courtès wrote: > Bruno Victal <mirai@makinata.eu> skribis: >> (define (erase-current-line port) >> - "Write an ANSI erase-current-line sequence to PORT to erase the whole line and >> -move the cursor to the beginning of the line." >> - (display "\r\x1b[K" port)) >> + "When @var{port} is interactive, write an ANSI erase-current-line sequence >> +to erase the whole line and move the cursor to the beginning of the line, >> +otherwise write a newline." >> + (if (isatty? port) >> + (display "\r\x1b[K" port) >> + (newline port))) > > We should avoid calling ‘isatty?’ every time, it’s too costly, which is > why there’s also ‘isatty?*’ somewhere that memoizes things. > > However, it seems up to the caller to check that before calling > ‘erase-current-line’. That seems to be the case within progress.scm and > in (guix status). guix/status.scm:471 defines a erase-current-line* which calls isatty?*. Does this mean that erase-current-line has to be “wrapped” every time we want it to have tty awareness? If that's not the case, perhaps we could change the signature of erase-current-line to: (define* (erase-current-line port #:optional tty?) > Could you see which use of ‘erase-current-line’ is causing problems? guix/scripts/substitute.scm:318 Cheers, Bruno
Hi Bruno, Bruno Victal <mirai@makinata.eu> skribis: > On 2023-03-16 21:30, Ludovic Courtès wrote: >> Bruno Victal <mirai@makinata.eu> skribis: >>> (define (erase-current-line port) >>> - "Write an ANSI erase-current-line sequence to PORT to erase the whole line and >>> -move the cursor to the beginning of the line." >>> - (display "\r\x1b[K" port)) >>> + "When @var{port} is interactive, write an ANSI erase-current-line sequence >>> +to erase the whole line and move the cursor to the beginning of the line, >>> +otherwise write a newline." >>> + (if (isatty? port) >>> + (display "\r\x1b[K" port) >>> + (newline port))) >> >> We should avoid calling ‘isatty?’ every time, it’s too costly, which is >> why there’s also ‘isatty?*’ somewhere that memoizes things. >> >> However, it seems up to the caller to check that before calling >> ‘erase-current-line’. That seems to be the case within progress.scm and >> in (guix status). > > guix/status.scm:471 defines a erase-current-line* which calls isatty?*. > Does this mean that erase-current-line has to be “wrapped” every time > we want it to have tty awareness? ‘erase-current-line’ is low-level and often the caller has already done an ‘isatty?’ check before calling it (for instance in progress bars). I think that’s the reason it doesn’t include that check. > If that's not the case, perhaps we could change the signature of erase-current-line to: > (define* (erase-current-line port #:optional tty?) I don’t think so. >> Could you see which use of ‘erase-current-line’ is causing problems? > > guix/scripts/substitute.scm:318 In this particular case, how about returning a different <progress-reporter> depending on ‘isatty?’? Thanks, Ludo’.
Hi Ludo’, On 2023-04-18 21:02, Ludovic Courtès wrote: > Bruno Victal <mirai@makinata.eu> skribis: >> On 2023-03-16 21:30, Ludovic Courtès wrote: >>> Could you see which use of ‘erase-current-line’ is causing problems? >> >> guix/scripts/substitute.scm:318 > > In this particular case, how about returning a different > <progress-reporter> depending on ‘isatty?’? It looks like the problem was not just at that particular line, rather anywhere that happens to call (erase-current-line …) which in addition to guix/scripts/substitute.scm are guix/status.scm and guix/progress.scm. Perhaps every instance of <progress-reporter> under guix/ should be checking whether or not they are running under a tty? WDYT?
diff --git a/guix/progress.scm b/guix/progress.scm index 33cf6f4a1a..a1cdd25557 100644 --- a/guix/progress.scm +++ b/guix/progress.scm @@ -3,6 +3,7 @@ ;;; Copyright © 2015 Steve Sprang <scs@stevesprang.com> ;;; Copyright © 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org> ;;; Copyright © 2018 Clément Lassieur <clement@lassieur.org> +;;; Copyright © 2023 Bruno Victal <mirai@makinata.eu> ;;; ;;; This file is part of GNU Guix. ;;; @@ -209,9 +210,12 @@ (define* (progress-bar % #:optional (bar-width 20)) (string (progress-bar-style-stop bar-style))))) (define (erase-current-line port) - "Write an ANSI erase-current-line sequence to PORT to erase the whole line and -move the cursor to the beginning of the line." - (display "\r\x1b[K" port)) + "When @var{port} is interactive, write an ANSI erase-current-line sequence +to erase the whole line and move the cursor to the beginning of the line, +otherwise write a newline." + (if (isatty? port) + (display "\r\x1b[K" port) + (newline port))) (define* (display-download-progress file size #:key