[PATCH] system: grub: Introduce foreign-menu-entry.

[PATCH] system: grub: Introduce foreign-menu-entry.

[Tomáš Čech]

* gnu/system/grub(foreign-menu-entry): New record type.

menu-entry type is suitable for kernel and initrd from GuixSD as it is looking
for menu-entry-linux/bzImage for kernel in every case which makes pasing any
other form impossible.

foreign-menu-entry is very similar but is intended for entering other
distributions so it doesn't limit the name and allows kernel and initrd to be
placed in different device (in Grub syntax).
---
 gnu/system/grub.scm | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/gnu/system/grub.scm b/gnu/system/grub.scm
index 45b46ca..9b60dc8 100644
--- a/gnu/system/grub.scm
+++ b/gnu/system/grub.scm
@@ -54,6 +54,9 @@
             menu-entry
             menu-entry?
 
+            foreign-menu-entry
+            foreign-menu-entry?
+
             grub-configuration-file))
 
 ;;; Commentary:
@@ -116,6 +119,16 @@
                    (default '()))          ; list of string-valued gexps
   (initrd          menu-entry-initrd))     ; file name of the initrd as a gexp
 
+(define-record-type* <foreign-menu-entry>
+  foreign-menu-entry make-foreign-menu-entry
+  foreign-menu-entry?
+  (label           foreign-menu-entry-label)
+  (device          foreign-menu-entry-device (default ""))
+  (linux           foreign-menu-entry-linux)
+  (linux-arguments foreign-menu-entry-linux-arguments
+                   (default '()))                  ; list of string-valued gexps
+  (initrd          foreign-menu-entry-initrd))     ; file name of the initrd as a gexp
+
 \f
 ;;;
 ;;; Background image & themes.
@@ -264,7 +277,16 @@ corresponding to old generations of the system."
                                     #~(string-append #$linux "/"
                                                      #$linux-image-name))
                 #$linux #$linux-image-name (string-join (list #$@arguments))
-                #$initrd))))
+                #$initrd))
+     (($ <foreign-menu-entry> label device linux arguments initrd)
+      #~(format port "menuentry ~s {
+  linux ~a ~a
+  initrd ~a
+}~%"
+                #$label
+                (string-append #$device #$linux)
+                (string-join (list #$@arguments))
+                (string-append #$device #$initrd)))))
 
   (mlet %store-monad ((sugar (eye-candy config store-fs system #~port)))
     (define builder
-- 
2.9.2

Re: [PATCH] system: grub: Introduce foreign-menu-entry.

[Chris Marusich]

[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]

Hi Tomáš,

Tomáš Čech <sleep_walker@gnu.org> writes:

> foreign-menu-entry is very similar but is intended for entering other
> distributions so it doesn't limit the name and allows kernel and initrd to be
> placed in different device (in Grub syntax).

What is the use case which motivates this change?  How do you intend to
use this after making the proposed changes?

> +(define-record-type* <foreign-menu-entry>
> +  foreign-menu-entry make-foreign-menu-entry
> +  foreign-menu-entry?
> +  (label           foreign-menu-entry-label)
> +  (device          foreign-menu-entry-device (default ""))
> +  (linux           foreign-menu-entry-linux)
> +  (linux-arguments foreign-menu-entry-linux-arguments
> +                   (default '()))                  ; list of string-valued gexps
> +  (initrd          foreign-menu-entry-initrd))     ; file name of the initrd as a gexp
> +

This is identical to the existing <menu-entry> record type; however, it
adds the "device" parameter:

--8<---------------cut here---------------start------------->8---

(define-record-type* <menu-entry>
  menu-entry make-menu-entry
  menu-entry?
  (label           menu-entry-label)
  (linux           menu-entry-linux)
  (linux-arguments menu-entry-linux-arguments
                   (default '()))          ; list of string-valued gexps
  (initrd          menu-entry-initrd))     ; file name of the initrd as a gexp
--8<---------------cut here---------------end--------------->8---

Would it be possible to accomplish the same thing by adding a "device"
parameter to the existing <menu-entry> record type?  That way, we
wouldn't have to add a new record type which seems largely redundant.

>                                      #~(string-append #$linux "/"
>                                                       #$linux-image-name))
>                  #$linux #$linux-image-name (string-join (list #$@arguments))
> -                #$initrd))))
> +                #$initrd))
> +     (($ <foreign-menu-entry> label device linux arguments initrd)
> +      #~(format port "menuentry ~s {
> +  linux ~a ~a
> +  initrd ~a
> +}~%"
> +                #$label
> +                (string-append #$device #$linux)

Don't we need to insert a separator of some kind between #$device and
#$linux?  Also, don't we need to include the linux image name (e.g.,
"bzImage")?

> +                (string-join (list #$@arguments))
> +                (string-append #$device #$initrd)))))
>  
>    (mlet %store-monad ((sugar (eye-candy config store-fs system #~port)))
>      (define builder

Isn't a separator of some kind needed between #$device and #$initrd?

-- 
Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] system: grub: Introduce foreign-menu-entry.

[Tomáš Čech]

[-- Attachment #1: Type: text/plain, Size: 4495 bytes --]

On Wed, Aug 03, 2016 at 01:48:36AM -0700, Chris Marusich wrote:
>Hi Tomáš,
>
>Tomáš Čech <sleep_walker@gnu.org> writes:
>
>> foreign-menu-entry is very similar but is intended for entering other
>> distributions so it doesn't limit the name and allows kernel and initrd to be
>> placed in different device (in Grub syntax).
>
>What is the use case which motivates this change?  How do you intend to
>use this after making the proposed changes?

I'd like to store configuration of other linux distribution and so far
I haven't found how to override hardcoded bzImage suffix.

With this change I can have configuration:

 (bootloader (grub-configuration
	      (device "/dev/sda")
	      (menu-entries
	       (list
		(foreign-menu-entry
		 (label "openSUSE")
		 (device "(hd0,1)")
		 (linux "/vmlinuz")
		 (linux-arguments (list
				   "root=/dev/venom/opensuse"
				   "init=/usr/lib/systemd/systemd"))
		 (initrd "/initrd")
		 )))))

which will transform to:

menuentry "openSUSE" {
  linux (hd0,1)/vmlinuz root=/dev/venom/opensuse init=/usr/lib/systemd/systemd
  initrd (hd0,1)/initrd
}

I asked on IRC how can I achieve adding such entry but never got answer. So I made a patch.

>
>> +(define-record-type* <foreign-menu-entry>
>> +  foreign-menu-entry make-foreign-menu-entry
>> +  foreign-menu-entry?
>> +  (label           foreign-menu-entry-label)
>> +  (device          foreign-menu-entry-device (default ""))
>> +  (linux           foreign-menu-entry-linux)
>> +  (linux-arguments foreign-menu-entry-linux-arguments
>> +                   (default '()))                  ; list of string-valued gexps
>> +  (initrd          foreign-menu-entry-initrd))     ; file name of the initrd as a gexp
>> +
>
>This is identical to the existing <menu-entry> record type; however, it
>adds the "device" parameter:
>
>--8<---------------cut here---------------start------------->8---
>
>(define-record-type* <menu-entry>
>  menu-entry make-menu-entry
>  menu-entry?
>  (label           menu-entry-label)
>  (linux           menu-entry-linux)
>  (linux-arguments menu-entry-linux-arguments
>                   (default '()))          ; list of string-valued gexps
>  (initrd          menu-entry-initrd))     ; file name of the initrd as a gexp
>--8<---------------cut here---------------end--------------->8---

Your observation is correct. My intention was to introduce new record
type to distinguish between menu-entry and foreign-menu-entry later in
the code. Different type, different interpretation.

>Would it be possible to accomplish the same thing by adding a "device"
>parameter to the existing <menu-entry> record type?  That way, we
>wouldn't have to add a new record type which seems largely redundant.
>
>>                                      #~(string-append #$linux "/"
>>                                                       #$linux-image-name))
>>                  #$linux #$linux-image-name (string-join (list #$@arguments))
>> -                #$initrd))))
>> +                #$initrd))
>> +     (($ <foreign-menu-entry> label device linux arguments initrd)
>> +      #~(format port "menuentry ~s {
>> +  linux ~a ~a
>> +  initrd ~a
>> +}~%"
>> +                #$label
>> +                (string-append #$device #$linux)
>
>Don't we need to insert a separator of some kind between #$device and
>#$linux?  Also, don't we need to include the linux image name (e.g.,
>"bzImage")?

That is the point. User can specify whatever suits him.

For the case with separate boot partition it can look like

  /vmlinuz-4.3.3-5-default

for the case with boot directory on the same partition as different
linux distribution it would look like

  /boot/vmlinuz-4.3.3-5-default

Note that installed kernel in other distribution has never name
'bzImage'.

>
>> +                (string-join (list #$@arguments))
>> +                (string-append #$device #$initrd)))))
>>
>>    (mlet %store-monad ((sugar (eye-candy config store-fs system #~port)))
>>      (define builder
>
>Isn't a separator of some kind needed between #$device and #$initrd?

This kernel or partition with it is not searched, absolute path is
expected, so no.

Code was using match-lambda so defining new type to distinguish the
cases seemed to me natural. Again, I'm new to Guile and I'd like to
see how to do that right. This worked for me as first solution.

Thanks for your review.

S_W

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] system: grub: Introduce foreign-menu-entry.

[Chris Marusich]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

Tomáš Čech <tcech@suse.com> writes:

> I'd like to store configuration of other linux distribution and so far
> I haven't found how to override hardcoded bzImage suffix.
>
> With this change I can have configuration:
>
> (bootloader (grub-configuration
> 	      (device "/dev/sda")
> 	      (menu-entries
> 	       (list
> 		(foreign-menu-entry
> 		 (label "openSUSE")
> 		 (device "(hd0,1)")
> 		 (linux "/vmlinuz")
> 		 (linux-arguments (list
> 				   "root=/dev/venom/opensuse"
> 				   "init=/usr/lib/systemd/systemd"))
> 		 (initrd "/initrd")
> 		 )))))
>
> which will transform to:
>
> menuentry "openSUSE" {
>  linux (hd0,1)/vmlinuz root=/dev/venom/opensuse init=/usr/lib/systemd/systemd
>  initrd (hd0,1)/initrd
> }

I see!  Yes, I agree it would be nice to be able to do that.

> My intention was to introduce new record type to distinguish between
> menu-entry and foreign-menu-entry later in the code. Different type,
> different interpretation.

I see what you're saying, but I think the alternative which Ludo
proposed in the following bug report is cleaner because it re-uses the
existing record type:

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20067

What do you think?

> Code was using match-lambda so defining new type to distinguish the
> cases seemed to me natural. Again, I'm new to Guile and I'd like to
> see how to do that right. This worked for me as first solution.

That's totally understandable.  I'm new to Guile, also, so I might not
always be right, either.  Thank you for taking the time to submit a
patch to improve the project!

-- 
Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

bug#20067: [PATCH] system: grub: Introduce foreign-menu-entry.

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

Hi!

Tomáš Čech <sleep_walker@gnu.org> skribis:

> * gnu/system/grub(foreign-menu-entry): New record type.
>
> menu-entry type is suitable for kernel and initrd from GuixSD as it is looking
> for menu-entry-linux/bzImage for kernel in every case which makes pasing any
> other form impossible.

AIUI, this is a followup to <http://bugs.gnu.org/20067>, and it’s
admittedly a shame that this isn’t fixed!

I still think that the approach proposed at
<http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20067#10> is more
appropriate; ‘menu-entry’ would always work, no duplication would be
necessary.

As a stop-gap measure, I would prefer to (1) allow:

  (menu-entry
    ;; …
    (linux #~(string-append #$kernel "/bzImage")))

(2) remove the “/bzImage” assumption and use the above idiom everywhere
in the current code, and (3) and have a hack along these lines to
correctly interpret (string-append …) in the ‘parameters’ file:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 586 bytes --]

diff --git a/gnu/system.scm b/gnu/system.scm
index d6bf6c4..467d907 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -766,7 +766,11 @@ this file is the reconstruction of GRUB menu entries for old configurations."
      (boot-parameters
       (label label)
       (root-device root)
-      (kernel linux)
+      (kernel (match linux
+                (('string-append (? string? strings) ...)
+                 (string-concatenate strings))
+                (_
+                 linux)))
       (kernel-arguments
        (match (assq 'kernel-arguments rest)
          ((_ args) args)

[-- Attachment #3: Type: text/plain, Size: 34 bytes --]


Thoughts?

Thanks,
Ludo’.

Re: [PATCH] system: grub: Introduce foreign-menu-entry.

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

Hi!

Tomáš Čech <sleep_walker@gnu.org> skribis:

> * gnu/system/grub(foreign-menu-entry): New record type.
>
> menu-entry type is suitable for kernel and initrd from GuixSD as it is looking
> for menu-entry-linux/bzImage for kernel in every case which makes pasing any
> other form impossible.

AIUI, this is a followup to <http://bugs.gnu.org/20067>, and it’s
admittedly a shame that this isn’t fixed!

I still think that the approach proposed at
<http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20067#10> is more
appropriate; ‘menu-entry’ would always work, no duplication would be
necessary.

As a stop-gap measure, I would prefer to (1) allow:

  (menu-entry
    ;; …
    (linux #~(string-append #$kernel "/bzImage")))

(2) remove the “/bzImage” assumption and use the above idiom everywhere
in the current code, and (3) and have a hack along these lines to
correctly interpret (string-append …) in the ‘parameters’ file:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 586 bytes --]

diff --git a/gnu/system.scm b/gnu/system.scm
index d6bf6c4..467d907 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -766,7 +766,11 @@ this file is the reconstruction of GRUB menu entries for old configurations."
      (boot-parameters
       (label label)
       (root-device root)
-      (kernel linux)
+      (kernel (match linux
+                (('string-append (? string? strings) ...)
+                 (string-concatenate strings))
+                (_
+                 linux)))
       (kernel-arguments
        (match (assq 'kernel-arguments rest)
          ((_ args) args)

[-- Attachment #3: Type: text/plain, Size: 34 bytes --]


Thoughts?

Thanks,
Ludo’.

Re: bug#20067: [PATCH] system: grub: Introduce foreign-menu-entry.

[Chris Marusich]

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]

ludo@gnu.org (Ludovic Courtès) writes:

> I still think that the approach proposed at
> <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20067#10> is more
> appropriate; ‘menu-entry’ would always work, no duplication would be
> necessary.
>
> As a stop-gap measure, I would prefer to (1) allow:
>
>   (menu-entry
>     ;; …
>     (linux #~(string-append #$kernel "/bzImage")))
>
> (2) remove the “/bzImage” assumption and use the above idiom everywhere
> in the current code, and (3) and have a hack along these lines to
> correctly interpret (string-append …) in the ‘parameters’ file:
>
>
> diff --git a/gnu/system.scm b/gnu/system.scm
> index d6bf6c4..467d907 100644
> --- a/gnu/system.scm
> +++ b/gnu/system.scm
> @@ -766,7 +766,11 @@ this file is the reconstruction of GRUB menu entries for old configurations."
>       (boot-parameters
>        (label label)
>        (root-device root)
> -      (kernel linux)
> +      (kernel (match linux
> +                (('string-append (? string? strings) ...)
> +                 (string-concatenate strings))
> +                (_
> +                 linux)))
>        (kernel-arguments
>         (match (assq 'kernel-arguments rest)
>           ((_ args) args)
>
>
> Thoughts?

Yes, that approach seems better to me.

-- 
Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

bug#20067: [PATCH] system: grub: Introduce foreign-menu-entry.

[Chris Marusich]

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]

ludo@gnu.org (Ludovic Courtès) writes:

> I still think that the approach proposed at
> <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20067#10> is more
> appropriate; ‘menu-entry’ would always work, no duplication would be
> necessary.
>
> As a stop-gap measure, I would prefer to (1) allow:
>
>   (menu-entry
>     ;; …
>     (linux #~(string-append #$kernel "/bzImage")))
>
> (2) remove the “/bzImage” assumption and use the above idiom everywhere
> in the current code, and (3) and have a hack along these lines to
> correctly interpret (string-append …) in the ‘parameters’ file:
>
>
> diff --git a/gnu/system.scm b/gnu/system.scm
> index d6bf6c4..467d907 100644
> --- a/gnu/system.scm
> +++ b/gnu/system.scm
> @@ -766,7 +766,11 @@ this file is the reconstruction of GRUB menu entries for old configurations."
>       (boot-parameters
>        (label label)
>        (root-device root)
> -      (kernel linux)
> +      (kernel (match linux
> +                (('string-append (? string? strings) ...)
> +                 (string-concatenate strings))
> +                (_
> +                 linux)))
>        (kernel-arguments
>         (match (assq 'kernel-arguments rest)
>           ((_ args) args)
>
>
> Thoughts?

Yes, that approach seems better to me.

-- 
Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]