Re: [PATCH] substitute: Improve readability of substitute-related output.

[Steve Sprang <steve.sprang@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4597 bytes --]

Thanks for the feedback! Here's an updated patch. Let me know if I missed
anything.

I agree that we can drop hours from the elapsed time (or adaptively add
them if needed). I will send a fix for that in a subsequent patch.

On Tue, Sep 15, 2015 at 1:22 AM, Ludovic Courtès <ludo@gnu.org> wrote:

> Steve Sprang <steve.sprang@gmail.com> skribis:
>
> > I added newlines to split up output and visually separate multiple
> > substitutions. Since the download size is unknown, we can't use a
> progress
> > bar, but I made the output more consistent with the progress bar
> behavior.
> > I also shortened the hash after it's been repeated in full twice (once
> for
> > the store and once for the download URL).
>
> Nice.
>
> > Patched behavior:
> >
> > Found valid signature for
> > /gnu/store/sin6c3n1440f8kza0k4hdms38fbb4dv0-boost-1.58.0
> > From
> http://hydra.gnu.org/nar/sin6c3n1440f8kza0k4hdms38fbb4dv0-boost-1.58.0
> > Downloading sin6c3…boost-1.58.0 (110.6MiB installed)...
> >  sin6c3…boost-1.58.0                      911KiB/s 00:00:11 | 9.4MiB
> > transferred
>
> Looks good to me.
>
> Regarding elapsed time, I wonder if we should get rid of hours–i.e.,
> print “00:11” in this case.  Most of the time downloads last less than
> an hour, and when it lasts more, it’s OK to either print 100:11 or
> 01:03:11.  WDYT?
>
> > From 95936bf25394d2985f9331cb8fa08d5b30cb64a5 Mon Sep 17 00:00:00 2001
> > From: Steve Sprang <scs@stevesprang.com>
> > Date: Mon, 14 Sep 2015 22:31:11 -0700
> > Subject: [PATCH] substitute: Improve readability of substitute-related
> output.
> >
> > * guix/build/download.scm (flexible-space, truncated-url): New
> procedures.
> >   (progress-proc): Generate a better indeterminate progress string.
> >   (nearest-exact-integer, seconds->string, byte-count->string): Move
> to...
> > * guix/utils.scm: ...here.
> > * guix/store.scm (truncated-store-path): New procedure.
> > * guix/scripts/substitute.scm (assert-valid-narinfo): Add newlines to
> output.
> >   (process-substitution): Use byte-count->string and
> truncated-store-path.
>
> In general, it is best to make a separate patch that simply moves
> procedures from one file to another.
>
> However, most of the time, (guix build …) modules must not use (guix …)
> modules (info "(guix) G-Expressions").  This is the case here.
>
> So what I would suggest is to export those helper procedures from (guix
> build download) itself, as is done for ‘progress-proc’ et al.
>
> > +(define* (flexible-space left right #:optional (columns 80))
> > +  "Return a string of spaces which can be used to separate LEFT and
> RIGHT so
> > +that RIGHT is justified to a width of COLUMNS."
>
> What about this slightly more idiomatic procedure instead:
>
>   (define (string-pad-middle left right len)
>     ;; + docstring
>     (string-append left
>                    (string-pad right (max 0 (- len (string-length
> left))))))
>
> > +(define (truncated-url url)
> > +  "Return a friendlier version of URL for display."
> > +  (let ((store-path (string-append (%store-prefix) "/" (basename url))))
> > +    ;; take advantage of the implementation for store paths
> > +    (truncated-store-path store-path)))
>
> What about calling it ‘store-url-abbreviation’, to convey that it’s
> similar in spirit to ‘uri-abbreviation’, but specialized?
>
> >  (define* (progress-proc file size #:optional (log-port
> (current-output-port)))
> >    "Return a procedure to show the progress of FILE's download, which is
> SIZE
> >  bytes long.  The returned procedure is suitable for use as an argument
> to
> > @@ -130,24 +115,26 @@ bytes long.  The returned procedure is suitable
> for use as an argument to
> >                                           (seconds->string elapsed)
> >                                           (progress-bar %) %))
> >                       ;; TODO: Make this adapt to the actual terminal
> width.
> > -                     (cols       80)
> > -                     (num-spaces (max 1 (- cols (+ (string-length left)
> > -                                                   (string-length
> right)))))
> > -                     (gap        (make-string num-spaces #\space)))
> > +                     (gap        (flexible-space left right)))
> >                  (format log-port "~a~a~a" left gap right)
>
> This would become:
>
>   (display (string-pad-middle left right cols) log-port)
>
> Could you send an updated patch?
>
> Thank you!
>
> Ludo’.
>

[-- Attachment #1.2: Type: text/html, Size: 5722 bytes --]

[-- Attachment #2: prettier-substitute-2.patch --]
[-- Type: text/x-patch, Size: 7794 bytes --]

From c8a04ac71e347de49a2795c4ef503a5abac2f882 Mon Sep 17 00:00:00 2001
From: Steve Sprang <scs@stevesprang.com>
Date: Mon, 14 Sep 2015 22:31:11 -0700
Subject: [PATCH] substitute: Improve readability of substitute-related output.

* guix/build/download.scm
  (string-pad-middle, store-url-abbreviation): New procedures.
  (progress-proc): Generate a better indeterminate progress string.
* guix/store.scm (truncated-store-path): New procedure.
* guix/scripts/substitute.scm (assert-valid-narinfo): Add newlines to output.
  (process-substitution): Use byte-count->string and truncated-store-path.
---
 guix/build/download.scm     | 42 ++++++++++++++++++++++++++++--------------
 guix/scripts/substitute.scm | 13 +++++++------
 guix/store.scm              | 11 ++++++++++-
 3 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/guix/build/download.scm b/guix/build/download.scm
index 31d60fb..0358136 100644
--- a/guix/build/download.scm
+++ b/guix/build/download.scm
@@ -24,6 +24,7 @@
   #:use-module (web response)
   #:use-module (guix ftp-client)
   #:use-module (guix build utils)
+  #:use-module (guix store)
   #:use-module (rnrs io ports)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-11)
@@ -36,6 +37,7 @@
             resolve-uri-reference
             maybe-expand-mirrors
             url-fetch
+            byte-count->string
             progress-proc
             uri-abbreviation))
 
@@ -96,6 +98,18 @@ width of the bar is BAR-WIDTH."
             (make-string filled #\#)
             (make-string empty #\space))))
 
+(define (string-pad-middle left right len)
+    "Combine LEFT and RIGHT with enough padding in the middle so that the
+resulting string has length at least LEN.  This right justifies RIGHT."
+    (string-append left
+                   (string-pad right (max 0 (- len (string-length left))))))
+
+(define (store-url-abbreviation url)
+  "Return a friendlier version of URL for display."
+  (let ((store-path (string-append (%store-prefix) "/" (basename url))))
+    ;; take advantage of the implementation for store paths
+    (truncated-store-path store-path)))
+
 (define* (progress-proc file size #:optional (log-port (current-output-port)))
   "Return a procedure to show the progress of FILE's download, which is SIZE
 bytes long.  The returned procedure is suitable for use as an argument to
@@ -128,26 +142,26 @@ bytes long.  The returned procedure is suitable for use as an argument to
                      (right      (format #f "~a/s ~a ~a~6,1f%"
                                          (byte-count->string throughput)
                                          (seconds->string elapsed)
-                                         (progress-bar %) %))
-                     ;; TODO: Make this adapt to the actual terminal width.
-                     (cols       80)
-                     (num-spaces (max 1 (- cols (+ (string-length left)
-                                                   (string-length right)))))
-                     (gap        (make-string num-spaces #\space)))
-                (format log-port "~a~a~a" left gap right)
+                                         (progress-bar %) %)))
+                ;; TODO: Make this adapt to the actual terminal width.
+                (display (string-pad-middle left right 80) log-port)
                 (display #\cr log-port)
                 (flush-output-port log-port)
                 (cont))))
           (lambda (transferred cont)
             (with-elapsed-time elapsed
-              (let ((throughput (if elapsed
-                                    (/ transferred elapsed)
-                                    0)))
+              (let* ((throughput (if elapsed
+                                     (/ transferred elapsed)
+                                     0))
+                     (left       (format #f " ~a"
+                                         (store-url-abbreviation file)))
+                     (right      (format #f "~a/s ~a | ~a transferred"
+                                         (byte-count->string throughput)
+                                         (seconds->string elapsed)
+                                         (byte-count->string transferred))))
+                ;; TODO: Make this adapt to the actual terminal width.
+                (display (string-pad-middle left right 80) log-port)
                 (display #\cr log-port)
-                (format log-port "~a\t~a transferred (~a/s)"
-                        file
-                        (byte-count->string transferred)
-                        (byte-count->string throughput))
                 (flush-output-port log-port)
                 (cont))))))))
 
diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm
index e908bc9..9c6e047 100755
--- a/guix/scripts/substitute.scm
+++ b/guix/scripts/substitute.scm
@@ -29,9 +29,10 @@
   #:use-module (guix base64)
   #:use-module (guix pk-crypto)
   #:use-module (guix pki)
+  #:use-module (guix store)
   #:use-module ((guix build utils) #:select (mkdir-p dump-port))
   #:use-module ((guix build download)
-                #:select (progress-proc uri-abbreviation))
+                #:select (progress-proc uri-abbreviation byte-count->string))
   #:use-module (ice-9 rdelim)
   #:use-module (ice-9 regex)
   #:use-module (ice-9 match)
@@ -337,8 +338,9 @@ or is signed by an unauthorized key."
           (unless %allow-unauthenticated-substitutes?
             (assert-valid-signature narinfo signature hash acl)
             (when verbose?
+              ;; visually separate substitutions with a newline
               (format (current-error-port)
-                      "found valid signature for '~a', from '~a'~%"
+                      "~%Found valid signature for ~a~%From ~a~%"
                       (narinfo-path narinfo)
                       (uri->string (narinfo-uri narinfo)))))
           narinfo))))
@@ -753,13 +755,12 @@ DESTINATION as a nar file.  Verify the substitute against ACL."
     ;; Tell the daemon what the expected hash of the Nar itself is.
     (format #t "~a~%" (narinfo-hash narinfo))
 
-    (format (current-error-port) "downloading `~a'~:[~*~; (~,1f MiB installed)~]...~%"
-            store-item
-
+    (format (current-error-port) "Downloading ~a~:[~*~; (~a installed)~]...~%"
+            (truncated-store-path store-item)
             ;; Use the Nar size as an estimate of the installed size.
             (narinfo-size narinfo)
             (and=> (narinfo-size narinfo)
-                   (cute / <> (expt 2. 20))))
+                   (cute byte-count->string <>)))
     (let*-values (((raw download-size)
                    ;; Note that Hydra currently generates Nars on the fly
                    ;; and doesn't specify a Content-Length, so
diff --git a/guix/store.scm b/guix/store.scm
index 132b8a3..a1b76e3 100644
--- a/guix/store.scm
+++ b/guix/store.scm
@@ -124,7 +124,8 @@
             store-path-package-name
             store-path-hash-part
             direct-store-path
-            log-file))
+            log-file
+            truncated-store-path))
 
 (define %protocol-version #x10c)
 
@@ -1088,3 +1089,11 @@ must be an absolute store file name, or a derivation file name."
             ;; Return the first that works.
             (any (cut log-file store <>) derivers))
            (_ #f)))))
+
+(define* (truncated-store-path store-path #:optional (prefix-length 6))
+  "Return a friendlier version of STORE-PATH for display."
+  (let* ((hash-part    (store-path-hash-part store-path))
+         (package-name (store-path-package-name store-path))
+         (safe-length  (max 0 (min prefix-length 24)))
+         (prefix       (string-take hash-part safe-length)))
+    (string-append prefix "…" package-name)))
-- 
2.5.0