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 <[email protected]> wrote: > Steve Sprang <[email protected]> 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 <[email protected]> > > 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’. >
From c8a04ac71e347de49a2795c4ef503a5abac2f882 Mon Sep 17 00:00:00 2001 From: Steve Sprang <[email protected]> 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
