Subject: Re: bug#26339: [PATCH 04/18] bootloader: Add install procedures and use them.

From: Danny Milosavljevic <[email-masked]>
Date: Sat, 15 Apr 2017 18:22:22 +0200

> +    (symlink bootcfg (string-append directory "/bootcfg"))))
                                                   ^^^^^^^

Is that safe? Will there only be one bootloader configuration at all times? What if the user uses multiple "guix system"s on the same machine?

> +    (install-boot-config bootcfg bootcfg-location target)
> +    (when install-bootloader
> +      (install-bootloader bootloader device target))

I think it's very good to finally seperate installation of the bootloader configuration and installation of the bootloader.  On some architectures (for example ARM) it's very uncommon to replace the bootloader - and if you do it too often it would wear out the flash storage prematurely.

>[exports]
> +            dd

Errrr... was that exported on purpose? Sounds very implementation-detaily to me.

> +                                   #:install-bootloader

Rather #:bootloader-installer or #:bootloader-installation ?  Or leave it as is, doesn't matter much, I guess.  I think a non-substantive for this is strange.

> +                                   #$(bootloader-configuration-install-procedure

If it's not installing a procedure it should be bootloader-configuration-installation-procedure or bootloader-configuration-installer, I think.

Otherwise LGTM!