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

[ludo@gnu.org (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’.