bug#26339: [PATCH 02/18] system: Add extlinux support.

[ludo@gnu.org (Ludovic Courtès)]

Hello!

Sorry for the delay, and thanks for reminding me of this patch.

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

> From: David Craven <david@craven.ch>
>
> * gnu/system.scm (operating-system): Add default bootloader.
>   (operating-system-grub.cfg): Use bootloader-configuration-file-procedure.
> * gnu/system/grub.scm (bootloader-configuration->grub-configuration): New
>   variable.
>   (grub-configuration-file): Use bootloader-configuration->grub-configuration.
> * guix/scripts/system.scm (profile-grub-entries): Rename system->grub-entry to
>   system->boot-parameters and adjust accordingly.
>   (perform-action): Make bootloader optional. Use
>   bootloader-configuration-device.
> * gnu/system/bootloader.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
> * tests/system.scm: Adjust operating-system to new API.
> * tests/guix-system.sh: Adjust operating-system to new API.

[...]

> +(define-record-type* <bootloader-configuration>
> +  bootloader-configuration make-bootloader-configuration
> +  bootloader-configuration?
> +  (bootloader                      bootloader-configuration-bootloader     ; package
> +                                   (default #f))
> +  (device                          bootloader-configuration-device         ; string
> +                                   (default #f))
> +  (menu-entries                    bootloader-configuration-menu-entries   ; list of <boot-parameters>
> +                                   (default '()))
> +  (default-entry                   bootloader-configuration-default-entry  ; integer
> +                                   (default 0))
> +  (timeout                         bootloader-configuration-timeout        ; integer
> +                                   (default 5))
> +  (configuration-file-location     bootloader-configuration-file-location
> +                                   (default #f))
> +  (configuration-file-procedure    bootloader-configuration-file-procedure ; procedure
> +                                   (default #f))
> +  (install-procedure               bootloader-configuration-install-procedure ; procedure
> +                                   (default #f))
> +  (additional-configuration        bootloader-configuration-additional-configuration ; record
> +                                   (default #f)))
> +

To avoid mistakes, I think we should remove default values for fields
that really need to be set.  For example, ‘configuration-file-location’
and ‘install-procedure’ should probably not have a default value.

I would rename ‘configuration-file-location’ to ‘configuration-file’.

I’m fine with ‘configuration-file-procedure’, but ‘generator’ is OK too.

I would make ‘bootloader’ a delayed field (see the ‘patches’ field of
<origin> for an example of that.)  That would avoid loading the package
modules upfront.

> +;;;
> +;;; Bootloader configurations.
> +;;;
> +
> +(define* (extlinux-configuration #:optional (config (bootloader-configuration)))
> +  (bootloader-configuration
> +   (inherit config)
> +   (configuration-file-location "/boot/extlinux/extlinux.conf")
> +   (configuration-file-procedure extlinux-configuration-file)))
> +
> +(define* (grub-configuration #:optional (config (bootloader-configuration)))
> +  (bootloader-configuration
> +   (inherit config)
> +   (bootloader (@ (gnu packages bootloaders) grub))
> +   (configuration-file-location "/boot/grub/grub.cfg")
> +   (configuration-file-procedure grub-configuration-file)
> +   (install-procedure install-grub)
> +   (additional-configuration
> +    (let ((additional-config (bootloader-configuration-additional-configuration config)))
> +      (if additional-config additional-config %default-theme)))))
> +
> +(define* (grub-efi-configuration #:optional (config (bootloader-configuration)))
> +  (bootloader-configuration
> +   (inherit (grub-configuration config))
> +   (bootloader (@ (gnu packages bootloaders) grub-efi))))
> +
> +(define* (syslinux-configuration #:optional (config (bootloader-configuration)))
> +  (bootloader-configuration
> +   (inherit (extlinux-configuration config))
> +   (bootloader (@ (gnu packages bootloaders) syslinux))
> +   (install-procedure install-syslinux)))

So it looks like this is the reason for all the default values.

What about distinguishing two things: bootloader configuration, and
bootloader implementation?  This would be akin to the relation between
<package> and <build-system>.

We’d have <bootloader-configuration>, but without the ‘bootloader’,
‘install-procedure’, ‘configuration-file-procedure’, and
‘configuration-file’ fields.  Instead of those 4 fields, we’d have a
single ‘bootloader’ field whose value would be a <bootloader> record.

The <bootloader> record would contain these 4 fields (package,
install-procedure, configuration-file, configuration-file-procedure),
maybe along with a ‘name’ field to aid debugging.

The <bootloader> record would define the implementation (GRUB, extlinux,
u-boot, etc.) while <bootloader-configuration> would be purely
configuration.

How does that sound?

We could do:

  (define grub-bootloader
    (bootloader
      (package grub)
      …))

in (gnu bootloader grub).

Likewise we’d define ‘extlinux-bootloader’ in (gnu bootloader extlinux).

The (gnu system) module would include (gnu bootloader), but it would not
include (gnu bootloader grub) nor (gnu bootloader extlinux).  Users
would include one of these in their config file instead.

> --- a/tests/system.scm
> +++ b/tests/system.scm
> @@ -36,7 +36,6 @@
>      (host-name "komputilo")
>      (timezone "Europe/Berlin")
>      (locale "en_US.utf8")
> -    (bootloader (grub-configuration (device "/dev/sdX")))
>      (file-systems (cons %root-fs %base-file-systems))

In gnu/system.scm, there’s:

> +  (bootloader operating-system-bootloader         ; <bootloader-configuration>
> +              (default (extlinux-configuration)))


If there was a default value here, I think it’d rather be GRUB ;-), but
probably we’d better not have a default at all.  How would the
installation device be found?

Thanks a lot, and sorry for the loooong delay!

Ludo’.