unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
* [bug#62056] [PATCH] guix: Only issue erase-current-line sequence for ttys.
@ 2023-03-08 15:55 Bruno Victal
  2023-03-16 21:30 ` Ludovic Courtès
  2024-06-02 20:19 ` [bug#62056] Custom progress reporter for scripts/substitute.scm Richard Sent
  0 siblings, 2 replies; 6+ messages in thread
From: Bruno Victal @ 2023-03-08 15:55 UTC (permalink / raw)
  To: 62056; +Cc: Bruno Victal

* 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(-)

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





^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [bug#62056] [PATCH] guix: Only issue erase-current-line sequence for ttys.
  2023-03-08 15:55 [bug#62056] [PATCH] guix: Only issue erase-current-line sequence for ttys Bruno Victal
@ 2023-03-16 21:30 ` Ludovic Courtès
  2023-03-18 11:27   ` Bruno Victal
  2024-06-02 20:19 ` [bug#62056] Custom progress reporter for scripts/substitute.scm Richard Sent
  1 sibling, 1 reply; 6+ messages in thread
From: Ludovic Courtès @ 2023-03-16 21:30 UTC (permalink / raw)
  To: Bruno Victal; +Cc: 62056

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’.




^ permalink raw reply	[flat|nested] 6+ messages in thread

* [bug#62056] [PATCH] guix: Only issue erase-current-line sequence for ttys.
  2023-03-16 21:30 ` Ludovic Courtès
@ 2023-03-18 11:27   ` Bruno Victal
  2023-04-18 20:02     ` Ludovic Courtès
  0 siblings, 1 reply; 6+ messages in thread
From: Bruno Victal @ 2023-03-18 11:27 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 62056

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




^ permalink raw reply	[flat|nested] 6+ messages in thread

* [bug#62056] [PATCH] guix: Only issue erase-current-line sequence for ttys.
  2023-03-18 11:27   ` Bruno Victal
@ 2023-04-18 20:02     ` Ludovic Courtès
  2023-05-28  9:54       ` Bruno Victal
  0 siblings, 1 reply; 6+ messages in thread
From: Ludovic Courtès @ 2023-04-18 20:02 UTC (permalink / raw)
  To: Bruno Victal; +Cc: 62056

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’.




^ permalink raw reply	[flat|nested] 6+ messages in thread

* [bug#62056] [PATCH] guix: Only issue erase-current-line sequence for ttys.
  2023-04-18 20:02     ` Ludovic Courtès
@ 2023-05-28  9:54       ` Bruno Victal
  0 siblings, 0 replies; 6+ messages in thread
From: Bruno Victal @ 2023-05-28  9:54 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 62056

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?

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

Cheers,
Bruno.




^ permalink raw reply	[flat|nested] 6+ messages in thread

* [bug#62056] Custom progress reporter for scripts/substitute.scm
  2023-03-08 15:55 [bug#62056] [PATCH] guix: Only issue erase-current-line sequence for ttys Bruno Victal
  2023-03-16 21:30 ` Ludovic Courtès
@ 2024-06-02 20:19 ` Richard Sent
  1 sibling, 0 replies; 6+ messages in thread
From: Richard Sent @ 2024-06-02 20:19 UTC (permalink / raw)
  To: 62056

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:
https://issues.guix.gnu.org/71299

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.




^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-06-03  4:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-08 15:55 [bug#62056] [PATCH] guix: Only issue erase-current-line sequence for ttys Bruno Victal
2023-03-16 21:30 ` Ludovic Courtès
2023-03-18 11:27   ` Bruno Victal
2023-04-18 20:02     ` Ludovic Courtès
2023-05-28  9:54       ` Bruno Victal
2024-06-02 20:19 ` [bug#62056] Custom progress reporter for scripts/substitute.scm Richard Sent

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).