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