Subject: Re: bug#26339: [PATCH v3 6/9] scripts: system: Adapt "reconfigure" to new bootloader API.

From: Danny Milosavljevic <[email-masked]>
Date: Sun, 7 May 2017 22:33:32 +0200

Hi Mathieu,

> +(define* (install-bootloader install-procedure
> +                             #:key
> +                             bootcfg bootcfg-location
> +                             device target)

Maybe "installer" instead of "install-procedure"?  It's not that important but it reads better I think...

>+(define (install-bootloader-derivation install-procedure
>+                                       bootloader device target)
>+  (with-monad %store-monad
>+    (gexp->file "install-bootloader"
>+                (with-imported-modules '((guix build utils))
>+                  #~(begin
>+                      (use-modules (guix build utils))
>+                      (#$install-procedure #$bootloader
>+                                           #$device
>+                                           #$target))))))

Hmm, importing (guix build utils) twice (on different sides, I guess)?  Could use a comment...

> +       (install-proc
> +        (let ((procedure (bootloader-configuration-installer
> +                          (operating-system-bootloader os)))
> +              (target    (or target "/")))
> +          (install-bootloader-derivation procedure bootloader device target)))

Here, I'd use "bootloader-installer" instead of "install-proc" - because this specific procedure could install lots of things - it's the main entry point in a sense.

Otherwise LGTM!  I also think the general approach is good.