Subject: Re: [bug#36404] [PATCH v4 3/4] Add 'guix deploy'.

From: Ludovic Courtès <[email-masked]>
Date: Fri, 05 Jul 2019 10:17:49 +0200

[email-masked] (Jakob L. Kreuze) skribis:

> * guix/scripts/deploy.scm: New file.
> * Makefile.am (MODULES): Add it.

Overall LGTM, just a couple of minor points:

> +++ b/guix/scripts/deploy.scm

Please add this file to po/guix/POTFILES.in so it can be subject to
localization.

> +(define %default-options
> +  '((system . ,(%current-system))
> +    (substitutes? . #t)
> +    (build-hook? . #t)
> +    (graft? . #t)
> +    (debug . 0)
> +    (verbosity . 2)))

‘verbosity’ should probably be 1 (only ‘guix build’ and ‘guix system
build’ default to 2.)

> +      (for-each (lambda (machine)
> +                  (format #t "building ~a... " (machine-display-name machine))
> +                  (run-with-store store (build-machine machine))
> +                  (display "done\n"))
> +                machines)
> +      (for-each (lambda (machine)
> +                  (format #t "deploying to ~a... " (machine-display-name machine))
> +                  (run-with-store store (deploy-machine machine))
> +                  (display "done\n"))
> +                machines))))

For i18n purposes and also to get consistent output, please avoid
‘format #t’ and instead write:

  (info (G_ "deploying ~a…~%") (machine-display-name machine))

I think you can omit the “done” message.

As a matter of style, it’s clearer IMO to have only one ‘run-with-store’
call in the whole program.

Also, the separate ‘build-machine’ phase is not needed—more on that in
another message.

Thanks,
Ludo’.