Patchwork [6/6] gnu: Add grub-efi.

login
register
mail settings
Submitter Marius Bakke
Date Nov. 5, 2016, 7:38 p.m.
Message ID <87h97l67kz.fsf@kirby.i-did-not-set--mail-host-address--so-tickle-me>
Download mbox | patch
Permalink /patch/17227/
State New
Headers show

Comments

Marius Bakke - Nov. 5, 2016, 7:38 p.m.
Leo Famulari <leo@famulari.name> writes:

> On Sat, Nov 05, 2016 at 12:55:11PM +0000, Marius Bakke wrote:
>> * gnu/packages/grub.scm (grub-efi): New variable.
>> ---
>>  gnu/packages/grub.scm | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>> 
>> diff --git a/gnu/packages/grub.scm b/gnu/packages/grub.scm
>> index ffce1bf..e06216f 100644
>> --- a/gnu/packages/grub.scm
>> +++ b/gnu/packages/grub.scm
>> @@ -157,3 +157,24 @@ on the same computer; upon booting the computer, the user is presented with a
>>  menu to select one of the installed operating systems.")
>>      (license gpl3+)
>>      (properties '((cpe-name . "grub2")))))
>> +
>> +(define-public grub-efi
>> +  (package
>> +    (inherit grub)
>> +    (name "grub-efi")
>> +    (synopsis (string-append (package-synopsis grub) " (UEFI version)"))
>> +    (inputs
>> +     `(("efibootmgr" ,efibootmgr)
>> +       ,@(package-inputs grub)))
>> +    (arguments
>> +     #:tests? #f ; FIXME: 40 failures, 24 skipped, 17 passed.
>
> Does this package work for you?

Oops, not sure what went wrong when fixing up this package for
publishing. Updated patch attached.

The tests are the same as the original grub package, so I don't get why
they are failing now.
Ludovic Courtès - Nov. 6, 2016, 10 p.m.
Marius Bakke <mbakke@fastmail.com> skribis:

> Leo Famulari <leo@famulari.name> writes:
>
>> On Sat, Nov 05, 2016 at 12:55:11PM +0000, Marius Bakke wrote:
>>> * gnu/packages/grub.scm (grub-efi): New variable.
>>> ---
>>>  gnu/packages/grub.scm | 21 +++++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>> 
>>> diff --git a/gnu/packages/grub.scm b/gnu/packages/grub.scm
>>> index ffce1bf..e06216f 100644
>>> --- a/gnu/packages/grub.scm
>>> +++ b/gnu/packages/grub.scm
>>> @@ -157,3 +157,24 @@ on the same computer; upon booting the computer, the user is presented with a
>>>  menu to select one of the installed operating systems.")
>>>      (license gpl3+)
>>>      (properties '((cpe-name . "grub2")))))
>>> +
>>> +(define-public grub-efi
>>> +  (package
>>> +    (inherit grub)
>>> +    (name "grub-efi")
>>> +    (synopsis (string-append (package-synopsis grub) " (UEFI version)"))
>>> +    (inputs
>>> +     `(("efibootmgr" ,efibootmgr)
>>> +       ,@(package-inputs grub)))
>>> +    (arguments
>>> +     #:tests? #f ; FIXME: 40 failures, 24 skipped, 17 passed.
>>
>> Does this package work for you?
>
> Oops, not sure what went wrong when fixing up this package for
> publishing. Updated patch attached.

I think Leo was asking whether you could get a bootable system with it.
:-)

> From 940c03c7dcddec019e27f6eb1470aeab4db57799 Mon Sep 17 00:00:00 2001
> From: Marius Bakke <mbakke@fastmail.com>
> Date: Thu, 20 Oct 2016 17:26:52 +0100
> Subject: [PATCH] gnu: Add grub-efi.
>
> * gnu/packages/grub.scm (grub-efi): New variable.

[...]

> +    (name "grub-efi")
> +    (synopsis (string-append (package-synopsis grub) " (UEFI version)"))

Please use a literal string for ‘synopsis’; use of ‘string-append’ like
this prevents i18n.

> +     `(#:tests? #f ; FIXME: 40 failures, 24 skipped, 17 passed.

It would be good to investigate, especially if the tests pass in the
‘grub’ package.

Also, what’s the rationale for making ‘grub-efi’ separate instead of
incorporating the changes in ‘grub’ proper?  Are there issues around the
portability of ‘efibootmgr’, or an increased closure size?

Thanks for working on it!

Ludo’.
Marius Bakke - Nov. 7, 2016, 12:19 a.m.
Ludovic Courtès <ludo@gnu.org> writes:

> Marius Bakke <mbakke@fastmail.com> skribis:
>
>> Leo Famulari <leo@famulari.name> writes:
>>
>>> On Sat, Nov 05, 2016 at 12:55:11PM +0000, Marius Bakke wrote:
>>>> * gnu/packages/grub.scm (grub-efi): New variable.
>>>> ---
>>>>  gnu/packages/grub.scm | 21 +++++++++++++++++++++
>>>>  1 file changed, 21 insertions(+)
>>>> 
>>>> diff --git a/gnu/packages/grub.scm b/gnu/packages/grub.scm
>>>> index ffce1bf..e06216f 100644
>>>> --- a/gnu/packages/grub.scm
>>>> +++ b/gnu/packages/grub.scm
>>>> @@ -157,3 +157,24 @@ on the same computer; upon booting the computer, the user is presented with a
>>>>  menu to select one of the installed operating systems.")
>>>>      (license gpl3+)
>>>>      (properties '((cpe-name . "grub2")))))
>>>> +
>>>> +(define-public grub-efi
>>>> +  (package
>>>> +    (inherit grub)
>>>> +    (name "grub-efi")
>>>> +    (synopsis (string-append (package-synopsis grub) " (UEFI version)"))
>>>> +    (inputs
>>>> +     `(("efibootmgr" ,efibootmgr)
>>>> +       ,@(package-inputs grub)))
>>>> +    (arguments
>>>> +     #:tests? #f ; FIXME: 40 failures, 24 skipped, 17 passed.
>>>
>>> Does this package work for you?
>>
>> Oops, not sure what went wrong when fixing up this package for
>> publishing. Updated patch attached.
>
> I think Leo was asking whether you could get a bootable system with it.

Yes, I'm using this right now, on top of the recent changes to "guix
system" :) There are a couple of other changes necessary for proper UEFI
support: the grub-install command needs "--efi-directory=<boot mount>"
and optionally "--bootloader-id=GNU" (I use these as well, but did not
publish them, since I haven't tested it on a BIOS system yet, and they
probably need to be conditional somehow). 

>> From 940c03c7dcddec019e27f6eb1470aeab4db57799 Mon Sep 17 00:00:00 2001
>> From: Marius Bakke <mbakke@fastmail.com>
>> Date: Thu, 20 Oct 2016 17:26:52 +0100
>> Subject: [PATCH] gnu: Add grub-efi.
>>
>> * gnu/packages/grub.scm (grub-efi): New variable.
>
> [...]
>
>> +    (name "grub-efi")
>> +    (synopsis (string-append (package-synopsis grub) " (UEFI version)"))
>
> Please use a literal string for ‘synopsis’; use of ‘string-append’ like
> this prevents i18n.
>
>> +     `(#:tests? #f ; FIXME: 40 failures, 24 skipped, 17 passed.
>
> It would be good to investigate, especially if the tests pass in the
> ‘grub’ package.
>
> Also, what’s the rationale for making ‘grub-efi’ separate instead of
> incorporating the changes in ‘grub’ proper?  Are there issues around the
> portability of ‘efibootmgr’, or an increased closure size?

This is a good point. The only difference with "--with-platform=efi" is
that another directory is created in place of the i386-pc directory. It
is perfectly possible to build multiple platforms and copying the
platform-specific stuff to $out/lib -- grub will pick the correct
platform at runtime. This is what the Gentoo ebuild does.

I'll give this a shot and send an updated patch later this week.
Ludovic Courtès - Nov. 7, 2016, 9 a.m.
Marius Bakke <mbakke@fastmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>>> Oops, not sure what went wrong when fixing up this package for
>>> publishing. Updated patch attached.
>>
>> I think Leo was asking whether you could get a bootable system with it.
>
> Yes, I'm using this right now, on top of the recent changes to "guix
> system" :) There are a couple of other changes necessary for proper UEFI
> support: the grub-install command needs "--efi-directory=<boot mount>"
> and optionally "--bootloader-id=GNU" (I use these as well, but did not
> publish them, since I haven't tested it on a BIOS system yet, and they
> probably need to be conditional somehow). 

OK, sounds good.  :-)

>>> From 940c03c7dcddec019e27f6eb1470aeab4db57799 Mon Sep 17 00:00:00 2001
>>> From: Marius Bakke <mbakke@fastmail.com>
>>> Date: Thu, 20 Oct 2016 17:26:52 +0100
>>> Subject: [PATCH] gnu: Add grub-efi.
>>>
>>> * gnu/packages/grub.scm (grub-efi): New variable.
>>
>> [...]
>>
>>> +    (name "grub-efi")
>>> +    (synopsis (string-append (package-synopsis grub) " (UEFI version)"))
>>
>> Please use a literal string for ‘synopsis’; use of ‘string-append’ like
>> this prevents i18n.
>>
>>> +     `(#:tests? #f ; FIXME: 40 failures, 24 skipped, 17 passed.
>>
>> It would be good to investigate, especially if the tests pass in the
>> ‘grub’ package.
>>
>> Also, what’s the rationale for making ‘grub-efi’ separate instead of
>> incorporating the changes in ‘grub’ proper?  Are there issues around the
>> portability of ‘efibootmgr’, or an increased closure size?
>
> This is a good point. The only difference with "--with-platform=efi" is
> that another directory is created in place of the i386-pc directory. It
> is perfectly possible to build multiple platforms and copying the
> platform-specific stuff to $out/lib -- grub will pick the correct
> platform at runtime. This is what the Gentoo ebuild does.

Are you saying that a GRUB compiled with UEFI support will no longer
work out-of-the-box on non-UEFI machines, unless platform-specific stuff
is moved like you suggest?

Thanks,
Ludo’.
Marius Bakke - Nov. 7, 2016, 9:23 a.m.
Ludovic Courtès <ludo@gnu.org> writes:

>>>> From 940c03c7dcddec019e27f6eb1470aeab4db57799 Mon Sep 17 00:00:00 2001
>>>> From: Marius Bakke <mbakke@fastmail.com>
>>>> Date: Thu, 20 Oct 2016 17:26:52 +0100
>>>> Subject: [PATCH] gnu: Add grub-efi.
>>>>
>>>> * gnu/packages/grub.scm (grub-efi): New variable.
>>>
>>> [...]
>>>
>>>> +    (name "grub-efi")
>>>> +    (synopsis (string-append (package-synopsis grub) " (UEFI version)"))
>>>
>>> Please use a literal string for ‘synopsis’; use of ‘string-append’ like
>>> this prevents i18n.
>>>
>>>> +     `(#:tests? #f ; FIXME: 40 failures, 24 skipped, 17 passed.
>>>
>>> It would be good to investigate, especially if the tests pass in the
>>> ‘grub’ package.
>>>
>>> Also, what’s the rationale for making ‘grub-efi’ separate instead of
>>> incorporating the changes in ‘grub’ proper?  Are there issues around the
>>> portability of ‘efibootmgr’, or an increased closure size?
>>
>> This is a good point. The only difference with "--with-platform=efi" is
>> that another directory is created in place of the i386-pc directory. It
>> is perfectly possible to build multiple platforms and copying the
>> platform-specific stuff to $out/lib -- grub will pick the correct
>> platform at runtime. This is what the Gentoo ebuild does.
>
> Are you saying that a GRUB compiled with UEFI support will no longer
> work out-of-the-box on non-UEFI machines, unless platform-specific stuff
> is moved like you suggest?

Ha, no, it was just a long-winded and intoxicated way of saying what you
proposed should work fine. :)

The platform-specific stuff ends up in $out/lib/<platform> by default,
so there won't be any conflicts there, and grub-install will pick the
correct one automatically. That approach is better, I think. Will have a
go at it.

The tricky part is invoking the 'configure, 'build and 'install steps
with appropriate arguments for each platform in the grub expression, but
that does not sound too difficult.
Ludovic Courtès - Dec. 16, 2016, 5:09 p.m.
Hi Marius,

Marius Bakke <mbakke@fastmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>>>>> From 940c03c7dcddec019e27f6eb1470aeab4db57799 Mon Sep 17 00:00:00 2001
>>>>> From: Marius Bakke <mbakke@fastmail.com>
>>>>> Date: Thu, 20 Oct 2016 17:26:52 +0100
>>>>> Subject: [PATCH] gnu: Add grub-efi.
>>>>>
>>>>> * gnu/packages/grub.scm (grub-efi): New variable.
>>>>
>>>> [...]
>>>>
>>>>> +    (name "grub-efi")
>>>>> +    (synopsis (string-append (package-synopsis grub) " (UEFI version)"))
>>>>
>>>> Please use a literal string for ‘synopsis’; use of ‘string-append’ like
>>>> this prevents i18n.
>>>>
>>>>> +     `(#:tests? #f ; FIXME: 40 failures, 24 skipped, 17 passed.
>>>>
>>>> It would be good to investigate, especially if the tests pass in the
>>>> ‘grub’ package.
>>>>
>>>> Also, what’s the rationale for making ‘grub-efi’ separate instead of
>>>> incorporating the changes in ‘grub’ proper?  Are there issues around the
>>>> portability of ‘efibootmgr’, or an increased closure size?
>>>
>>> This is a good point. The only difference with "--with-platform=efi" is
>>> that another directory is created in place of the i386-pc directory. It
>>> is perfectly possible to build multiple platforms and copying the
>>> platform-specific stuff to $out/lib -- grub will pick the correct
>>> platform at runtime. This is what the Gentoo ebuild does.
>>
>> Are you saying that a GRUB compiled with UEFI support will no longer
>> work out-of-the-box on non-UEFI machines, unless platform-specific stuff
>> is moved like you suggest?
>
> Ha, no, it was just a long-winded and intoxicated way of saying what you
> proposed should work fine. :)

It turns out I have an immediate need ;-), so I pushed this as commit
3eee16130d858ae96510ec1c7d38d31290de2699.  Let me know if that doesn’t
seem right!

Now there are things I didn’t quite get.  Apparently you’re supposed to
have a /boot/efi as a vfat partition, and ‘grub-install’ is supposed to
detect it and install the EFI stuff, or so I thought (info "(grub)
Installing GRUB using grub-install").

However, ‘grub-install’ still seems to be installing for “i386-pc”
instead of EFI.

What am I missing?

Thanks!

Ludo’.
Danny Milosavljevic - Dec. 16, 2016, 5:16 p.m.
Did you add (grub grub-efi) in your <grub-configuration> in your system config? Or another package with --with-platform=efi ?

Patch

From 940c03c7dcddec019e27f6eb1470aeab4db57799 Mon Sep 17 00:00:00 2001
From: Marius Bakke <mbakke@fastmail.com>
Date: Thu, 20 Oct 2016 17:26:52 +0100
Subject: [PATCH] gnu: Add grub-efi.

* gnu/packages/grub.scm (grub-efi): New variable.
---
 gnu/packages/grub.scm | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/gnu/packages/grub.scm b/gnu/packages/grub.scm
index ffce1bf..7dcfc47 100644
--- a/gnu/packages/grub.scm
+++ b/gnu/packages/grub.scm
@@ -157,3 +157,24 @@  on the same computer; upon booting the computer, the user is presented with a
 menu to select one of the installed operating systems.")
     (license gpl3+)
     (properties '((cpe-name . "grub2")))))
+
+(define-public grub-efi
+  (package
+    (inherit grub)
+    (name "grub-efi")
+    (synopsis (string-append (package-synopsis grub) " (UEFI version)"))
+    (inputs
+     `(("efibootmgr" ,efibootmgr)
+       ,@(package-inputs grub)))
+    (arguments
+     `(#:tests? #f ; FIXME: 40 failures, 24 skipped, 17 passed.
+       ,@(substitute-keyword-arguments (package-arguments grub)
+           ((#:configure-flags flags) `(cons* "--with-platform=efi"
+                                              ,flags))
+           ((#:phases phases)
+            `(modify-phases ,phases
+               (add-after 'patch-stuff 'patch-efibootmgr-path
+                 (lambda* (#:key inputs #:allow-other-keys)
+                   (substitute* "grub-core/osdep/unix/platform.c"
+                     (("efibootmgr") (string-append (assoc-ref inputs "efibootmgr")
+                                                    "/sbin/efibootmgr"))))))))))))
-- 
2.10.2