From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: [PATCH] substitute: Improve readability of substitute-related output. Date: Tue, 15 Sep 2015 10:22:55 +0200 Message-ID: <87si6gdrc0.fsf@gnu.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:56149) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZblW3-0001Px-Kc for guix-devel@gnu.org; Tue, 15 Sep 2015 04:23:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZblVy-0003HX-Dk for guix-devel@gnu.org; Tue, 15 Sep 2015 04:23:03 -0400 In-Reply-To: (Steve Sprang's message of "Mon, 14 Sep 2015 22:55:49 -0700") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Steve Sprang Cc: guix-devel@gnu.org Steve Sprang 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=E2=80=A6boost-1.58.0 (110.6MiB installed)... > sin6c3=E2=80=A6boost-1.58.0 911KiB/s 00:00:11 | 9.4= MiB > transferred Looks good to me. Regarding elapsed time, I wonder if we should get rid of hours=E2=80=93i.e., print =E2=80=9C00:11=E2=80=9D in this case. Most of the time downloads las= t less than an hour, and when it lasts more, it=E2=80=99s OK to either print 100:11 or 01:03:11. WDYT? > From 95936bf25394d2985f9331cb8fa08d5b30cb64a5 Mon Sep 17 00:00:00 2001 > From: Steve Sprang > Date: Mon, 14 Sep 2015 22:31:11 -0700 > Subject: [PATCH] substitute: Improve readability of substitute-related ou= tput. > > * 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 out= put. > (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 =E2=80=A6) modules must not use (gui= x =E2=80=A6) 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 =E2=80=98progress-proc=E2=80=99 et a= l. > +(define* (flexible-space left right #:optional (columns 80)) > + "Return a string of spaces which can be used to separate LEFT and RIGH= T 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 =E2=80=98store-url-abbreviation=E2=80=99, to convey t= hat it=E2=80=99s similar in spirit to =E2=80=98uri-abbreviation=E2=80=99, but specialized? > (define* (progress-proc file size #:optional (log-port (current-output-p= ort))) > "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 wid= th. > - (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=E2=80=99.