[bug#32634] RFC: Process build output

[bug#32634] RFC: Process build output

[Ricardo Wurmus]

Hi Guix,

this patch set is a first draft to stylize (potentially confusing) build
output when using “guix package” and “guix build”.

This is done by adding a soft port that matches on lines in the build
output and colorizes them (unless INSIDE_EMACS or NO_COLOR are set, or
when output is redirected).  For “guix package” the default behaviour is
to also hide all build output that does not announce progress (unless
“--verbose” is passed) and to let a spinner show progress instead.  For
“guix build” all build output is still printed.

Honestly, I’m not really happy with the results, but I think it’s enough
to start a discussion about where this should lead.

One thing I don’t like is that I had to set the “print-build-trace?”
default option to be able to display what build is currently happening.
Unfortunately, for small derivations this leads to output like this:

--8<---------------cut here---------------start------------->8---
Building /gnu/store/2x5xmvimja0pbkvvr8rym91q0249ajiv-fonts-dir.drv - x86_64-linux
Built /gnu/store/2x5xmvimja0pbkvvr8rym91q0249ajiv-fonts-dir.drv
Building /gnu/store/diz3pmgrqibvp2pyvgh4wyr4nx5vlx0y-glib-schemas.drv - x86_64-linux
Built /gnu/store/diz3pmgrqibvp2pyvgh4wyr4nx5vlx0y-glib-schemas.drv
Building /gnu/store/ss70j6lf8xxiiykdys92iw92khx68ix9-info-dir.drv - x86_64-linux
Built /gnu/store/ss70j6lf8xxiiykdys92iw92khx68ix9-info-dir.drv
Building /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv - x86_64-linux
Built /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv
--8<---------------cut here---------------end--------------->8---

I would prefer:

    Building /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv … DONE

or similar.

I don’t know about whether the colours are any good; I think the bold
green is hard to read on a bright terminal, while the black is hard to
read on a dark terminal.

Lastly: the spinner.  It’s a bit boring, I think.

What do you think?  Is this a step in the right direction?

--
Ricardo

[bug#32634] [PATCH 1/2] ui: Add support for colorization.

[Ricardo Wurmus]

From: Sahithi Yarlagadda <sahi@swecha.net>

* guix/ui.scm (ansi-color-tables): New variable.
(color, colorize-string): New procedures.

Signed-off-by: Ricardo Wurmus <rekado@elephly.net>
---
 guix/ui.scm | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index 29c0b2b9c..f8f2cad69 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -10,6 +10,8 @@
 ;;; Copyright © 2016 Roel Janssen <roel@gnu.org>
 ;;; Copyright © 2016 Benz Schenk <benz.schenk@uzh.ch>
 ;;; Copyright © 2018 Kyle Meyer <kyle@kyleam.com>
+;;; Copyright © 2013, 2014 Free Software Foundation, Inc.
+;;; Copyright © 2018 Sahithi Yarlagadda <sahi@swecha.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -115,7 +117,8 @@
             guix-warning-port
             warning
             info
-            guix-main))
+            guix-main
+            colorize-string))
 
 ;;; Commentary:
 ;;;
@@ -1622,4 +1625,54 @@ and signal handling has already been set up."
   (initialize-guix)
   (apply run-guix args))
 
