bug#36876: guix system delete-generations removes custom boot menu entries

bug#36876: guix system delete-generations removes custom boot menu entries

[Jesse Gibbons]

I dual-booted Guix with another gnu/linux-libre distro.
My configuration includes the other distro in the grub menu. When I run
"sudo guix system delete-generations" the changes to the grub menu drop
the other distro with the older system generations of guix.

My current work-around for this is to run "guix system reconfigure ..."
which includes the boot menu entries specified in the configuration.

bug#36876: guix system delete-generations removes custom boot menu entries

[Jakob L. Kreuze]

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

Hi Jesse,

Jesse Gibbons <jgibbons2357@gmail.com> writes:

> I dual-booted Guix with another gnu/linux-libre distro.
> My configuration includes the other distro in the grub menu. When I run
> "sudo guix system delete-generations" the changes to the grub menu drop
> the other distro with the older system generations of guix.
>
> My current work-around for this is to run "guix system reconfigure ..."
> which includes the boot menu entries specified in the configuration.

Thanks for reporting this; it's a rather serious issue. The problem lies
in the 'reinstall-bootloader' procedure. Chiefly, it uses the default
bootloader configuration for whatever it can find using
'lookup-bootloader-by-name' and generates menu entries for the
generations reachable from '%system-profile', which is quite a bit
different from how 'guix system reconfigure' produces the bootloader
configuration. It really isn't ideal. To quote a comment in
'system.scm': "[i]t will be enough to allow the system to boot."

I don't think this should be _too_ hard to fix. To me, parsing the
installed Grub configuration to get existing menu entries seems like a
logical step forward.

Thoughts from anyone else?

Regards,
Jakob

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

bug#36876: guix system delete-generations removes custom boot menu entries

[Jesse Gibbons]

On Mon, 2019-08-05 at 12:05 -0400, Jakob L. Kreuze wrote:
> ...
> 
> I don't think this should be _too_ hard to fix. To me, parsing the
> installed Grub configuration to get existing menu entries seems like
> a
> logical step forward.
> 
> Thoughts from anyone else?
> 
> Regards,
> Jakob

Alternatively, we could save in the store a derivation for the the grub
config generated from the bootloader of the configuration. When the
user calls "guix system delete-generations", the derivation can be run,
and the remaining system generations (if any) can be appended in a menu
like when the user calls "guix system reconfigure". (Although it does
not work for me right now, I'm assuming "guix system delete-generations 
2m" as described in the manual will be implemented in the future.)

The immediate downsides I see to my solution:

- It would take space in the store per generation, which can add up if
the user does not often call "guix system delete-generations" and calls
"guix system reconfigure" on a healthy basis. The user could just be
reminded to call "guix system delete-generations" occasionally, and any
 official service that automatically updates the system via "guix
system reconfigure" can (and considering how large a generation with a
lot of updated system packages can get, probably should) also be
configured to call "guix system delete-generations".

- If someone hand-edits the grub config the changes would be lost. This
is the case as it is right now, and grub options can be edited in the
configuration, so I'm not too concerned about this.

-It would be much simpler to identify menu entries generated by guix
that are no longer in the store and remove them, and remove all empty
submenus. Parsing would make hand-editing grub.cfg more dangerous than
a solution that simply scraps the hand-made changes and rebuilds as I
propose, because the user doing the hand-editing would not necessarily
be aware what patterns the parser checks. It would also be inconsitent:
edits to grub.cfg being scrapped when "guix system reconfigure" is
called, but not when 'guix system delete-generations" is called looks
to me like a good way to introduce a bug to the more adventurous
"Murphy's Law"-type users down the road.


These are just my thoughts. I would love to hear other downsides to my
solution.

-- 
-Jesse

bug#36876: guix system delete-generations removes custom boot menu entries

[Jakob L. Kreuze]

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

Jesse Gibbons <jgibbons2357@gmail.com> writes:

> Alternatively, we could save in the store a derivation for the the grub
> config generated from the bootloader of the configuration. When the
> user calls "guix system delete-generations", the derivation can be run,
> and the remaining system generations (if any) can be appended in a menu
> like when the user calls "guix system reconfigure". (Although it does
> not work for me right now, I'm assuming "guix system delete-generations 
> 2m" as described in the manual will be implemented in the future.)
>
> The immediate downsides I see to my solution:
>
> - It would take space in the store per generation, which can add up if
> the user does not often call "guix system delete-generations" and calls
> "guix system reconfigure" on a healthy basis. The user could just be
> reminded to call "guix system delete-generations" occasionally, and any
>  official service that automatically updates the system via "guix
> system reconfigure" can (and considering how large a generation with a
> lot of updated system packages can get, probably should) also be
> configured to call "guix system delete-generations".
>
> - If someone hand-edits the grub config the changes would be lost. This
> is the case as it is right now, and grub options can be edited in the
> configuration, so I'm not too concerned about this.
>
> -It would be much simpler to identify menu entries generated by guix
> that are no longer in the store and remove them, and remove all empty
> submenus. Parsing would make hand-editing grub.cfg more dangerous than
> a solution that simply scraps the hand-made changes and rebuilds as I
> propose, because the user doing the hand-editing would not necessarily
> be aware what patterns the parser checks. It would also be
> inconsitent: edits to grub.cfg being scrapped when "guix system
> reconfigure" is called, but not when 'guix system delete-generations"
> is called looks to me like a good way to introduce a bug to the more
> adventurous "Murphy's Law"-type users down the road.

I haven't tried it yet, but 'menuentry' seems as though it would be a
fairly simple structure to parse.

> These are just my thoughts. I would love to hear other downsides to my
> solution.

I prefer your suggestion to mine and think that the benefits may
outweigh the costs in this case. This seems to fit into the Guix idea of
purity a bit better than parsing a grub.cfg that may have been
overwritten by another system.

Regards,
Jakob

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

bug#36876: guix system delete-generations removes custom boot menu entries

[Danny Milosavljevic]

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

Hi Jakob,

keep in mind that switching back to a previous generation could also entail a
switch of bootloader projects.  It does you no good to parse the grub config to
find menu entries and then append them to u-boot.

Fundamentally, it would be best if either the bootloader is part of the state
that guix manages (and each time uses), or it isn't; not both.

Current situation:

* If you select a previous generation in the boot menu, then the state that is
selected does NOT include the bootloader (i.e. it doesn't change the bootloader
or the bootloader config).

* If you do "guix system reconfigure", the state that is saved includes only
the bootloader in the boot sector (simplified) but not the bootloader installer
or the bootloader derivation.

* If you do "guix system delete-generations", the state that is restored does
not include the bootloader installer and previous bootloader configuration.

Clearly, it's not nice to have these different things happen.

It would be better if we retained the generation's bootloader installer
and bootloader config and reinstalled it on each of those.

So, I would suggest to retain the following for each system generation:

* The bootloader package derivation
* The bootloader config derivation
* The bootloader installer derivation

And to install the bootloader each time using the latter.

This introduces cycles--not sure whether that's a problem or not.
For the GC I think it's not a problem.

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

bug#36876: guix system delete-generations removes custom boot menu entries

[Danny Milosavljevic]

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

Addition:

* If you do "guix system switch-generation", the state that is restored does
not include the bootloader installer and previous bootloader configuration.

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

bug#36876: guix system delete-generations removes custom boot menu entries

[Jakob L. Kreuze]

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

Hi Danny,

Danny Milosavljevic <dannym@scratchpost.org> writes:

> Hi Jakob,
>
> keep in mind that switching back to a previous generation could also entail a
> switch of bootloader projects.  It does you no good to parse the grub config to
> find menu entries and then append them to u-boot.
>
> Fundamentally, it would be best if either the bootloader is part of the state
> that guix manages (and each time uses), or it isn't; not both.
>
> Current situation:
>
> * If you select a previous generation in the boot menu, then the state that is
> selected does NOT include the bootloader (i.e. it doesn't change the bootloader
> or the bootloader config).
>
> * If you do "guix system reconfigure", the state that is saved includes only
> the bootloader in the boot sector (simplified) but not the bootloader installer
> or the bootloader derivation.
>
> * If you do "guix system delete-generations", the state that is restored does
> not include the bootloader installer and previous bootloader configuration.
>
> Clearly, it's not nice to have these different things happen.
>
> It would be better if we retained the generation's bootloader installer
> and bootloader config and reinstalled it on each of those.
>
> So, I would suggest to retain the following for each system generation:
>
> * The bootloader package derivation
> * The bootloader config derivation
> * The bootloader installer derivation
>
> And to install the bootloader each time using the latter.
>
> This introduces cycles--not sure whether that's a problem or not.
> For the GC I think it's not a problem.

