[PATCH] guix: Only issue erase-current-line sequence for ttys.

  • Open
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Bruno Victal
Owner
unassigned
Submitted by
Bruno Victal
Severity
normal
B
B
Bruno Victal wrote on 8 Mar 2023 16:55
(address . guix-patches@gnu.org)(name . Bruno Victal)(address . mirai@makinata.eu)
6a78ee837fe579d6b1beaeda54beddba3c05fd2e.1678290800.git.mirai@makinata.eu
* 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.

guix/progress.scm | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

Toggle diff (32 lines)
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

base-commit: 85d4e8af5baf9b0a9cadf95d802674d0254433da
--
2.39.1
L
L
Ludovic Courtès wrote on 16 Mar 2023 22:30
(name . Bruno Victal)(address . mirai@makinata.eu)(address . 62056@debbugs.gnu.org)
874jqkbd0h.fsf@gnu.org
Hi,

Bruno Victal <mirai@makinata.eu> skribis:

Toggle quote (5 lines)
> * 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!

Toggle quote (11 lines)
> (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’.
B
B
Bruno Victal wrote on 18 Mar 2023 12:27
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 62056@debbugs.gnu.org)
99afbc21-920c-1867-e368-d541ae524283@makinata.eu
Hi Ludo’,

On 2023-03-16 21:30, Ludovic Courtès wrote:
Toggle quote (19 lines)
> 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?)


Toggle quote (2 lines)
> Could you see which use of ‘erase-current-line’ is causing problems?

guix/scripts/substitute.scm:318


Cheers,
Bruno
L
L
Ludovic Courtès wrote on 18 Apr 2023 22:02
(name . Bruno Victal)(address . mirai@makinata.eu)(address . 62056@debbugs.gnu.org)
877cu9hscn.fsf_-_@gnu.org
Hi Bruno,

Bruno Victal <mirai@makinata.eu> skribis:

Toggle quote (24 lines)
> 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.

Toggle quote (3 lines)
> 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.

Toggle quote (4 lines)
>> 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’.
B
B
Bruno Victal wrote on 28 May 2023 11:54
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 62056@debbugs.gnu.org)
7e2399a1-e073-cf11-612d-b130ec71fa54@makinata.eu
Hi Ludo’,

On 2023-04-18 21:02, Ludovic Courtès wrote:
Toggle quote (9 lines)
> 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?

--
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.
?