GNU bug report logs - #65546
[PATCH] guix: Properly compute progress bar width.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 65546 in the body.
You can then email your comments to 65546 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: guix-patches <at> gnu.org
Subject: [PATCH] guix: Properly compute progress bar width.
Date: Sat, 26 Aug 2023 08:36:55 +0200

* guix/progress.scm (terminal-width): New procedure.
(progress-reporter/bar): Use it to compute progress bar width.
* guix/git.scm (show-progress): Use it to compute progress bar width.
---
 guix/git.scm      |  2 +-
 guix/progress.scm | 26 +++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/guix/git.scm b/guix/git.scm
index dbc3b7caa7..870045cddf 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -153,7 +153,7 @@ (define %
   ;; TODO: Both should be handled & exposed by the PROGRESS-BAR API instead.
   (define width
     (max (- (current-terminal-columns)
-            (string-length label) 7)
+            (terminal-width label) 7)
          3))
 
   (define grain
diff --git a/guix/progress.scm b/guix/progress.scm
index 33cf6f4a1a..8a84bcd1b0 100644
--- a/guix/progress.scm
+++ b/guix/progress.scm
@@ -24,6 +24,7 @@ (define-module (guix progress)
   #:use-module (srfi srfi-19)
   #:use-module (rnrs io ports)
   #:use-module (rnrs bytevectors)
+  #:use-module (system foreign)
   #:use-module (ice-9 format)
   #:use-module (ice-9 match)
   #:export (<progress-reporter>
@@ -47,6 +48,7 @@ (define-module (guix progress)
             progress-bar
             byte-count->string
             current-terminal-columns
+            terminal-width
 
             dump-port*))
 
@@ -166,6 +168,28 @@ (define current-terminal-columns
   ;; Number of columns of the terminal.
   (make-parameter 80))
 
+(define (terminal-width str)
+  "Return the width of a string as it would be printed on the terminal.  This
+procedure accounts for characters that have a different width than 1, such as
+CJK double-width characters."
+  (define get-wchar-ffi
+    (pointer->procedure int
+                        (dynamic-func "mbstowcs" (dynamic-link))
+                        (list '* '* size_t)))
+  (define terminal-width-ffi
+    (pointer->procedure int
+                        (dynamic-func "wcswidth" (dynamic-link))
+                        (list '* size_t)))
+  (define (get-wchar str)
+    (let ((wchar (make-bytevector (* (+ (string-length str) 1) 4))))
+      (get-wchar-ffi (bytevector->pointer wchar)
+                     (string->pointer str)
+                     (string-length str))
+      wchar))
+  (terminal-width-ffi
+    (bytevector->pointer (get-wchar str))
+    (string-length str)))
+
 (define-record-type* <progress-bar-style>
   progress-bar-style make-progress-bar-style progress-bar-style?
   (start  progress-bar-style-start)
@@ -307,7 +331,7 @@ (define (draw-bar)
       (if (string-null? prefix)
           (display (progress-bar ratio (current-terminal-columns)) port)
           (let ((width (- (current-terminal-columns)
-                          (string-length prefix) 3)))
+                          (terminal-width prefix) 3)))
             (display prefix port)
             (display "  " port)
             (display (progress-bar ratio width) port)))
-- 
2.41.0

Message #8 received at 65546 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 65546 <at> debbugs.gnu.org
Subject: Re: bug#65546: [PATCH] guix: Properly compute progress bar width.
Date: Sat, 09 Sep 2023 15:48:23 +0200

Hi Julien,

Julien Lepiller <julien <at> lepiller.eu> skribis:

> * guix/progress.scm (terminal-width): New procedure.
> (progress-reporter/bar): Use it to compute progress bar width.
> * guix/git.scm (show-progress): Use it to compute progress bar width.

[...]

> +(define (terminal-width str)
> +  "Return the width of a string as it would be printed on the terminal.  This
> +procedure accounts for characters that have a different width than 1, such as
> +CJK double-width characters."
> +  (define get-wchar-ffi
> +    (pointer->procedure int
> +                        (dynamic-func "mbstowcs" (dynamic-link))
> +                        (list '* '* size_t)))
> +  (define terminal-width-ffi
> +    (pointer->procedure int
> +                        (dynamic-func "wcswidth" (dynamic-link))
> +                        (list '* size_t)))
> +  (define (get-wchar str)
> +    (let ((wchar (make-bytevector (* (+ (string-length str) 1) 4))))
> +      (get-wchar-ffi (bytevector->pointer wchar)
> +                     (string->pointer str)
> +                     (string-length str))
> +      wchar))
> +  (terminal-width-ffi
> +    (bytevector->pointer (get-wchar str))
> +    (string-length str)))

Neat!  However, could you move it to (guix build syscalls), next to
‘terminal-columns’ (making sure ‘pointer->procedure’ is called only once
rather than once per call), and with a test or two in
‘tests/syscalls.scm’?

For clarity, perhaps rename it to ‘terminal-string-width’?

TIA!

Ludo’.

Message #11 received at 65546 <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: 65546 <at> debbugs.gnu.org
Subject: [PATCH v2] guix: Properly compute progress bar width.
Date: Sat,  9 Sep 2023 19:20:42 +0200

* guix/build/syscalls.scm (terminal-width): New procedure.
* guix/progress.scm (progress-reporter/bar): Use it to compute progress
bar width.
* guix/git.scm (show-progress): Use it to compute progress bar width.
* tests/syscalls.scm: Add tests.
---
 guix/build/syscalls.scm | 24 ++++++++++++++++++++++++
 guix/git.scm            |  4 +++-
 guix/progress.scm       |  5 ++++-
 tests/syscalls.scm      |  6 ++++++
 4 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/guix/build/syscalls.scm b/guix/build/syscalls.scm
index d947b010d3..a1365cc68c 100644
--- a/guix/build/syscalls.scm
+++ b/guix/build/syscalls.scm
@@ -192,6 +192,7 @@ (define-module (guix build syscalls)
             terminal-window-size
             terminal-columns
             terminal-rows
+            terminal-string-width
             openpty
             login-tty
 
@@ -2335,6 +2336,29 @@ (define* (terminal-rows #:optional (port (current-output-port)))
 always a positive integer."
   (terminal-dimension window-size-rows port (const 25)))
 
+(define get-wchar-ffi
+  (pointer->procedure int
+                      (dynamic-func "mbstowcs" (dynamic-link))
+                      (list '* '* size_t)))
+(define terminal-string-width-ffi
+  (pointer->procedure int
+                      (dynamic-func "wcswidth" (dynamic-link))
+                      (list '* size_t)))
+
+(define (terminal-string-width str)
+  "Return the width of a string as it would be printed on the terminal.  This
+procedure accounts for characters that have a different width than 1, such as
+CJK double-width characters."
+  (define (get-wchar str)
+    (let ((wchar (make-bytevector (* (+ (string-length str) 1) 4))))
+      (get-wchar-ffi (bytevector->pointer wchar)
+                     (string->pointer str)
+                     (string-length str))
+      wchar))
+  (terminal-string-width-ffi
+    (bytevector->pointer (get-wchar str))
+    (string-length str)))
+
 (define openpty
   (let ((proc (syscall->procedure int "openpty" '(* * * * *)
                                   #:library "libutil")))
diff --git a/guix/git.scm b/guix/git.scm
index 1cb87a4560..728b761e62 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -29,6 +29,8 @@ (define-module (guix git)
   #:use-module (gcrypt hash)
   #:use-module ((guix build utils)
                 #:select (mkdir-p delete-file-recursively))
+  #:use-module ((guix build syscalls)
+                #:select (terminal-string-width))
   #:use-module (guix store)
   #:use-module (guix utils)
   #:use-module (guix records)
@@ -153,7 +155,7 @@ (define %
   ;; TODO: Both should be handled & exposed by the PROGRESS-BAR API instead.
   (define width
     (max (- (current-terminal-columns)
-            (string-length label) 7)
+            (terminal-string-width label) 7)
          3))
 
   (define grain
diff --git a/guix/progress.scm b/guix/progress.scm
index 33cf6f4a1a..e7cf7e168a 100644
--- a/guix/progress.scm
+++ b/guix/progress.scm
@@ -21,9 +21,12 @@
 
 (define-module (guix progress)
   #:use-module (guix records)
+  #:use-module ((guix build syscalls)
+                #:select (terminal-string-width))
   #:use-module (srfi srfi-19)
   #:use-module (rnrs io ports)
   #:use-module (rnrs bytevectors)
+  #:use-module (system foreign)
   #:use-module (ice-9 format)
   #:use-module (ice-9 match)
   #:export (<progress-reporter>
@@ -307,7 +310,7 @@ (define (draw-bar)
       (if (string-null? prefix)
           (display (progress-bar ratio (current-terminal-columns)) port)
           (let ((width (- (current-terminal-columns)
-                          (string-length prefix) 3)))
+                          (terminal-string-width prefix) 3)))
             (display prefix port)
             (display "  " port)
             (display (progress-bar ratio width) port)))
diff --git a/tests/syscalls.scm b/tests/syscalls.scm
index c9e011f453..eb85b358c4 100644
--- a/tests/syscalls.scm
+++ b/tests/syscalls.scm
@@ -583,6 +583,12 @@ (define perform-container-tests?
 (test-assert "terminal-rows"
   (> (terminal-rows) 0))
 
+(test-assert "terminal-string-width English"
+  (= (terminal-string-width "hello") 5))
+
+(test-assert "terminal-string-width Japanese"
+  (= (terminal-string-width "今日は") 6))
+
 (test-assert "openpty"
   (let ((head inferior (openpty)))
     (and (integer? head) (integer? inferior)
-- 
2.41.0

Message #14 received at 65546 <at> debbugs.gnu.org (full text, mbox):

From: Ricardo Wurmus <rekado <at> elephly.net>
To: 65546 <at> debbugs.gnu.org
Subject: [PATCH] guix: Properly compute progress bar width.
Date: Tue, 26 Sep 2023 10:40:56 +0200

Hi Julien,

this v2 of your patch looks good to me.  Please push!

-- 
Ricardo

Message #17 received at 65546 <at> debbugs.gnu.org (full text, mbox):

From: chris <chris <at> bumblehead.com>
To: 65546 <at> debbugs.gnu.org
Cc: chris <at> bumblehead.com
Subject: I have this page bookmarked
Date: Tue, 26 Sep 2023 21:02:32 -0700

This page is bookmarked here and when this patch is applied it will be a happy thing :)

Message #20 received at 65546 <at> debbugs.gnu.org (full text, mbox):

From: chris <chris <at> bumblehead.com>
To: 65546 <at> debbugs.gnu.org
Subject: please apply this patch :)
Date: Wed, 11 Oct 2023 13:04:14 -0700

please apply this patch :)

Message #23 received at 65546 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: Ricardo Wurmus <rekado <at> elephly.net>, 65546 <at> debbugs.gnu.org
Subject: Re: [bug#65546] [PATCH v2] guix: Properly compute progress bar width.
Date: Wed, 11 Oct 2023 23:00:07 +0200

Hi Julien,

Julien Lepiller <julien <at> lepiller.eu> skribis:

> * guix/build/syscalls.scm (terminal-width): New procedure.
> * guix/progress.scm (progress-reporter/bar): Use it to compute progress
> bar width.
> * guix/git.scm (show-progress): Use it to compute progress bar width.
> * tests/syscalls.scm: Add tests.

Others have already said “LGTM”, and I concur.  I’ll still make a couple
of minor suggestions but that shoudln’t stop you from going ahead
(everyone’s waiting for it :-)).

> +(define get-wchar-ffi
> +  (pointer->procedure int
> +                      (dynamic-func "mbstowcs" (dynamic-link))
> +                      (list '* '* size_t)))
> +(define terminal-string-width-ffi
> +  (pointer->procedure int
> +                      (dynamic-func "wcswidth" (dynamic-link))
> +                      (list '* size_t)))
> +
> +(define (terminal-string-width str)
> +  "Return the width of a string as it would be printed on the terminal.  This
> +procedure accounts for characters that have a different width than 1, such as
> +CJK double-width characters."

I’d suggest following the style of the rest of the file, which is to do
something like:

(define terminal-string-width
  (let ((mbstowcs (syscall->procedure …))
        (wcswidth (syscall->procedure …)))
    (lambda (str)
      …)))

Ideally the syscalls.scm changes would be in a commit separate from the
progress.scm changes.

Now we have the problem that OpenJDK unfortunately depends on (guix
build syscalls), which makes this change half-of-the-world-rebuild.
There’s another pending syscalls.scm change:

  https://issues.guix.gnu.org/66055
  https://issues.guix.gnu.org/66054

Time to create a branch?

Ludo’.

Message #28 received at 65546 <at> debbugs.gnu.org (full text, mbox):

From: chris <chris <at> bumblehead.com>
To: 65546 <at> debbugs.gnu.org
Cc: chris <at> bumblehead.com
Subject: [PATCH] guix: Properly compute progress bar width.
Date: Sat, 28 Oct 2023 20:50:10 -0700

Please merge this patch :)

Message #31 received at 65546 <at> debbugs.gnu.org (full text, mbox):

From: chris <chris <at> bumblehead.com>
To: 65546 <at> debbugs.gnu.org
Cc: chris <at> bumblehead.com
Subject: Re: [PATCH] guix: Properly compute progress bar width.
Date: Thu, 9 Nov 2023 02:54:36 -0800

On 10月28日 土, chris wrote:
> Please merge this patch :)

Please apply this patch! :)

Message #36 received at 65546-done <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Ricardo Wurmus <rekado <at> elephly.net>, 65546-done <at> debbugs.gnu.org
Subject: Re: [bug#65546] [PATCH v2] guix: Properly compute progress bar width.
Date: Sat, 11 Nov 2023 11:11:06 +0100

Le Wed, 11 Oct 2023 23:00:07 +0200,
Ludovic Courtès <ludo <at> gnu.org> a écrit :

> Hi Julien,
> 
> Julien Lepiller <julien <at> lepiller.eu> skribis:
> 
> > * guix/build/syscalls.scm (terminal-width): New procedure.
> > * guix/progress.scm (progress-reporter/bar): Use it to compute
> > progress bar width.
> > * guix/git.scm (show-progress): Use it to compute progress bar
> > width.
> > * tests/syscalls.scm: Add tests.  
> 
> Others have already said “LGTM”, and I concur.  I’ll still make a
> couple of minor suggestions but that shoudln’t stop you from going
> ahead (everyone’s waiting for it :-)).
> 
> > +(define get-wchar-ffi
> > +  (pointer->procedure int
> > +                      (dynamic-func "mbstowcs" (dynamic-link))
> > +                      (list '* '* size_t)))
> > +(define terminal-string-width-ffi
> > +  (pointer->procedure int
> > +                      (dynamic-func "wcswidth" (dynamic-link))
> > +                      (list '* size_t)))
> > +
> > +(define (terminal-string-width str)
> > +  "Return the width of a string as it would be printed on the
> > terminal.  This +procedure accounts for characters that have a
> > different width than 1, such as +CJK double-width characters."  
> 
> I’d suggest following the style of the rest of the file, which is to
> do something like:
> 
> (define terminal-string-width
>   (let ((mbstowcs (syscall->procedure …))
>         (wcswidth (syscall->procedure …)))
>     (lambda (str)
>       …)))
> 
> Ideally the syscalls.scm changes would be in a commit separate from
> the progress.scm changes.
> 
> Now we have the problem that OpenJDK unfortunately depends on (guix
> build syscalls), which makes this change half-of-the-world-rebuild.
> There’s another pending syscalls.scm change:
> 
>   https://issues.guix.gnu.org/66055
>   https://issues.guix.gnu.org/66054
> 
> Time to create a branch?
> 
> Ludo’.

It seems OpenJDK and other packages don't depend on syscalls anymore
(and the blocking bug was marked as done), so pushed to master as
28ca80717da67f90a3b33491e9807a053cf09c2d. I tried to follow your advice
and split the patch in two. Thanks!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.