Thank you for your comments -- after reading this and Jesse's
suggestion, I'm quite confident that this is the way to go about it.

Regards,
Jakob

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

bug#36876: guix system delete-generations removes custom boot menu entries

[Ludovic Courtès]

Hello!

zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) skribis:

> Jesse Gibbons <jgibbons2357@gmail.com> writes:
>
>> I dual-booted Guix with another gnu/linux-libre distro.
>> My configuration includes the other distro in the grub menu. When I run
>> "sudo guix system delete-generations" the changes to the grub menu drop
>> the other distro with the older system generations of guix.
>>
>> My current work-around for this is to run "guix system reconfigure ..."
>> which includes the boot menu entries specified in the configuration.
>
> Thanks for reporting this; it's a rather serious issue. The problem lies
> in the 'reinstall-bootloader' procedure. Chiefly, it uses the default
> bootloader configuration for whatever it can find using
> 'lookup-bootloader-by-name' and generates menu entries for the
> generations reachable from '%system-profile', which is quite a bit
> different from how 'guix system reconfigure' produces the bootloader
> configuration. It really isn't ideal. To quote a comment in
> 'system.scm': "[i]t will be enough to allow the system to boot."
>
> I don't think this should be _too_ hard to fix. To me, parsing the
> installed Grub configuration to get existing menu entries seems like a
> logical step forward.

I agree with Danny here that parsing the GRUB config wouldn’t be great.

We have information about the user’s extra menu entries.  The issue, as
I see it, as that this information is lost once the system is
instantiated.

But!  We have the <boot-parameters> structure, that gets serialized with
the system, and which we could extend with those extra menu entries.
That way, the info would be preserved, and we can restore them upon
‘delete-generations’.  <menu-entry> records are bootloader-independent,
which is good.

How does that sound?

Thanks,
Ludo’.

bug#36876: guix system delete-generations removes custom boot menu entries

[Jakob L. Kreuze]

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

Hi Ludo,

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

> I agree with Danny here that parsing the GRUB config wouldn’t be great.
>
> We have information about the user’s extra menu entries.  The issue, as
> I see it, as that this information is lost once the system is
> instantiated.
>
> But!  We have the <boot-parameters> structure, that gets serialized with
> the system, and which we could extend with those extra menu entries.
> That way, the info would be preserved, and we can restore them upon
> ‘delete-generations’.  <menu-entry> records are bootloader-independent,
> which is good.
>
> How does that sound?

Would that involve appending an additional field to <boot-parameters>
for storing the previous entries? I think that would have the pleasant
side-effect of making the code for deployment/reconfiguration simpler :)

Regards,
Jakob

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

bug#36876: guix system delete-generations removes custom boot menu entries

[Ludovic Courtès]

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

Hello,

zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> I agree with Danny here that parsing the GRUB config wouldn’t be great.
>>
>> We have information about the user’s extra menu entries.  The issue, as
>> I see it, as that this information is lost once the system is
>> instantiated.
>>
>> But!  We have the <boot-parameters> structure, that gets serialized with
>> the system, and which we could extend with those extra menu entries.
>> That way, the info would be preserved, and we can restore them upon
>> ‘delete-generations’.  <menu-entry> records are bootloader-independent,
>> which is good.
>>
>> How does that sound?
>
> Would that involve appending an additional field to <boot-parameters>
> for storing the previous entries? I think that would have the pleasant
> side-effect of making the code for deployment/reconfiguration simpler :)

Oh that’d be nice.

The attached patches should fix this.  I’ve successfully deleted a
generation on my system. :-)  I don’t have extra menu entries though, so
it’d be great if you could give it a try.

Thanks,
Ludo’.


