bug#26339: [PATCH v4 1/7] bootloader: Add extlinux support.

[Mathieu Othacehe <m.othacehe@gmail.com>]

Hi Ludo,

> Nice!  Some very minor comments before you push:

Thank you :)

> Only two semicolons here.  :-)

Ok.

>
>> +  (theme                           bootloader-theme
>> +                                   (default #f))
>
> I would expect the theme to be part of <bootloader-configuration> rather
> than <bootloader> no?  For example GRUB does not impose a particular
> theme.

Ok I moved it to <bootloader-configuration>.

>
>> +(define dd
>> +  #~(lambda (bs count if of)
>> +      (zero? (system* "dd"
>> +                      (string-append "bs=" (number->string bs))
>> +                      (string-append "count=" (number->string count))
>> +                      (string-append "if=" if)
>> +                      (string-append "of=" of)))))
>
> Dunno but in the future it may be better to use ‘dump-port’ from (guix
> build utils) than to invoke ‘dd’.

Ok, I'll consider it in a follow-up serie.

>
>> +(define install-syslinux
>> +  #~(lambda (bootloader device mount-point)
>> +      (let ((extlinux (string-append bootloader "/sbin/extlinux"))
>> +            (install-dir (string-append mount-point "/boot/extlinux"))
>> +            (syslinux-dir (string-append bootloader "/share/syslinux")))
>> +        (mkdir-p install-dir)
>> +        (for-each (lambda (file)
>> +                    (copy-file file
>> +                               (string-append install-dir "/" (basename file))))
>
> Rather: (install-file file install-dir).

Ok.

>
>> +;;;
>> +;;; Bootloader definitions.
>> +;;;
>> +
>> +(define extlinux-bootloader
>> +  (bootloader
>> +   (name 'extlinux)
>> +   (package #f)
>> +   (installer #f)
>
> Why #f here?  Looks fishy.

It turns out that the distinction between extlinux and syslinux was
weird in David serie. "extlinux" bootloader was no installing anything.

My understanding is that extlinux is one of the bootloaders provided by
the syslinux package. So, I removed the notion of "syslinux-bootloader"
and only kepy an "extlinux-bootloader" before applying.

>
>> +;;;
>> +;;; Compatibility macros.
>> +;;;
>> +
>> +(define-syntax-rule (extlinux-configuration fields ...)
>> +  (bootloader-configuration
>> +   (bootloader extlinux-bootloader)
>> +   fields ...))
>> +
>> +(define-syntax-rule (syslinux-configuration fields ...)
>> +  (bootloader-configuration
>> +   (bootloader syslinux-bootloader)
>> +   fields ...))
>
> We can omit these two macros since they have never existed before this
> patch.  New users will write:
>
>   (bootloader-configuration
>     (bootloader syslinux-bootloader)
>     …)

Ok.

>
> Speaking of which, could you update guix.texi, maybe in a subsequent
> patch to avoid blocking this one?  I suppose “GRUB Configuration” would
> have to be renamed to “Bootloader Configuration”.
>
>> +;;;
>> +;;; Compatibility macros.
>> +;;;
>> +
>> +(define-syntax-rule (grub-configuration fields ...)
>> +  (bootloader-configuration
>> +   (bootloader grub-bootloader)
>> +   fields ...))
>> +
>> +(define-syntax-rule (grub-efi-configuration fields ...)
>> +  (bootloader-configuration
>> +   (bootloader grub-efi-bootloader)
>> +   fields ...))
>
> The second macro can be removed (it did not exist before).
>
> However the first one might need to be improved to be really compatible
> with what exists now.  For instance, my laptop’s config has this:
>
>   (grub-configuration
>     (grub grub-efi)
>     (device "/dev/sda"))
>
> So we probably need something like this (untested):
>
>   (define-syntax grub-configuration
>     (syntax-rules (grub)
>       ((_ (grub package) fields ...)
>        (if (eq? package grub)
>            (bootloader-configuration
>              (bootloader grub-bootloader)
>              fields ...)
>            (bootloader-configuration
>              (bootloader grub-efi-bootloader)
>              fields ...)))
>       ((_ fields ...)
>        (bootloader-configuration
>          (bootloader grub-bootloader)
>          fields ...))))
>
> This will only address the simple case where the ‘grub’ field comes
> first, but that should be OK (esp. since UEFI support was not documented
> until now…).

It seems to work ok, I took it :)

Thanks,

Mathieu