+(define color-table
+  `((CLEAR       .   "0")
+    (RESET       .   "0")
+    (BOLD        .   "1")
+    (DARK        .   "2")
+    (UNDERLINE   .   "4")
+    (UNDERSCORE  .   "4")
+    (BLINK       .   "5")
+    (REVERSE     .   "6")
+    (CONCEALED   .   "8")
+    (BLACK       .  "30")
+    (RED         .  "31")
+    (GREEN       .  "32")
+    (YELLOW      .  "33")
+    (BLUE        .  "34")
+    (MAGENTA     .  "35")
+    (CYAN        .  "36")
+    (WHITE       .  "37")
+    (ON-BLACK    .  "40")
+    (ON-RED      .  "41")
+    (ON-GREEN    .  "42")
+    (ON-YELLOW   .  "43")
+    (ON-BLUE     .  "44")
+    (ON-MAGENTA  .  "45")
+    (ON-CYAN     .  "46")
+    (ON-WHITE    .  "47")))
+
+(define (color . lst)
+  "Return a string containing the ANSI escape sequence for producing the
+requested set of attributes in LST.  Unknown attributes are ignored."
+  (let ((color-list
+         (remove not
+                 (map (lambda (color) (assq-ref color-table color))
+                      lst))))
+    (if (null? color-list)
+        ""
+        (string-append
+         (string #\esc #\[)
+         (string-join color-list ";" 'infix)
+         "m"))))
+
+(define (colorize-string str . color-list)
+  "Return a copy of STR colorized using ANSI escape sequences according to the
+attributes STR.  At the end of the returned string, the color attributes will
+be reset such that subsequent output will not have any colors in effect."
+  (string-append
+   (apply color color-list)
+   str
+   (color 'RESET)))
+
 ;;; ui.scm ends here
-- 
2.18.0

[bug#32634] [PATCH 2/2] ui: Add soft port for styling and filtering build output.

[Ricardo Wurmus]

* guix/ui.scm (build-output-port): New procedure.
* guix/scripts/package.scm (%default-options): Print build trace.
(guix-package): Use build-output-port.
* guix/scripts/build.scm (guix-build): Use build-output-port.

Co-authored-by: Sahithi Yarlagadda <sahi@swecha.net>
---
 guix/scripts/build.scm   |   2 +-
 guix/scripts/package.scm |  39 ++++++++------
 guix/ui.scm              | 109 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 132 insertions(+), 18 deletions(-)

diff --git a/guix/scripts/build.scm b/guix/scripts/build.scm
index 4dd4fbccd..3fa3c2c20 100644
--- a/guix/scripts/build.scm
+++ b/guix/scripts/build.scm
@@ -735,7 +735,7 @@ needed."
 
         (parameterize ((current-build-output-port (if quiet?
                                                       (%make-void-port "w")
-                                                      (current-error-port))))
+                                                      (build-output-port #:verbose? #t))))
           (let* ((mode  (assoc-ref opts 'build-mode))
                  (drv   (options->derivations store opts))
                  (urls  (map (cut string-append <> "/log")
diff --git a/guix/scripts/package.scm b/guix/scripts/package.scm
index b38a55d01..216b63049 100644
--- a/guix/scripts/package.scm
+++ b/guix/scripts/package.scm
@@ -328,7 +328,8 @@ ENTRIES, a list of manifest entries, in the context of PROFILE."
   `((verbosity . 0)
     (graft? . #t)
     (substitutes? . #t)
-    (build-hook? . #t)))
+    (build-hook? . #t)
+    (print-build-trace? . #t)))
 
 (define (show-help)
   (display (G_ "Usage: guix package [OPTION]...
@@ -883,18 +884,24 @@ processed, #f otherwise."
         (arg-handler arg result)
         (leave (G_ "~A: extraneous argument~%") arg)))
 
-  (let ((opts (parse-command-line args %options (list %default-options #f)
-                                  #:argument-handler handle-argument)))
-    (with-error-handling
-      (or (process-query opts)
-          (parameterize ((%store  (open-connection))
-                         (%graft? (assoc-ref opts 'graft?)))
-            (set-build-options-from-command-line (%store) opts)
-
-            (parameterize ((%guile-for-build
-                            (package-derivation
-                             (%store)
-                             (if (assoc-ref opts 'bootstrap?)
-                                 %bootstrap-guile
-                                 (canonical-package guile-2.2)))))
-              (process-actions (%store) opts)))))))
+  (define opts
+    (parse-command-line args %options (list %default-options #f)
+                        #:argument-handler handle-argument))
+  (define verbose?
+    (assoc-ref opts 'verbose?))
+
+  (with-error-handling
+    (or (process-query opts)
+        (parameterize ((%store  (open-connection))
+                       (%graft? (assoc-ref opts 'graft?)))
+          (set-build-options-from-command-line (%store) opts)
+
+          (parameterize ((%guile-for-build
+                          (package-derivation
+                           (%store)
+                           (if (assoc-ref opts 'bootstrap?)
+                               %bootstrap-guile
+                               (canonical-package guile-2.2))))
+                         (current-build-output-port
+                          (build-output-port #:verbose? verbose?)))
+            (process-actions (%store) opts))))))
diff --git a/guix/ui.scm b/guix/ui.scm
index f8f2cad69..5482d919b 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -12,6 +12,7 @@
 ;;; Copyright © 2018 Kyle Meyer <kyle@kyleam.com>
 ;;; Copyright © 2013, 2014 Free Software Foundation, Inc.
 ;;; Copyright © 2018 Sahithi Yarlagadda <sahi@swecha.net>
+;;; Copyright © 2018 Ricardo Wurmus <rekado@elephly.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -118,7 +119,7 @@
             warning
             info
             guix-main
-            colorize-string))
+            build-output-port))
 
 ;;; Commentary:
 ;;;
@@ -1675,4 +1676,110 @@ be reset such that subsequent output will not have any colors in effect."
    str
    (color 'RESET)))
 
+(define* (build-output-port #:key
+                            (colorize? #t)
+                            verbose?
+                            (port (current-error-port)))
+  "Return a soft port that processes build output.  By default it colorizes
+phase announcements and replaces any other output with a spinner."
+  (define spun? #f)
+  (define spin!
+    (let ((steps (circular-list "\\" "|" "/" "-")))
+      (lambda ()
+        (match steps
+          ((first . rest)
+           (set! steps rest)
+           (set! spun? #t) ; remember to erase spinner
+           first)))))
+
+  (define use-color?
+    (and colorize?
+         (not (or (getenv "NO_COLOR")
+                  (getenv "INSIDE_EMACS")
+                  (not (isatty? port))))))
+
+  (define handle-string
+    (let ((rules `(("^(@ build-started) (.*) (.*)"
+                    #:transform
+                    ,(lambda (m)
+                       (string-append
+                        (colorize-string "Building " 'BLUE 'BOLD)
+                        (match:substring m 2) "\n")))
+                   ("^(@ build-failed) (.*) (.*)"
+                    #:transform
+                    ,(lambda (m)
+                       (string-append
+                        (colorize-string "Build failed: " 'RED 'BOLD)
+                        (match:substring m 2) "\n")))
+                   ("^(@ build-succeeded) (.*) (.*)"
+                    #:transform
+                    ,(lambda (m)
+                       (string-append
+                        (colorize-string "Built " 'GREEN 'BOLD)
+                        (match:substring m 2) "\n")))
+                   ("^(@ substituter-started) (.*) (.*)"
+                    #:transform
+                    ,(lambda (m)
+                       (string-append
+                        (colorize-string "Substituting " 'RED 'BOLD)
+                        (match:substring m 2) "\n")))
+                   ("^(@ substituter-failed) (.*) (.*) (.*)"
+                    #:transform
+                    ,(lambda (m)
+                       (string-append
+                        (colorize-string "Substituter failed: " 'RED 'BOLD)
+                        (match:substring m 2) "\n"
+                        (match:substring m 3) ": "
+                        (match:substring m 4) "\n")))
+                   ("^(@ substituter-succeeded) (.*)"
+                    #:transform
+                    ,(lambda (m)
+                       (string-append
+                        (colorize-string "Substituted " 'GREEN 'BOLD)
+                        (match:substring m 2) "\n")))
+                   ("^(starting phase )(.*)"
+                    BLUE GREEN)
+                   ("^(phase)(.*)(succeeded after)(.*)(seconds)(.*)"
+                    GREEN BLUE GREEN BLUE GREEN BLUE)
+                   ("^(phase)(.*)(failed after)(.*)(seconds)(.*)"
+                    RED BLUE RED BLUE RED BLUE)))
+          (proc (if use-color?
+                    colorize-string
+                    (lambda (s _) s))))
+      (lambda (str)
+        (let ((processed
+               (any (match-lambda
+                      ((pattern #:transform transform)
+                       (and=> (string-match pattern str)
+                              transform))
+                      ((pattern . colors)
+                       (and=> (string-match pattern str)
+                              (lambda (m)
+                                (let ((substrings
+                                       (map (cut match:substring m <>)
+                                            (iota (- (match:count m) 1) 1))))
+                                  (string-join (map proc substrings colors) ""))))))
+                    rules)))
+          (when spun?
+            (display (string #\backspace) port))
+          (if processed
+              (begin
+                (display processed port)
+                (set! spun? #f))
+              ;; Print unprocessed line, or replace with spinner
+              (display (if verbose? str (spin!)) port))))))
+  (make-soft-port
+   (vector
+    ;; procedure accepting one character for output
+    (cut write <> port)
+    ;; procedure accepting a string for output
+    handle-string
+    ;; thunk for flushing output
+    (lambda () (force-output port))
+    ;; thunk for getting one character
+    (const #t)
+    ;; thunk for closing port (not by garbage collection)
+    (lambda () (close port)))
+   "w"))
+
 ;;; ui.scm ends here
-- 
2.18.0

[bug#32634] [PATCH 1/2] ui: Add support for colorization.

[Danny Milosavljevic]

[-- Attachment #1: Type: text/plain, Size: 7 bytes --]

LGTM!

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[bug#32634] [PATCH 2/2] ui: Add soft port for styling and filtering build output.

[Danny Milosavljevic]

[-- Attachment #1: Type: text/plain, Size: 100 bytes --]

LGTM (although the "print-build-trace" option is not documented in the help - is that on purpose?)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

[bug#32634] RFC: Process build output

[Nils Gillmann]

Hi,

Ricardo Wurmus transcribed 2.1K bytes:
> Hi Guix,
> 
> this patch set is a first draft to stylize (potentially confusing) build
> output when using “guix package” and “guix build”.
> 
> This is done by adding a soft port that matches on lines in the build
> output and colorizes them (unless INSIDE_EMACS or NO_COLOR are set, or
> when output is redirected).  For “guix package” the default behaviour is

So far I have one comment:

Would it make sense to use 'GUIX_UI_NO_COLOR' instead? This makes it
clear what it is for (use clear function names), and it does not
impose using "no colors" in other terminal applications if you
permanently export it.

> to also hide all build output that does not announce progress (unless
> “--verbose” is passed) and to let a spinner show progress instead.  For
> “guix build” all build output is still printed.
> 
> Honestly, I’m not really happy with the results, but I think it’s enough
> to start a discussion about where this should lead.
> 
> One thing I don’t like is that I had to set the “print-build-trace?”
> default option to be able to display what build is currently happening.
> Unfortunately, for small derivations this leads to output like this:
> 
> --8<---------------cut here---------------start------------->8---
> Building /gnu/store/2x5xmvimja0pbkvvr8rym91q0249ajiv-fonts-dir.drv - x86_64-linux
> Built /gnu/store/2x5xmvimja0pbkvvr8rym91q0249ajiv-fonts-dir.drv
> Building /gnu/store/diz3pmgrqibvp2pyvgh4wyr4nx5vlx0y-glib-schemas.drv - x86_64-linux
> Built /gnu/store/diz3pmgrqibvp2pyvgh4wyr4nx5vlx0y-glib-schemas.drv
> Building /gnu/store/ss70j6lf8xxiiykdys92iw92khx68ix9-info-dir.drv - x86_64-linux
> Built /gnu/store/ss70j6lf8xxiiykdys92iw92khx68ix9-info-dir.drv
> Building /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv - x86_64-linux
> Built /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv
> --8<---------------cut here---------------end--------------->8---
> 
> I would prefer:
> 
>     Building /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv … DONE
> 
> or similar.
> 
> I don’t know about whether the colours are any good; I think the bold
> green is hard to read on a bright terminal, while the black is hard to
> read on a dark terminal.
> 
> Lastly: the spinner.  It’s a bit boring, I think.
> 
> What do you think?  Is this a step in the right direction?
> 
> --
> Ricardo
> 
> 
> 
> 

[bug#32634] RFC: Process build output

[Tobias Geerinckx-Rice]

ng0,

Nils Gillmann wrote:
> Hi,
>
> Ricardo Wurmus transcribed 2.1K bytes:
>> Hi Guix,
>> 
>> this patch set is a first draft to stylize (potentially 
>> confusing) build
>> output when using “guix package” and “guix build”.
>> 
>> This is done by adding a soft port that matches on lines in the 
>> build
>> output and colorizes them (unless INSIDE_EMACS or NO_COLOR are 
>> set, or
>> when output is redirected).  For “guix package” the default 
>> behaviour is
>
> So far I have one comment:
>
> Would it make sense to use 'GUIX_UI_NO_COLOR' instead? This 
> makes it
> clear what it is for (use clear function names), and it does not
> impose using "no colors" in other terminal applications if you
> permanently export it.
>
>> to also hide all build output that does not announce progress 
>> (unless
>> “--verbose” is passed) and to let a spinner show progress 
>> instead.  For
>> “guix build” all build output is still printed.
>> 
>> Honestly, I’m not really happy with the results, but I think 
>> it’s enough
>> to start a discussion about where this should lead.
>> 
>> One thing I don’t like is that I had to set the 
>> “print-build-trace?”
>> default option to be able to display what build is currently 
>> happening.
>> Unfortunately, for small derivations this leads to output like 
>> this:
>> 
>> --8<---------------cut 
>> here---------------start------------->8---
>> Building 
>> /gnu/store/2x5xmvimja0pbkvvr8rym91q0249ajiv-fonts-dir.drv - 
>> x86_64-linux
>> Built /gnu/store/2x5xmvimja0pbkvvr8rym91q0249ajiv-fonts-dir.drv
>> Building 
>> /gnu/store/diz3pmgrqibvp2pyvgh4wyr4nx5vlx0y-glib-schemas.drv - 
>> x86_64-linux
>> Built 
>> /gnu/store/diz3pmgrqibvp2pyvgh4wyr4nx5vlx0y-glib-schemas.drv
>> Building 
>> /gnu/store/ss70j6lf8xxiiykdys92iw92khx68ix9-info-dir.drv - 
>> x86_64-linux
>> Built /gnu/store/ss70j6lf8xxiiykdys92iw92khx68ix9-info-dir.drv
>> Building 
>> /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv 
>> - x86_64-linux
>> Built 
>> /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv
>> --8<---------------cut 
>> here---------------end--------------->8---
>> 
>> I would prefer:
>> 
>>     Building 
>>     /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv 
>>     … DONE
>> 
>> or similar.
>> 
>> I don’t know about whether the colours are any good; I think 
>> the bold
>> green is hard to read on a bright terminal, while the black is 
>> hard to
>> read on a dark terminal.
>> 
>> Lastly: the spinner.  It’s a bit boring, I think.
>> 
>> What do you think?  Is this a step in the right direction?
>> 
>> --
>> Ricardo
>> 
>> 
>> 
>> 


-- 
Kind regards,

T G-R

[bug#32634] RFC: Process build output

[Tobias Geerinckx-Rice]

ng0,

Nils Gillmann wrote:
> Would it make sense to use 'GUIX_UI_NO_COLOR' instead? This 
> makes it
> clear what it is for (use clear function names), and it does not
> impose using "no colors" in other terminal applications if you
> permanently export it.

Note that the (putative) point of the NO_COLORS[0] standard is to 
work *across* project boundaries.

Kind regards,

T G-R

[0]: http://no-color.org/

[bug#32634] RFC: Process build output

[Danny Milosavljevic]

[-- Attachment #1: Type: text/plain, Size: 2417 bytes --]

Hi Ricardo,

On Tue, 04 Sep 2018 17:29:03 +0200
Ricardo Wurmus <rekado@elephly.net> wrote:

> Honestly, I’m not really happy with the results, but I think it’s enough
> to start a discussion about where this should lead.

I think it's at least vastly better than bothering a normal user with build output
(which happens pretty often).

> One thing I don’t like is that I had to set the “print-build-trace?”
> default option to be able to display what build is currently happening.
> Unfortunately, for small derivations this leads to output like this:
> 
> --8<---------------cut here---------------start------------->8---
> Building /gnu/store/2x5xmvimja0pbkvvr8rym91q0249ajiv-fonts-dir.drv - x86_64-linux
> Built /gnu/store/2x5xmvimja0pbkvvr8rym91q0249ajiv-fonts-dir.drv
> Building /gnu/store/diz3pmgrqibvp2pyvgh4wyr4nx5vlx0y-glib-schemas.drv - x86_64-linux
> Built /gnu/store/diz3pmgrqibvp2pyvgh4wyr4nx5vlx0y-glib-schemas.drv
> Building /gnu/store/ss70j6lf8xxiiykdys92iw92khx68ix9-info-dir.drv - x86_64-linux
> Built /gnu/store/ss70j6lf8xxiiykdys92iw92khx68ix9-info-dir.drv
> Building /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv - x86_64-linux
> Built /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv
> --8<---------------cut here---------------end--------------->8---
> 
> I would prefer:
> 
>     Building /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv … DONE
> 
> or similar.

I agree.  First thing I noticed as well.

> I don’t know about whether the colours are any good; I think the bold
> green is hard to read on a bright terminal, 

Yeah, it's unfortunate that some terminals just swap black and white instead
of inverting all the colors (which would mean that light green becomes dark green
etc).

>while the black is hard to
> read on a dark terminal.

Hehe.

> Lastly: the spinner.  It’s a bit boring, I think.

Yeah :)

In this case, boring is good.  It's mostly just to reassure the user that the
build didn't hang (which unfortunately can always happen in principle).

An overall progress bar would be nicer - but it would have to be supported by
the build system - and worse, by "make" etc.  I think that this would be
a larger but worthwhile project.

I did a Knight Rider style spinner in the past, but it's not really adding
anything and it was too distracting to me.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[bug#32634] RFC: Process build output

[Ricardo Wurmus]

Hi Danny,

>> Honestly, I’m not really happy with the results, but I think it’s enough
>> to start a discussion about where this should lead.
>
> I think it's at least vastly better than bothering a normal user with build output
> (which happens pretty often).

Yes, that’s what I thought as well.

>> I don’t know about whether the colours are any good; I think the bold
>> green is hard to read on a bright terminal, 
>
> Yeah, it's unfortunate that some terminals just swap black and white instead
> of inverting all the colors (which would mean that light green becomes dark green
> etc).
>
>>while the black is hard to
>> read on a dark terminal.
>
> Hehe.

Do you know of any “best practices” in that field?  Maybe there are some
terminal capabilities / settings I should query first…?

Or should we look for settings in, say, ~/.config/guix/colors?

>> Lastly: the spinner.  It’s a bit boring, I think.
>
> Yeah :)
>
> In this case, boring is good.  It's mostly just to reassure the user that the
> build didn't hang (which unfortunately can always happen in principle).
>
> An overall progress bar would be nicer - but it would have to be supported by
> the build system - and worse, by "make" etc.  I think that this would be
> a larger but worthwhile project.

Yeah, it’s very difficult.  With CMake the progress indicator is
built-in; but with plain Make it’s really not easy to get an advance
estimate of the amount of work that will be done and to get an idea of
how many tasks are still remaining.

> I did a Knight Rider style spinner in the past, but it's not really adding
> anything and it was too distracting to me.

Heh :)

-- 
Ricardo

PS: I noticed a bug in this implementation: NO_COLOR is ignored for the
“@” lines; I’ll fix this before pushing later tonight.

bug#32634: [PATCH 2/2] ui: Add soft port for styling and filtering build output.

[Ricardo Wurmus]

Danny Milosavljevic <dannym@scratchpost.org> writes:

> LGTM (although the "print-build-trace" option is not documented in the help - is that on purpose?)

Thanks for the review.

I’ve made a minor change so that even for “@” lines no colour is used
when NO_COLOR is set and pushed the patches to the master branch with
commit 15cc7e6ad.

About “print-build-trace”: I only learned about it on IRC.

--
Ricardo

[bug#32634] [PATCH 2/2] ui: Add soft port for styling and filtering build output.

[Ludovic Courtès]

Hello,

Ricardo Wurmus <rekado@elephly.net> skribis:

> * guix/ui.scm (build-output-port): New procedure.
> * guix/scripts/package.scm (%default-options): Print build trace.
> (guix-package): Use build-output-port.
> * guix/scripts/build.scm (guix-build): Use build-output-port.
>
> Co-authored-by: Sahithi Yarlagadda <sahi@swecha.net>

Really nice to see this happen!

I’m late to the party but here are some comments:

> +(define* (build-output-port #:key
> +                            (colorize? #t)
> +                            verbose?
> +                            (port (current-error-port)))
> +  "Return a soft port that processes build output.  By default it colorizes
> +phase announcements and replaces any other output with a spinner."
> +  (define spun? #f)
> +  (define spin!
> +    (let ((steps (circular-list "\\" "|" "/" "-")))
> +      (lambda ()
> +        (match steps
> +          ((first . rest)
> +           (set! steps rest)
> +           (set! spun? #t) ; remember to erase spinner
> +           first)))))
> +
> +  (define use-color?
> +    (and colorize?
> +         (not (or (getenv "NO_COLOR")
> +                  (getenv "INSIDE_EMACS")
> +                  (not (isatty? port))))))

I would rather do:

  (define* (build-output-port #:key
                              (colorize? (not (or …))))
    …)

so that callers can still choose to turn it on and off.

Also, perhaps we can optimize things:

  (if (and verbose? (not colorize?))
      port
      (make-soft-port …))

> +  (define handle-string
> +    (let ((rules `(("^(@ build-started) (.*) (.*)"
> +                    #:transform
> +                    ,(lambda (m)
> +                       (string-append
> +                        (colorize-string "Building " 'BLUE 'BOLD)
> +                        (match:substring m 2) "\n")))

It think we should precompile all the regexps with ‘make-regexp’ instead
of calling ‘string-match’ (which in turn calls ‘make-regexp’) for every
line.

> +  (make-soft-port
> +   (vector
> +    ;; procedure accepting one character for output
> +    (cut write <> port)
> +    ;; procedure accepting a string for output
> +    handle-string
> +    ;; thunk for flushing output
> +    (lambda () (force-output port))
> +    ;; thunk for getting one character
> +    (const #t)

I think we should probably user ‘make-custom-binary-output-port’ instead
of ‘make-soft-port’ because it’s a cleaner and more faithful API
(‘make-soft-port’ no longer matches the operations implemented by port
types.)

The brings the issue of how to properly decode build output.  Commit
ce72c780746776a86f59747f5eff8731cb4ff39b fixes issues in this area.  I
wrote the tests below to see how ‘build-output-port’ behaves when passed
valid UTF-8 and garbage, and it appears to behave correctly, though the
question mark that we get in return when throwing garbage at it suggests
that decoding substitution is not happening where it should.

If we use a custom-binary-output-port, we’ll have to do something
similar to what ‘read-maybe-utf8-string’, and that way we’ll be fully in
control.

> +    ;; thunk for closing port (not by garbage collection)
> +    (lambda () (close port)))

At call sites, we should probably do:

  (build-output-port #:port (duplicate-port (current-error-port)))

instead of just:

  (build-output-port)

so that stderr doesn’t get closed.

Thoughts?

Thanks!

Ludo’.

[bug#32634] [PATCH 2/2] ui: Add soft port for styling and filtering build output.

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

ludo@gnu.org (Ludovic Courtès) skribis:

> The brings the issue of how to properly decode build output.  Commit
> ce72c780746776a86f59747f5eff8731cb4ff39b fixes issues in this area.  I
> wrote the tests below to see how ‘build-output-port’ behaves when passed
> valid UTF-8 and garbage, and it appears to behave correctly, though the
> question mark that we get in return when throwing garbage at it suggests
> that decoding substitution is not happening where it should.

Oops, forgot to send the tests:


[-- Attachment #2: Type: text/x-patch, Size: 1786 bytes --]

diff --git a/tests/ui.scm b/tests/ui.scm
index 1e98e3534..598402db4 100644
--- a/tests/ui.scm
+++ b/tests/ui.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2013, 2014, 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2018 Ludovic Courtès <ludo@gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -27,6 +27,8 @@
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-19)
   #:use-module (srfi srfi-64)
+  #:use-module (rnrs bytevectors)
+  #:use-module (rnrs io ports)
   #:use-module (ice-9 regex))
 
 ;; Test the (guix ui) module.
@@ -260,4 +262,26 @@ Second line" 24))
                                                  "ISO-8859-1")
                              (show-manifest-transaction store m t))))))))
 
+(test-equal "build-output-port, UTF-8"
+  "lambda is λ!\n"
+  (let* ((output (open-output-string))
+         (port   (build-output-port #:port output #:verbose? #t))
+         (bv     (string->utf8 "lambda is λ!\n")))
+    (put-bytevector port bv)
+    (force-output port)
+    ;; Note: 'build-output-port' automatically closes OUTPUT.
+    (get-output-string output)))
+
+(test-equal "current-build-output-port, UTF-8 + garbage"
+  ;; What about a mixture of UTF-8 + garbage?
+  "garbage: ?lambda: λ"                   ;XXX: why not REPLACEMENT CHARACTER?
+  (let* ((output (open-output-string))
+         (port   (build-output-port #:port output #:verbose? #t)))
+    (set-port-conversion-strategy! output 'error)
+    (display "garbage: " port)
+    (put-bytevector port #vu8(128))
+    (put-bytevector port (string->utf8 "lambda: λ"))
+    (force-output port)
+    (get-output-string output)))
+
 (test-end "ui")

[-- Attachment #3: Type: text/plain, Size: 12 bytes --]


Ludo’.

[bug#32634] RFC: Process build output

[Ludovic Courtès]

Hello!

Ricardo Wurmus <rekado@elephly.net> skribis:

> One thing I don’t like is that I had to set the “print-build-trace?”
> default option to be able to display what build is currently happening.
> Unfortunately, for small derivations this leads to output like this:
>
> Building /gnu/store/2x5xmvimja0pbkvvr8rym91q0249ajiv-fonts-dir.drv - x86_64-linux
> Built /gnu/store/2x5xmvimja0pbkvvr8rym91q0249ajiv-fonts-dir.drv
> Building /gnu/store/diz3pmgrqibvp2pyvgh4wyr4nx5vlx0y-glib-schemas.drv - x86_64-linux
> Built /gnu/store/diz3pmgrqibvp2pyvgh4wyr4nx5vlx0y-glib-schemas.drv
> Building /gnu/store/ss70j6lf8xxiiykdys92iw92khx68ix9-info-dir.drv - x86_64-linux
> Built /gnu/store/ss70j6lf8xxiiykdys92iw92khx68ix9-info-dir.drv
> Building /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv - x86_64-linux
> Built /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv
>
> I would prefer:
>
>     Building /gnu/store/rh92rslbj4x9abyna6lc11jqifbavx13-librsvg-2.40.20.drv … DONE
>
> or similar.
>
> I don’t know about whether the colours are any good; I think the bold
> green is hard to read on a bright terminal, while the black is hard to
> read on a dark terminal.
>
> Lastly: the spinner.  It’s a bit boring, I think.
>
> What do you think?  Is this a step in the right direction?

Surely!  Another thing we could do, instead of writing “Building” lines,
is to display MAX-JOBS status lines that get updated as things go (when
on a TTY).  This is what I had in mind with:

  https://git.savannah.gnu.org/cgit/guix.git/commit/?h=wip-ui&id=c367549b4779591844cfbf2975301e4d8ae70b92
  https://lists.gnu.org/archive/html/guix-devel/2017-05/msg00516.html

That branch also had changes to print more “build traces” such that we
can distinguish downloads (corresponding to fixed-output derivations)
from other derivations, which has pros and cons.

Thoughts?

Ludo’.