[PATCH] guix: Properly compute progress bar width.

  • Done
  • quality assurance status badge
Details
4 participants
  • chris
  • Julien Lepiller
  • Ludovic Courtès
  • Ricardo Wurmus
Owner
unassigned
Submitted by
Julien Lepiller
Severity
normal
Blocked by
J
J
Julien Lepiller wrote on 26 Aug 2023 08:36
(address . guix-patches@gnu.org)
20230826063655.2227-1-julien@lepiller.eu
* 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(-)

Toggle diff (73 lines)
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
L
L
Ludovic Courtès wrote on 9 Sep 2023 15:48
(name . Julien Lepiller)(address . julien@lepiller.eu)(address . 65546@debbugs.gnu.org)
87wmwzwj88.fsf@gnu.org
Hi Julien,

Julien Lepiller <julien@lepiller.eu> skribis:

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

[...]

Toggle quote (22 lines)
> +(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’.
J
J
Julien Lepiller wrote on 9 Sep 2023 19:20
[PATCH v2] guix: Properly compute progress bar width.
(address . 65546@debbugs.gnu.org)
20230909172047.6254-1-julien@lepiller.eu
* 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(-)

Toggle diff (109 lines)
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
R
R
Ricardo Wurmus wrote on 26 Sep 2023 10:40
[PATCH] guix: Properly compute progress bar width.
(address . 65546@debbugs.gnu.org)
87bkdp497o.fsf@elephly.net
Hi Julien,

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

--
Ricardo
C
C
chris wrote on 27 Sep 2023 06:02
I have this page bookmarked
(address . 65546@debbugs.gnu.org)(address . chris@bumblehead.com)
ZROpWBQ6PYgc7E0c@guix-xps
This page is bookmarked here and when this patch is applied it will be a happy thing :)
C
C
chris wrote on 11 Oct 2023 22:04
please apply this patch :)
(address . 65546@debbugs.gnu.org)
ZSb_vnfMKPgnimdV@guix-xps
please apply this patch :)
L
L
Ludovic Courtès wrote on 11 Oct 2023 23:00
Re: [bug#65546] [PATCH v2] guix: Properly compute progress bar width.
(name . Julien Lepiller)(address . julien@lepiller.eu)
8734ygswmg.fsf@gnu.org
Hi Julien,

Julien Lepiller <julien@lepiller.eu> skribis:

Toggle quote (6 lines)
> * 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 :-)).

Toggle quote (14 lines)
> +(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:


Time to create a branch?

Ludo’.
L
L
Ludovic Courtès wrote on 13 Oct 2023 17:53
control message for bug #65546
(address . control@debbugs.gnu.org)
87sf6eikmo.fsf@gnu.org
block 65546 by 66525
quit
C
C
chris wrote on 29 Oct 2023 04:50
[PATCH] guix: Properly compute progress bar width.
(address . 65546@debbugs.gnu.org)(address . chris@bumblehead.com)
ZT3WcqJ1xAUP4jr3@guix-xps
Please merge this patch :)
C
C
chris wrote on 9 Nov 2023 11:54
(address . 65546@debbugs.gnu.org)(address . chris@bumblehead.com)
ZUy6bPts1dfObL53@guix-xps
On 10?28? ?, chris wrote:
Toggle quote (2 lines)
> Please merge this patch :)

Please apply this patch! :)
J
J
Julien Lepiller wrote on 11 Nov 2023 11:11
Re: [bug#65546] [PATCH v2] guix: Properly compute progress bar width.
(name . Ludovic Courtès)(address . ludo@gnu.org)
20231111111106.790e92b7@lepiller.eu
Le Wed, 11 Oct 2023 23:00:07 +0200,
Ludovic Courtès <ludo@gnu.org> a écrit :

Toggle quote (52 lines)
> Hi Julien,
>
> Julien Lepiller <julien@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!
Closed
?