From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50153) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dAbxO-0002Qu-Sr for guix-patches@gnu.org; Tue, 16 May 2017 08:52:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dAbxK-00015c-VC for guix-patches@gnu.org; Tue, 16 May 2017 08:52:06 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:44858) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dAbxK-00015Y-Qm for guix-patches@gnu.org; Tue, 16 May 2017 08:52:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dAbxK-0005h6-HI for guix-patches@gnu.org; Tue, 16 May 2017 08:52:02 -0400 Subject: bug#26339: [PATCH v4 1/7] bootloader: Add extlinux support. Resent-Message-ID: References: <20170514074803.25556-1-m.othacehe@gmail.com> <20170514074803.25556-2-m.othacehe@gmail.com> <87lgpzwoxj.fsf@gnu.org> From: Mathieu Othacehe In-reply-to: <87lgpzwoxj.fsf@gnu.org> Date: Tue, 16 May 2017 14:51:44 +0200 Message-ID: <86h90lt15r.fsf@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 26339@debbugs.gnu.org Hi Ludo, > Nice! Some very minor comments before you push: Thank you :) > Only two semicolons here. :-) Ok. > >> + (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. Ok I moved it to . > >> +(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’. Ok, I'll consider it in a follow-up serie. > >> +(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). Ok. > >> +;;; >> +;;; Bootloader definitions. >> +;;; >> + >> +(define extlinux-bootloader >> + (bootloader >> + (name 'extlinux) >> + (package #f) >> + (installer #f) > > Why #f here? Looks fishy. It turns out that the distinction between extlinux and syslinux was weird in David serie. "extlinux" bootloader was no installing anything. My understanding is that extlinux is one of the bootloaders provided by the syslinux package. So, I removed the notion of "syslinux-bootloader" and only kepy an "extlinux-bootloader" before applying. > >> +;;; >> +;;; 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) > …) Ok. > > 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…). It seems to work ok, I took it :) Thanks, Mathieu