GNU bug report logs - #26339
[PATCH 00/18] wip: Support non grub bootloaders.

Previous Next

Full log

View this message in rfc822 format

From: ludo <at> gnu.org (Ludovic Courtès)
To: Mathieu Othacehe <m.othacehe <at> gmail.com>
Cc: 26339 <at> debbugs.gnu.org
Subject: bug#26339: [PATCH v4 1/7] bootloader: Add extlinux support.
Date: Sun, 14 May 2017 15:25:28 +0200

Hello Mathieu,

Mathieu Othacehe <m.othacehe <at> gmail.com> 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’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.