diff mbox

system: grub: Introduce foreign-menu-entry.

Message ID 20160803064249.7433-1-sleep_walker@gnu.org
State New
Headers show

Commit Message

Tomáš Čech Aug. 3, 2016, 6:42 a.m. UTC
* 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(-)

Comments

Chris Marusich Aug. 3, 2016, 8:48 a.m. UTC | #1
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?
Tomáš Čech Aug. 3, 2016, 9:05 a.m. UTC | #2
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
Chris Marusich Aug. 4, 2016, 6:43 a.m. UTC | #3
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!
diff mbox

Patch

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
+
 
 ;;;
 ;;; 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