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

["Ludovic Courtès" <ludo@gnu.org>]

Hi,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> 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"))

In the example above, I think that you could merge the 2nd and 3rd
arguments by using ‘file-append’:

  (bootloader-chain grub-efi-netboot-bootloader
                    (list (file-append u-boot "/libexec/u-boot.bin")
                          (file-append firmware "/firmware")))

No?

I think we should look at how to simplify this interface.  Intuitively,
I would expect the ‘bootloader-chain’ to take a list of <bootloader>
records and return a <bootloader> record.

Is this something that can be achieved?  If not, what are the extra
constraints that need to be taken into account?

>> +(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).

Yes, if it’s about building a profile, you could just use a <profile>
object.  Would that work here?

>> +  (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?

From a quick look at the patch, I don’t fully understand yet what’s
going on.

Stylistically, I’d suggest a few things to make the code more consistent
with the rest of the code base, and thus hopefully easier to grasp for
the rest of us:

  1. Don’t sort the result of ‘scandir’, it’s already sorted.

  2. Remove ‘select-names’ as it requires people to read more code to
     understand that we’re just filtering/removing.   Instead, declare:

       (define absolute-file-name? (cut string-suffix? "/" <>))

     and write:

       (filter absolute-file-name? names)
       (remote absolute-file-name? names)

HTH!

Ludo’.