From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44975) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d9tXD-0000mP-VI for guix-patches@gnu.org; Sun, 14 May 2017 09:26:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d9tX8-0003MQ-VW for guix-patches@gnu.org; Sun, 14 May 2017 09:26:07 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:41007) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1d9tX8-0003M5-N9 for guix-patches@gnu.org; Sun, 14 May 2017 09:26:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1d9tX8-0006yP-2J for guix-patches@gnu.org; Sun, 14 May 2017 09:26:02 -0400 Subject: bug#26339: [PATCH v4 1/7] bootloader: Add extlinux support. Resent-Message-ID: From: ludo@gnu.org (Ludovic =?UTF-8?Q?Court=C3=A8s?=) References: <20170514074803.25556-1-m.othacehe@gmail.com> <20170514074803.25556-2-m.othacehe@gmail.com> Date: Sun, 14 May 2017 15:25:28 +0200 In-Reply-To: <20170514074803.25556-2-m.othacehe@gmail.com> (Mathieu Othacehe's message of "Sun, 14 May 2017 09:47:57 +0200") Message-ID: <87lgpzwoxj.fsf@gnu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Mathieu Othacehe Cc: 26339@debbugs.gnu.org Hello Mathieu, Mathieu Othacehe 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 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 rather than no? For example GRUB does not impose a particular theme. > +(define dd > + #~(lambda (bs count if of) > + (zero? (system* "dd" > + (string-append "bs=3D" (number->string bs)) > + (string-append "count=3D" (number->string count)) > + (string-append "if=3D" if) > + (string-append "of=3D" of))))) Dunno but in the future it may be better to use =E2=80=98dump-port=E2=80=99= from (guix build utils) than to invoke =E2=80=98dd=E2=80=99. > +(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) =E2=80=A6) Speaking of which, could you update guix.texi, maybe in a subsequent patch to avoid blocking this one? I suppose =E2=80=9CGRUB Configuration=E2= =80=9D would have to be renamed to =E2=80=9CBootloader Configuration=E2=80=9D. > +;;; > +;;; 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=E2=80=99s 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 =E2=80=98grub=E2=80=99 fie= ld comes first, but that should be OK (esp. since UEFI support was not documented until now=E2=80=A6). Otherwise LGTM, thanks! Ludo=E2=80=99.