[-- Attachment #2: 0001-system-Add-bootloader-menu-entries-field-to-boot-par.patch --]
[-- Type: text/x-patch, Size: 5639 bytes --]

From 0735c71707a5ea14e43e9adf6125801afa7db1ce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Wed, 28 Aug 2019 23:27:20 +0200
Subject: [PATCH 1/2] system: Add 'bootloader-menu-entries' field to
 <boot-parameters>.

This allows us to keep track of the extra menu entries specified in the
OS configuration.

* gnu/system.scm (<boot-parameters>)[bootloader-menu-entries]: New field.
(read-boot-parameters): Initialize it.
(operating-system-boot-parameters): Likewise.
(operating-system-boot-parameters-file): Serialize it.
* gnu/bootloader.scm (menu-entry->sexp, sexp->menu-entry): New
procedures.
---
 gnu/bootloader.scm | 34 ++++++++++++++++++++++++++++++++++
 gnu/system.scm     | 15 +++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index a381f67145..03616c12ab 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2017 David Craven <david@craven.ch>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;; Copyright © 2017 Leo Famulari <leo@famulari.name>
+;;; Copyright © 2019 Ludovic Courtès <ludo@gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -23,6 +24,7 @@
   #:use-module (guix records)
   #:use-module (guix ui)
   #:use-module (srfi srfi-1)
+  #:use-module (ice-9 match)
   #:export (menu-entry
             menu-entry?
             menu-entry-label
@@ -32,6 +34,9 @@
             menu-entry-initrd
             menu-entry-device-mount-point
 
+            menu-entry->sexp
+            sexp->menu-entry
+
             bootloader
             bootloader?
             bootloader-name
@@ -76,6 +81,35 @@
                    (default '()))          ; list of string-valued gexps
   (initrd          menu-entry-initrd))     ; file name of the initrd as a gexp
 
+(define (menu-entry->sexp entry)
+  "Return ENTRY serialized as an sexp."
+  (match entry
+    (($ <menu-entry> label device mount-point linux linux-arguments initrd)
+     `(menu-entry (version 0)
+                  (label ,label)
+                  (device ,device)
+                  (device-mount-point ,mount-point)
+                  (linux ,linux)
+                  (linux-arguments ,linux-arguments)
+                  (initrd ,initrd)))))
+
+(define (sexp->menu-entry sexp)
+  "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a <menu-entry>
+record."
+  (match sexp
+    (('menu-entry ('version 0)
+                  ('label label) ('device device)
+                  ('device-mount-point mount-point)
+                  ('linux linux) ('linux-arguments linux-arguments)
+                  ('initrd initrd) _ ...)
+     (menu-entry
+      (label label)
+      (device device)
+      (device-mount-point mount-point)
+      (linux linux)
+      (linux-arguments linux-arguments)
+      (initrd initrd)))))
+
 \f
 ;;;
 ;;; Bootloader record.
diff --git a/gnu/system.scm b/gnu/system.scm
index 01be1243fe..a599ba2750 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -116,6 +116,7 @@
             boot-parameters-label
             boot-parameters-root-device
             boot-parameters-bootloader-name
+            boot-parameters-bootloader-menu-entries
             boot-parameters-store-device
             boot-parameters-store-mount-point
             boot-parameters-kernel
@@ -251,6 +252,8 @@ directly by the user."
   ;; OS's root file system, so it might be a device path like "/dev/sda3".
   (root-device      boot-parameters-root-device)
   (bootloader-name  boot-parameters-bootloader-name)
+  (bootloader-menu-entries                        ;list of <menu-entry>
+   boot-parameters-bootloader-menu-entries)
   (store-device     boot-parameters-store-device)
   (store-mount-point boot-parameters-store-mount-point)
   (kernel           boot-parameters-kernel)
@@ -297,6 +300,11 @@ file system labels."
          ((_ args) args)
          (#f       'grub))) ; for compatibility reasons.
 
+      (bootloader-menu-entries
+       (match (assq 'bootloader-menu-entries rest)
+         ((_ . entries) (map sexp->menu-entry entries))
+         (#f            '())))
+
       ;; In the past, we would store the directory name of the kernel instead
       ;; of the absolute file name of its image.  Detect that and correct it.
       (kernel (if (string=? linux (direct-store-path linux))
@@ -1005,6 +1013,8 @@ such as '--root' and '--load' to <boot-parameters>."
           (operating-system-user-kernel-arguments os)))
      (initrd initrd)
      (bootloader-name bootloader-name)
+     (bootloader-menu-entries
+      (bootloader-configuration-menu-entries (operating-system-bootloader os)))
      (store-device (ensure-not-/dev (file-system-device store)))
      (store-mount-point (file-system-mount-point store)))))
 
@@ -1046,6 +1056,11 @@ being stored into the \"parameters\" file)."
                      #$(boot-parameters-kernel-arguments params))
                     (initrd #$(boot-parameters-initrd params))
                     (bootloader-name #$(boot-parameters-bootloader-name params))
+                    (bootloader-menu-entries
+                     #$(map menu-entry->sexp
+                            (or (and=> (operating-system-bootloader os)
+                                       bootloader-configuration-menu-entries)
+                                '())))
                     (store
                      (device
                       #$(device->sexp (boot-parameters-store-device params)))
-- 
2.23.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-guix-system-Reinstalling-the-bootloader-preserves-ex.patch --]
[-- Type: text/x-patch, Size: 1879 bytes --]

From f0c5d8f1479d899ce5e646f7a157c571c9b6d80c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Wed, 28 Aug 2019 23:31:28 +0200
Subject: [PATCH 2/2] guix system: Reinstalling the bootloader preserves extra
 menu entries.

Fixes <https://bugs.gnu.org/36876>.
Reported by Jesse Gibbons <jgibbons2357@gmail.com>.

Previously 'guix system delete-generations' or 'switch-generation' would
lose the extra bootloader menu entries specified in the current system's
configuration.  This fixes that.

* guix/scripts/system.scm (reinstall-bootloader): Turn PARAMS into a
single <boot-parameters>.  Adjust ENTRIES to include extra menu entries
specified in PARAMS.
---
 guix/scripts/system.scm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm
index 9fc3a10e98..27b014db68 100644
--- a/guix/scripts/system.scm
+++ b/guix/scripts/system.scm
@@ -384,12 +384,14 @@ STORE is an open connection to the store."
                              (bootloader bootloader)))
 
          ;; Make the specified system generation the default entry.
-         (params (profile-boot-parameters %system-profile (list number)))
+         (params (first (profile-boot-parameters %system-profile
+                                                 (list number))))
          (old-generations
           (delv number (reverse (generation-numbers %system-profile))))
          (old-params (profile-boot-parameters
                        %system-profile old-generations))
-         (entries (map boot-parameters->menu-entry params))
+         (entries (cons (boot-parameters->menu-entry params)
+                        (boot-parameters-bootloader-menu-entries params)))
          (old-entries (map boot-parameters->menu-entry old-params)))
     (run-with-store store
       (mlet* %store-monad
-- 
2.23.0

bug#36876: guix system delete-generations removes custom boot menu entries

[Ludovic Courtès]

Hi there!

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

> The attached patches should fix this.  I’ve successfully deleted a
> generation on my system. :-)  I don’t have extra menu entries though, so
> it’d be great if you could give it a try.

So I tried it on my laptop: I added a dummy menu entry, reconfigured,
rebooted, then ran “guix system delete-generations N”, and I confirm
that it preserves the extra menu entry.

There was a typo though:

> --- a/gnu/system.scm
> +++ b/gnu/system.scm
> @@ -116,6 +116,7 @@
>              boot-parameters-label
>              boot-parameters-root-device
>              boot-parameters-bootloader-name
> +            boot-parameters-bootloader-menu-entries
>              boot-parameters-store-device
>              boot-parameters-store-mount-point
>              boot-parameters-kernel
> @@ -251,6 +252,8 @@ directly by the user."
>    ;; OS's root file system, so it might be a device path like "/dev/sda3".
>    (root-device      boot-parameters-root-device)
>    (bootloader-name  boot-parameters-bootloader-name)
> +  (bootloader-menu-entries                        ;list of <menu-entry>
> +   boot-parameters-bootloader-menu-entries)
>    (store-device     boot-parameters-store-device)
>    (store-mount-point boot-parameters-store-mount-point)
>    (kernel           boot-parameters-kernel)
> @@ -297,6 +300,11 @@ file system labels."
>           ((_ args) args)
>           (#f       'grub))) ; for compatibility reasons.
>  
> +      (bootloader-menu-entries
> +       (match (assq 'bootloader-menu-entries rest)
> +         ((_ . entries) (map sexp->menu-entry entries))
                ^
There shouldn’t be a dot here.

I’ll go ahead and push these patches (with this correction) if there are
no objections.

Thanks,
Ludo’.

bug#36876: guix system delete-generations removes custom boot menu entries

[Ludovic Courtès]

Pushed as c3e59de9b1340f1a0ef7e30dd2e4e7bf7b484ee9.

Let me know if anything’s still not quite as expected!

Ludo’.