[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
?
Your comment

This issue is archived.

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

To respond to this issue using the mumi CLI, first switch to it
mumi current 65546
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