Package: guix-patches;
Reported by: Mathieu Othacehe <m.othacehe <at> gmail.com>
Date: Sun, 2 Apr 2017 13:51:01 UTC
Severity: important
Tags: patch
Done: Mathieu Othacehe <m.othacehe <at> gmail.com>
Bug is archived. No further changes may be made.
Message #339 received at 26339 <at> debbugs.gnu.org (full text, mbox):
From: ludo <at> gnu.org (Ludovic Courtès) To: Mathieu Othacehe <m.othacehe <at> gmail.com> Cc: 26339 <at> debbugs.gnu.org, David Craven <david <at> craven.ch> Subject: Re: bug#26339: [PATCH 02/18] system: Add extlinux support. Date: Mon, 08 May 2017 22:06:18 +0200
Hello!
Sorry for the delay, and thanks for reminding me of this patch.
Mathieu Othacehe <m.othacehe <at> gmail.com> skribis:
> From: David Craven <david <at> 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’.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.