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’.
