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

  • Open
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Bruno Victal
  • Richard Sent
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.
R
R
Richard Sent wrote on 2 Jun 22:19 +0200
Custom progress reporter for scripts/substitute.scm
(address . 62056@debbugs.gnu.org)
87bk4jdrnt.fsf@freakingpenguin.com
I started investigating this issue for the substitutes script
specifically and it's more challenging than a straightforward isatty?
call. I opened an issue for that here:

I agree that modifying erase-current-line isn't the right solution. I
think it's the responsibility of the caller to ensure that the output
port is appropriate.

--
Take it easy,
Richard Sent
Making my computer weirder one commit at a time.
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 62056@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 62056
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch