[bug#41066] [PATCH] gnu: bootloader: Support for chain loading.

[Danny Milosavljevic]

Hi Stefan,

this looks very good in general.

I have only a few doubts--mostly concerning the end-user API "bootloader-chain":

On Sun, 18 Oct 2020 13:21:28 +0200
Stefan <stefan-guix@vodafonemail.de> wrote:

>         (bootloader-chain grub-efi-netboot-bootloader
>                           (list u-boot-my-scb
>                                 firmware)
>                           '("libexec/u-boot.bin"
>                             "firmware/")
>                           (list (plain-file "config.txt"
>                                             "kernel=u-boot.bin"))
>                           #:installer (install-grub-efi-netboot "efi/boot"))

I would prefer if it was possible to do the following:

 (bootloader-chain (list firmware (plain-file "config.txt" "kernel=u-boot.bin") 
u-boot-my-scb) grub-efi-netboot-bootloader)

(I would put the main bootloader last because the user probably thinks of the
list of bootloaders in the order they are loaded at boot)

[maybe even

 (bootloader-chain (list u-boot-my-scb firmware (plain-file "config.txt" 
"kernel=u-boot.bin") grub-efi-netboot-bootloader))

-- with documentation that the order of the entries matters.  But maybe that
would be too magical--since only the last entry in that list would have their
installer called, and the actual guix generations also only go into the last
one's configuration.  But maybe that would be OK anyway]

Also, I don't like the ad hoc derivation subset selection mechanism you have.

I understand that it can be nice to be able to select a subset in general, but:

* Usually we make a special package, inherit from the original package, and then
just make it put the files we want into the derivation output directory.
The user would put "u-boot-my-scb-minimal" instead of "u-boot-my-scb" in
that case, and then only get the subset.

* In this special case of chaining bootloaders, I think that the profile hook
can take care of deleting all the unnecessary files, and of all the other weird
complications (installing FILES, moving stuff around etc--maybe even generating
or updating that config.txt one day).
One of the reasons I suggested using a profile in the first place is that
now the profile hook can do all the dirty work, even long term.

* If that isn't possible either, it would be nicer to have a helper
procedure that allows you to select a subset of the files that
are in the derivation of a package, and not have this subset selection mingled
into the innards of bootloader-chain.  (separation of concerns)
Maybe there could even be a package transformation to do that.

I know that there are probably good reasons why you did it like you did.

But still, I think a profile hook is exactly the right kind of tool to hide
the extra setup complexity that my API requires, and the API would be less
complicated to use and more stable (less likely to ever need to be changed).

What do you think?

Also, if it is difficult to collect bootloader packages into a profile
automatically (without extra user-supplied information) because of the subdir
"libexec" in u-boot derivations, then we can modify all the u-boot packages
(for good) to put u-boot into "$output/" instead of "$output/libexec".
I would prefer fixing things instead of having to put workarounds into user
configuration.  Please tell me if you want that--we can do that.

>                           #:installer (install-grub-efi-netboot "efi/boot"))

That should be automatic but overridable.
This seems to be the case already.  Nice!

> +(define (bootloader-profile packages package-contents files)

If using my suggested bootloader-chain API, this would just get PACKAGES,
not PACKAGE-CONTENTS and FILES (FILES would be mixed into the PACKAGES 
list--which
thus should be renamed to ITEMS or something).

> +  (define (bootloader-collection manifest)
> +    (define build
> +        (with-imported-modules '((guix build utils)
> +                                 (ice-9 ftw)
> +                                 (srfi srfi-1))
> +          #~(begin
> +            (use-modules ((guix build utils)
> +                          #:select (mkdir-p strip-store-file-name))
> +                         ((ice-9 ftw)
> +                          #:select (scandir))
> +                         ((srfi srfi-1)
> +                          #:select (append-map every remove)))
> +            (define (symlink-to file directory transform)
> +              "Creates a symlink to FILE named (TRANSFORM FILE) in 
> DIRECTORY."
> +              (when (file-exists? file)
> +                    (symlink file
> +                             (string-append directory "/" (transform 
> file)))))
> +            (define (directory-content directory)
> +              "Creates a list of absolute path names inside DIRECTORY."
> +              (map (lambda (name)
> +                     (string-append directory name))
> +                   (sort (or (scandir directory
> +                                      (lambda (name)
> +                                        (not (member name '("." "..")))))
> +                             '())
> +                         string<?)))
> +            (define (select-names select names)
> +              "Set SELECT to 'filter' or 'remove' names ending with '/'."
> +              (select (lambda (name)
> +                        (string-suffix? "/" name))
> +                      names))
> +            (define (filter-names-without-slash names)
> +              (select-names remove names))
> +            (define (filter-names-with-slash names)
> +              (select-names filter names))

I would prefer these to be in another procedure that can be used to transform
any package (or profile?) like this.  @Ludo: What do you think?

[...]
> +    (gexp->derivation "bootloader-collection"
> +                      build
> +                      #:local-build? #t
> +                      #:substitutable? #f
> +                      #:properties
> +                      `((type . profile-hook)
> +                        (hook . bootloader-collection))))
> +
> +  (profile (content (packages->manifest packages))
> +           (name "bootloader-profile")
> +           (hooks (list bootloader-collection))
> +           (locales? #f)
> +           (allow-collisions? #f)
> +           (relative-symlinks? #f)))
> +
> +(define* (bootloader-chain
[...]

> +    (bootloader
> +     (inherit final-bootloader)
> +     (package profile)

I like that.  Smart way to reuse existing code.

> +     (installer
> +      #~(lambda (bootloader target mount-point)
> +          (#$final-installer bootloader target mount-point)
> +          (copy-recursively
> +           (string-append bootloader "/collection")
> +           (string-append mount-point target)
> +           #:follow-symlinks? #t
> +           #:log (%make-void-port "w")))))))

Thanks!

pgp2Jo8i2695p.pgp
Description: OpenPGP digital signature