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

[Ludovic Courtès]

Hello Mathieu,

Mathieu Othacehe <address@hidden> skribis:

> * gnu/bootloader.scm: New file.
> * gnu/bootloader/extlinux.scm: New file.
> * gnu/bootloader/grub.scm: New file.
> * gnu/local.mk: Build new files.
> * gnu/system.scm: Adapt to new bootloader api.
> * gnu/scripts/system.scm: Adapt to new bootloader api.
> * gnu.scm: Remove (gnu system grub) and replace by (gnu bootloader) and (gnu
> bootloader grub) modules.
> * gnu/system/grub.scm: Moved content to gnu/bootloader/grub.scm.
> * gnu/system/vm: Replace (gnu system grub) module by (gnu bootloader).
> * gnu/tests.scm: Ditto.
> * gnu/tests/nfs.scm: Ditto.

Nice!  Some very minor comments before you push:

> +;;; The <bootloader> record contains fields expressing how the bootloader
> +;;; should be installed. Every bootloader in gnu/bootloader/ directory
> +;;; has to be described by this record.
   ^
Only two semicolons here.  :-)

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

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

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

> +;;;
> +;;; Bootloader definitions.
> +;;;
> +
> +(define extlinux-bootloader
> +  (bootloader
> +   (name 'extlinux)
> +   (package #f)
> +   (installer #f)

Why #f here?  Looks fishy.

> +;;;
> +;;; 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)
    …)

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…).

Otherwise LGTM, thanks!

Ludo’.