bug#44196: Problems with /gnu/store in a different btrfs subvolume

bug#44196: Problems with /gnu/store in a different btrfs subvolume

[Miguel Ángel Arruga Vivas]

I've been testing the installation and the use case for separate btrfs
subvolumes, so I created two different btrfs subvolumes for the root
file system (/rootfs) and the store (/storefs), and installed with guix
system init and a basic operating-system configuration.

The problems detected were:

  - [ ] Grub localization doesn't properly work as the root file system
        is not located in the literal route (that I hard-coded based on
        grub defaults).
        This was the main test, I still don't have a patch, but I'm
        thinking that we should generate the locale and provide there
        the store path.  WDYT?
  - [*] The keymap doesn't work on stage2, as the path contains the
        wrong prefix for the store file.  Fixed with patch 1.
  - [?] The store-prefix was not being provided in other places than
        the generation of a new system generation (sic), so
        "guix system delete-generations" generates grub.cfg with wrong
        paths.  This should be fixed with patch 2, but I'm not sure how
        to write another test with a marionette: gnu/tests/installer.scm
        contains most of the code needed, but I'm not sure how to do it.

The patches will follow this email.

bug#44196: [PATCH 1/3] system: Fix grub keymap with store in btrfs subvolume.

[Miguel Ángel Arruga Vivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-system-fix-grub-keymap.patch --]
[-- Type: text/x-patch, Size: 1454 bytes --]

From b310cb18021e421be3256b85ab3f2d8f61fe0ab8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
 <rosen644835@gmail.com>
Date: Sat, 24 Oct 2020 17:48:28 +0200
Subject: [PATCH 1/2] system: Fix grub keymap with store in btrfs subvolume.

* gnu/bootloader/grub.scm (grub-configuration-file)
[keyboard-layout-config]: Use normalize-file.
---
 gnu/bootloader/grub.scm | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index 611580a350..f1479024e6 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -421,11 +421,12 @@ set lang=~a~%" locale))))
                     (bootloader-configuration-bootloader config)))
            (keymap* (and layout
                          (keyboard-layout-file layout #:grub grub)))
+           (entry (first all-entries))
+           (device (menu-entry-device entry))
+           (mount-point (menu-entry-device-mount-point entry))
            (keymap (and keymap*
-                        (if store-directory-prefix
-                            #~(string-append #$store-directory-prefix
-                                             #$keymap*)
-                            keymap*))))
+                        (normalize-file keymap* mount-point
+                                        store-directory-prefix))))
       #~(when #$keymap
           (format port "\
 insmod keylayouts
-- 
2.28.0

bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.

[Miguel Ángel Arruga Vivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0002-system-add-store-directory-prefix.patch --]
[-- Type: text/x-patch, Size: 10170 bytes --]

From 527a9271122f7b83f31dc0b910c6704af81bde66 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
 <rosen644835@gmail.com>
Date: Sat, 24 Oct 2020 18:15:53 +0200
Subject: [PATCH 2/2] system: Add store-directory-prefix to boot-parameters.

* gnu/machine/ssh.scm (roll-back-managed-host): Use
boot-parameters-store-directory-prefix.
* gnu/system.scm (define-module): Export
boot-parameters-store-directory-prefix.
(<boot-parameters>)[store-directory-prefix]: New field.
[boot-parameters-store-directory-prefix]: New accessor.
(read-boot-parameters): Read directory-prefix from store field.
(operating-system-boot-parameters-file): Add directory-prefix to
store field.
* guix/scripts/system.scm (reinstall-bootloader): Use
boot-parameters-store-directory-prefix.
* test/boot-parameters.scm (%default-btrfs-subvolume,
%default-store-directory-prefix): New variables.
(%grub-boot-parameters): Use %default-store-directory-prefix.
(%default-operating-system): Use %default-btrfs-subvolume.
(test-boot-parameters): Add directory-prefix.
(test optional fields): Add test for directory-prefix.
(test os store-directory-prefix): New test.
---
 gnu/machine/ssh.scm       |  3 +++
 gnu/system.scm            | 19 ++++++++++++++++++-
 guix/scripts/system.scm   |  3 +++
 tests/boot-parameters.scm | 23 ++++++++++++++++++++---
 4 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/gnu/machine/ssh.scm b/gnu/machine/ssh.scm
index 5020bd362f..a3a12fb54b 100644
--- a/gnu/machine/ssh.scm
+++ b/gnu/machine/ssh.scm
@@ -482,6 +482,8 @@ an environment type of 'managed-host."
                                         (list (second boot-parameters))))
                        (locale -> (boot-parameters-locale
                                    (second boot-parameters)))
+                       (store-dir -> (boot-parameters-store-directory-prefix
+                                      (second boot-parameters)))
                        (old-entries -> (map boot-parameters->menu-entry
                                             (drop boot-parameters 2)))
                        (bootloader -> (operating-system-bootloader
@@ -492,6 +494,7 @@ an environment type of 'managed-host."
                                     bootloader))
                                   bootloader entries
                                   #:locale locale
+                                  #:store-directory-prefix store-dir
                                   #:old-entries old-entries)))
                        (remote-result (machine-remote-eval machine remote-exp)))
     (when (eqv? 'error remote-result)
diff --git a/gnu/system.scm b/gnu/system.scm
index a3122eaa65..30a5c418d0 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -148,6 +148,7 @@
             boot-parameters-bootloader-name
             boot-parameters-bootloader-menu-entries
             boot-parameters-store-device
+            boot-parameters-store-directory-prefix
             boot-parameters-store-mount-point
             boot-parameters-locale
             boot-parameters-kernel
@@ -299,6 +300,7 @@ directly by the user."
    boot-parameters-bootloader-menu-entries)
   (store-device     boot-parameters-store-device)
   (store-mount-point boot-parameters-store-mount-point)
+  (store-directory-prefix boot-parameters-store-directory-prefix)
   (locale           boot-parameters-locale)
   (kernel           boot-parameters-kernel)
   (kernel-arguments boot-parameters-kernel-arguments)
@@ -394,6 +396,17 @@ file system labels."
           (_                                      ;the old format
            root-device))))
 
+      (store-directory-prefix
+       (match (assq 'store rest)
+         (('store . store-data)
+          (match (assq 'directory-prefix store-data)
+            (('directory-prefix prefix) prefix)
+            ;; No directory-prefix found.
+            (_ #f)))
+         (_
+          ;; No store found, old format.
+          #f)))
+
       (store-mount-point
        (match (assq 'store rest)
          (('store ('device _) ('mount-point mount-point) _ ...)
@@ -1294,6 +1307,7 @@ such as '--root' and '--load' to <boot-parameters>."
   (let* ((initrd          (and (not (operating-system-hurd os))
                                (operating-system-initrd-file os)))
          (store           (operating-system-store-file-system os))
+         (file-systems    (operating-system-file-systems os))
          (locale          (operating-system-locale os))
          (bootloader      (bootloader-configuration-bootloader
                            (operating-system-bootloader os)))
@@ -1315,6 +1329,7 @@ such as '--root' and '--load' to <boot-parameters>."
       (bootloader-configuration-menu-entries (operating-system-bootloader os)))
      (locale locale)
      (store-device (ensure-not-/dev (file-system-device store)))
+     (store-directory-prefix (btrfs-store-subvolume-file-name file-systems))
      (store-mount-point (file-system-mount-point store)))))
 
 (define (device->sexp device)
@@ -1371,7 +1386,9 @@ being stored into the \"parameters\" file)."
                       (device
                        #$(device->sexp (boot-parameters-store-device params)))
                       (mount-point #$(boot-parameters-store-mount-point
-                                      params))))
+                                      params))
+                      (directory-prefix
+                       #$(boot-parameters-store-directory-prefix params))))
                   #:set-load-path? #f)))
 
 (define-gexp-compiler (operating-system-compiler (os <operating-system>)
diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm
index 9ed5c26483..ad998156c2 100644
--- a/guix/scripts/system.scm
+++ b/guix/scripts/system.scm
@@ -385,6 +385,8 @@ STORE is an open connection to the store."
          (params (first (profile-boot-parameters %system-profile
                                                  (list number))))
          (locale (boot-parameters-locale params))
+         (store-directory-prefix
+          (boot-parameters-store-directory-prefix params))
          (old-generations
           (delv number (reverse (generation-numbers %system-profile))))
          (old-params (profile-boot-parameters
@@ -398,6 +400,7 @@ STORE is an open connection to the store."
                      ((bootloader-configuration-file-generator bootloader)
                       bootloader-config entries
                       #:locale locale
+                      #:store-directory-prefix store-directory-prefix
                       #:old-entries old-entries)))
            (drvs -> (list bootcfg)))
         (mbegin %store-monad
diff --git a/tests/boot-parameters.scm b/tests/boot-parameters.scm
index d7e579bc89..a00b227551 100644
--- a/tests/boot-parameters.scm
+++ b/tests/boot-parameters.scm
@@ -46,6 +46,9 @@
 (define %default-initrd (string-append %default-initrd-path "/initrd.cpio.gz"))
 (define %default-root-device (uuid "abcdef12-3456-7890-abcd-ef1234567890"))
 (define %default-store-device (uuid "01234567-89ab-cdef-0123-456789abcdef"))
+(define %default-btrfs-subvolume "testfs")
+(define %default-store-directory-prefix
+  (string-append "/" %default-btrfs-subvolume))
 (define %default-store-mount-point (%store-prefix))
 (define %default-multiboot-modules '())
 (define %default-locale "es_ES.utf8")
@@ -63,6 +66,7 @@
    (multiboot-modules %default-multiboot-modules)
    (locale %default-locale)
    (store-device %default-store-device)
+   (store-directory-prefix %default-store-directory-prefix)
    (store-mount-point %default-store-mount-point)))
 
 (define %default-operating-system
@@ -81,7 +85,10 @@
 		         (file-system
                            (device %default-store-device)
                            (mount-point %default-store-mount-point)
-                           (type "btrfs"))
+                           (type "btrfs")
+                           (options
+                            (string-append "subvol="
+                                           %default-btrfs-subvolume)))
                          %base-file-systems))))
 
 (define (quote-uuid uuid)
@@ -103,6 +110,7 @@
           (with-store #t)
           (store-device
            (quote-uuid %default-store-device))
+          (store-directory-prefix %default-store-directory-prefix)
           (store-mount-point %default-store-mount-point))
   (define (generate-boot-parameters)
     (define (sexp-or-nothing fmt val)
@@ -117,10 +125,12 @@
             (sexp-or-nothing " (kernel-arguments ~S)" kernel-arguments)
             (sexp-or-nothing " (initrd ~S)" initrd)
             (if with-store
-                (format #false " (store~a~a)"
+                (format #false " (store~a~a~a)"
                         (sexp-or-nothing " (device ~S)" store-device)
                         (sexp-or-nothing " (mount-point ~S)"
-                                         store-mount-point))
+                                         store-mount-point)
+                        (sexp-or-nothing " (directory-prefix ~S)"
+                                         store-directory-prefix))
                 "")
             (sexp-or-nothing " (locale ~S)" locale)
             (sexp-or-nothing " (bootloader-name ~a)" bootloader-name)
@@ -149,6 +159,7 @@
        (test-read-boot-parameters #:store-device #false)
        (test-read-boot-parameters #:store-device 'false)
        (test-read-boot-parameters #:store-mount-point #false)
+       (test-read-boot-parameters #:store-directory-prefix #false)
        (test-read-boot-parameters #:multiboot-modules #false)
        (test-read-boot-parameters #:locale #false)
        (test-read-boot-parameters #:bootloader-name #false
@@ -253,4 +264,10 @@
    (operating-system-boot-parameters %default-operating-system
                                      %default-root-device)))
 
+(test-equal "from os, store-directory-prefix"
+  %default-store-directory-prefix
+  (boot-parameters-store-directory-prefix
+   (operating-system-boot-parameters %default-operating-system
+                                     %default-root-device)))
+
 (test-end "boot-parameters")
-- 
2.28.0

bug#44196: [PATCH 3/3] gnu: grub: Add output locale

[Miguel Ángel Arruga Vivas]

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

Hi!

This solves Grub localization too, even though I'm not very happy with
the gexp juggling...  Any idea?

Happy hacking!
Miguel


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-gnu-grub-add-locale-output.patch --]
[-- Type: text/x-patch, Size: 4602 bytes --]

From 333a12f2eff427986efd0ed660fff7d7bb113839 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
 <rosen644835@gmail.com>
Date: Sat, 24 Oct 2020 20:36:21 +0200
Subject: [PATCH 3/3] gnu: grub: Add output locale.

* gnu/bootloader/grub.scm (define-module): Use (guix packages).
(grub-configuration-file)[locale-config]: Use grub:locale output.
* gnu/packages/bootloaders.scm (grub)[outputs]: Define output "locale".
[arguments]: Populate "locale" output with new phase 'install-locale.
---
 gnu/bootloader/grub.scm      | 34 +++++++++++++++++++++++-----------
 gnu/packages/bootloaders.scm | 17 ++++++++++++++++-
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index f1479024e6..fedb609095 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -25,6 +25,7 @@
 
 (define-module (gnu bootloader grub)
   #:use-module (guix build union)
+  #:use-module (guix packages)
   #:use-module (guix records)
   #:use-module (guix store)
   #:use-module (guix utils)
@@ -402,18 +403,29 @@ menuentry ~s {
                  #:port #~port)))
 
   (define locale-config
-    #~(let ((locale #$(and locale
-                           (locale-definition-source
-                            (locale-name->definition locale)))))
-        (when locale
-          (format port "\
+    (let* ((entry (first all-entries))
+           (device (menu-entry-device entry))
+           (bootloader (bootloader-configuration-bootloader config))
+           (grub (bootloader-package bootloader))
+           (locale-dir (normalize-file #~(format #f "~a" #$grub:locale)
+                                       (menu-entry-device-mount-point entry)
+                                       store-directory-prefix)))
+      #~(let ((locale #$(and locale
+                             (locale-definition-source
+                              (locale-name->definition locale)))))
+          (when locale
+            (format port "\
 # Localization configuration.
-if search --file --set boot_partition /grub/grub.cfg; then
-    set locale_dir=(${boot_partition})/grub/locale
-else
-    set locale_dir=/boot/grub/locale
-fi
-set lang=~a~%" locale))))
+~a
+set locale_dir=~a
+set lang=~a~%"
+                    ;; We search an auto-generated file because the
+                    ;; locale name might not match the .mo file name.
+                    #$(grub-root-search device
+                                        #~(string-append #$locale-dir
+                                                         "/en@quot.mo"))
+                    #$locale-dir
+                    locale)))))
 
   (define keyboard-layout-config
     (let* ((layout (bootloader-configuration-keyboard-layout config))
diff --git a/gnu/packages/bootloaders.scm b/gnu/packages/bootloaders.scm
index d1de5cea4e..985b7b89eb 100644
--- a/gnu/packages/bootloaders.scm
+++ b/gnu/packages/bootloaders.scm
@@ -99,6 +99,7 @@
                        "grub-verifiers-Blocklist-fallout-cleanup.patch"
                        "grub-cross-system-i686.patch"))))
     (build-system gnu-build-system)
+    (outputs '("out" "locale"))
     (arguments
      `(#:configure-flags
        ;; Counterintuitively, this *disables* a spurious Python dependency by
@@ -148,7 +149,21 @@
                       (substitute* "Makefile.in"
                         (("test_unset grub_func_test")
                           "test_unset"))
-                      #t)))
+                      #t))
+                  (add-after 'install 'install-locale
+                    ;; Install mo files with the expected names at boot-time.
+                    (lambda* (#:key outputs #:allow-other-keys)
+                      (let ((locale-out (assoc-ref outputs "locale")))
+                        (mkdir-p locale-out)
+                        (for-each (lambda (file)
+                                    (let ((mo (string-append
+                                               (basename file ".gmo")
+                                               ".mo")))
+                                      (copy-file file
+                                                 (string-append locale-out
+                                                                "/" mo))))
+                                  (find-files "po" "\\.gmo$"))
+                        #t))))
        ;; Disable tests on ARM and AARCH64 platforms.
        #:tests? ,(not (any (cute string-prefix? <> (or (%current-target-system)
                                                        (%current-system)))
-- 
2.28.0

bug#44196: [PATCH 3/3 v2] gnu: grub: Add locale output.

[Miguel Ángel Arruga Vivas]

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

This v2 doesn't change the other patches, only this one, because it was
the one I wasn't happy with.  It changes from v1 as the separate output
"locale" is not generated, but instead the mo files are placed into
share/locale with the expected names.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-gnu-grub-add-locale-output.patch --]
[-- Type: text/x-patch, Size: 4367 bytes --]

From c8aff861461b599095d55b9f694e074d433c72bb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
 <rosen644835@gmail.com>
Date: Sat, 24 Oct 2020 20:36:21 +0200
Subject: [PATCH 3/4] gnu: grub: Add locale output.

* gnu/bootloader/grub.scm (define-module): Use (guix packages).
(grub-configuration-file)[locale-config]: Use locale output from grub.
* gnu/packages/bootloaders.scm (grub)[outputs]: Define output "locale".
[arguments]: Populate "locale" output with new phase 'install-locale.
---
 gnu/bootloader/grub.scm      | 35 ++++++++++++++++++++++++-----------
 gnu/packages/bootloaders.scm | 16 +++++++++++++++-
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index f1479024e6..7cce9e1da7 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -25,6 +25,7 @@
 
 (define-module (gnu bootloader grub)
   #:use-module (guix build union)
+  #:use-module (guix packages)
   #:use-module (guix records)
   #:use-module (guix store)
   #:use-module (guix utils)
@@ -402,18 +403,30 @@ menuentry ~s {
                  #:port #~port)))
 
   (define locale-config
-    #~(let ((locale #$(and locale
-                           (locale-definition-source
-                            (locale-name->definition locale)))))
-        (when locale
-          (format port "\
+    (let* ((entry (first all-entries))
+           (device (menu-entry-device entry))
+           (bootloader (bootloader-configuration-bootloader config))
+           (grub (bootloader-package bootloader))
+           (locale-dir (normalize-file (file-append grub "/share/locale")
+                                       (menu-entry-device-mount-point entry)
+                                       store-directory-prefix)))
+      #~(let ((locale #$(and locale
+                             (locale-definition-source
+                              (locale-name->definition locale)))))
+          (when locale
+            (format port "\
 # Localization configuration.
-if search --file --set boot_partition /grub/grub.cfg; then
-    set locale_dir=(${boot_partition})/grub/locale
-else
-    set locale_dir=/boot/grub/locale
-fi
-set lang=~a~%" locale))))
+~a
+set locale_dir=~a
+set lang=~a~%"
+                    #$(grub-root-search device
+                                        ;; We search an auto-generated file
+                                        ;; because the locale name might not
+                                        ;; match the .mo file name.
+                                        (file-append locale-dir
+                                                     "/en@quot.mo"))
+                    #$locale-dir
+                    locale)))))
 
   (define keyboard-layout-config
     (let* ((layout (bootloader-configuration-keyboard-layout config))
diff --git a/gnu/packages/bootloaders.scm b/gnu/packages/bootloaders.scm
index 7034085d67..8057a6dd8a 100644
--- a/gnu/packages/bootloaders.scm
+++ b/gnu/packages/bootloaders.scm
@@ -148,7 +148,21 @@
                       (substitute* "Makefile.in"
                         (("test_unset grub_func_test")
                           "test_unset"))
-                      #t)))
+                      #t))
+                  (add-after 'install 'install-locale-for-boot
+                    ;; Install mo files with the expected names at boot-time.
+                    (lambda* (#:key outputs #:allow-other-keys)
+                      (let* ((out (assoc-ref outputs "out"))
+                             (locale (string-append out "/share/locale")))
+                        (for-each (lambda (file)
+                                    (let ((mo (string-append
+                                               (basename file ".gmo")
+                                               ".mo")))
+                                      (copy-file file
+                                                 (string-append locale
+                                                                "/" mo))))
+                                  (find-files "po" "\\.gmo$"))
+                        #t))))
        ;; Disable tests on ARM and AARCH64 platforms.
        #:tests? ,(not (any (cute string-prefix? <> (or (%current-target-system)
                                                        (%current-system)))
-- 
2.28.0

bug#44196: [PATCH 3/3 v3] system: Do not depend on locale folder generated by grub-install.

[Miguel Ángel Arruga Vivas]

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

This version doesn't modify grub package at all, and it only generates
the folder when needed, and fixes a bug in Grub 2.04 that I didn't
noticed until I started playing with the installer:
https://savannah.gnu.org/bugs/?57889

Happy hacking!
Miguel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-v3-system-do-not-depend-on-locale-folder.patch --]
[-- Type: text/x-patch, Size: 3432 bytes --]

From 770532b931c312edb9f9a075d86dc132b804dc0b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
 <rosen644835@gmail.com>
Date: Sat, 24 Oct 2020 20:36:21 +0200
Subject: [PATCH v3 3/5] system: Do not depend on locale folder generated by
 grub-install.

* gnu/bootloader/grub.scm (define-module): Use (guix packages).
(grub-locale-folder): New computed folder.
(grub-configuration-file)[locale-config]: Use grub-locale-folder only when a
locale is needed.
---
 gnu/bootloader/grub.scm | 50 ++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 11 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index f1479024e6..b610ecbfe5 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -25,6 +25,7 @@
 
 (define-module (gnu bootloader grub)
   #:use-module (guix build union)
+  #:use-module (guix packages)
   #:use-module (guix records)
   #:use-module (guix store)
   #:use-module (guix utils)
@@ -142,6 +143,25 @@ file with the resolution provided in CONFIG."
             (image->png image #:width width #:height height))
            (_ #f)))))
 
+(define (grub-locale-folder grub)
+  "Generate a folder with the locales from GRUB."
+  (define builder
+    #~(begin
+        (use-modules (ice-9 ftw))
+
+        (define (copy-mo lang-path)
+          (let ((file (string-append lang-path "/LC_MESSAGES/guix.mo"))
+                (mo   (string-append (basename lang-path) ".mo")))
+            (when (file-exists? file)
+              (copy-file file mo))))
+
+        (let* ((locale (string-append #$grub "/share/locale"))
+               (langs  (scandir locale)))
+          (mkdir #$output)
+          (chdir #$output)
+          (for-each copy-mo langs))))
+  (computed-file "grub-boot-locale" builder))
+
 (define* (eye-candy config store-device store-mount-point
                     #:key store-directory-prefix port)
   "Return a gexp that writes to PORT (a port-valued gexp) the 'grub.cfg' part
@@ -402,18 +422,26 @@ menuentry ~s {
                  #:port #~port)))
 
   (define locale-config
-    #~(let ((locale #$(and locale
-                           (locale-definition-source
-                            (locale-name->definition locale)))))
-        (when locale
-          (format port "\
+    (let* ((entry (first all-entries))
+           (device (menu-entry-device entry))
+           (mount-point (menu-entry-device-mount-point entry))
+           (bootloader (bootloader-configuration-bootloader config))
+           (grub (bootloader-package bootloader))
+           (locale-dir (grub-locale-folder grub)))
+
+      #~(let ((locale #$(and locale
+                             (locale-definition-source
+                              (locale-name->definition locale))))
+              (locale-dir #$(and locale (grub-locale-folder grub))))
+          (when locale
+            (format port "\
 # Localization configuration.
-if search --file --set boot_partition /grub/grub.cfg; then
-    set locale_dir=(${boot_partition})/grub/locale
-else
-    set locale_dir=/boot/grub/locale
-fi
-set lang=~a~%" locale))))
+search --file --set ~a
+set locale_dir=~a
+set lang=~a~%"
+                    (string-append locale-dir "/es@quot.mo")
+                    locale-dir
+                    locale)))))
 
   (define keyboard-layout-config
     (let* ((layout (bootloader-configuration-keyboard-layout config))
-- 
2.28.0

bug#44196: [PATCH 4/3] system: Fix dependency for grub.cfg generation.

[Miguel Ángel Arruga Vivas]

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

One extra bit :-)

* The keymap was depending on the exported package grub, not on the one
provided by the bootloader configuration, so e.g. systems that use
grub-efi also ended up with grub in their dependency graph.  This patch
solves that issue.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0004-system-Fix-dependency-for-grub.cfg-generation.patch --]
[-- Type: text/x-patch, Size: 1252 bytes --]

From ae2d83f91d522b5b0edfe1abbe88dec0a797235c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
 <rosen644835@gmail.com>
Date: Sun, 25 Oct 2020 10:13:46 +0100
Subject: [PATCH v3 4/5] system: Fix dependency for grub.cfg generation.

* gnu/bootloader/grub.scm (eye-candy)[font-file]: Use the bootloader
package provided with the configuration.
---
 gnu/bootloader/grub.scm | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index b610ecbfe5..8636e9c690 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -191,9 +191,11 @@ fi~%"
                      (symbol->string (assoc-ref colors 'bg)))))
 
   (define font-file
-    (normalize-file (file-append grub "/share/grub/unicode.pf2")
-                    store-mount-point
-                    store-directory-prefix))
+    (let* ((bootloader (bootloader-configuration-bootloader config))
+           (grub (bootloader-package bootloader)))
+      (normalize-file (file-append grub "/share/grub/unicode.pf2")
+                      store-mount-point
+                      store-directory-prefix)))
 
   (define image
     (normalize-file (grub-background-image config)
-- 
2.28.0

bug#44196: [PATCH v4 3/4] system: Do not depend on locale folder generated by

[Miguel Ángel Arruga Vivas]

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

Sorry, the last one didn't generate correctly the locale folder for two
reasons: wrong name for the .mo name, and incomplete path.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v4-0003-system-do-not-depend-on-locale-folder.patch --]
[-- Type: text/x-patch, Size: 3500 bytes --]

From 37ef6594c6db00595b4e3aec6bcaec3eb9b4e1cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
 <rosen644835@gmail.com>
Date: Sat, 24 Oct 2020 20:36:21 +0200
Subject: [PATCH v4 3/5] system: Do not depend on locale folder generated by
 grub-install.

* gnu/bootloader/grub.scm (define-module): Use (guix packages).
(grub-locale-folder): New computed folder.
(grub-configuration-file)[locale-config]: Use grub-locale-folder only when a
locale is needed.
---
 gnu/bootloader/grub.scm | 49 ++++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index f1479024e6..1fe09d01c1 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -25,6 +25,7 @@
 
 (define-module (gnu bootloader grub)
   #:use-module (guix build union)
+  #:use-module (guix packages)
   #:use-module (guix records)
   #:use-module (guix store)
   #:use-module (guix utils)
@@ -142,6 +143,24 @@ file with the resolution provided in CONFIG."
             (image->png image #:width width #:height height))
            (_ #f)))))
 
+(define (grub-locale-folder grub)
+  "Generate a folder with the locales from GRUB."
+  (define builder
+    #~(begin
+        (use-modules (ice-9 ftw))
+        (let ((locale (string-append #$grub "/share/locale"))
+              (out    #$output))
+          (mkdir out)
+          (chdir out)
+          (for-each (lambda (lang)
+                      (let ((file (string-append locale "/" lang
+                                                 "/LC_MESSAGES/grub.mo"))
+                            (dest (string-append lang ".mo")))
+                        (when (file-exists? file)
+                          (copy-file file dest))))
+                    (scandir locale)))))
+  (computed-file "grub-boot-locale" builder))
+
 (define* (eye-candy config store-device store-mount-point
                     #:key store-directory-prefix port)
   "Return a gexp that writes to PORT (a port-valued gexp) the 'grub.cfg' part
@@ -402,18 +421,26 @@ menuentry ~s {
                  #:port #~port)))
 
   (define locale-config
-    #~(let ((locale #$(and locale
-                           (locale-definition-source
-                            (locale-name->definition locale)))))
-        (when locale
-          (format port "\
+    (let* ((entry (first all-entries))
+           (device (menu-entry-device entry))
+           (mount-point (menu-entry-device-mount-point entry))
+           (bootloader (bootloader-configuration-bootloader config))
+           (grub (bootloader-package bootloader))
+           (locale-dir (grub-locale-folder grub)))
+
+      #~(let ((locale #$(and locale
+                             (locale-definition-source
+                              (locale-name->definition locale))))
+              (locale-dir #$(and locale (grub-locale-folder grub))))
+          (when locale
+            (format port "\
 # Localization configuration.
-if search --file --set boot_partition /grub/grub.cfg; then
-    set locale_dir=(${boot_partition})/grub/locale
-else
-    set locale_dir=/boot/grub/locale
-fi
-set lang=~a~%" locale))))
+search --file --set ~a
+set locale_dir=~a
+set lang=~a~%"
+                    (string-append locale-dir "/es@quot.mo")
+                    locale-dir
+                    locale)))))
 
   (define keyboard-layout-config
     (let* ((layout (bootloader-configuration-keyboard-layout config))
-- 
2.28.0

bug#44196: [PATCH v5 3/4] system: Do not depend on locale folder generated by

[Miguel Ángel Arruga Vivas]

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

Changes w.r.t. previous version:
  - Call normalize-file (the parameters were there but I lost the call).
  - Only call search when there is no image configured.

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

From 3eb494947ed863e60d3d49d4ac4a982b1f9237e3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
 <rosen644835@gmail.com>
Date: Sat, 24 Oct 2020 20:36:21 +0200
Subject: [PATCH] system: Do not depend on locale folder generated by
 grub-install.

* gnu/bootloader/grub.scm (define-module): Use (guix packages).
(grub-locale-folder): New computed folder.
(grub-configuration-file)[locale-config]: Use grub-locale-folder only when a
locale is needed.
---
 gnu/bootloader/grub.scm | 56 +++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index 5508319d3b..2604f59117 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -25,6 +25,7 @@
 
 (define-module (gnu bootloader grub)
   #:use-module (guix build union)
+  #:use-module (guix packages)
   #:use-module (guix records)
   #:use-module (guix store)
   #:use-module (guix utils)
@@ -142,6 +143,24 @@ file with the resolution provided in CONFIG."
             (image->png image #:width width #:height height))
            (_ #f)))))
 
+(define (grub-locale-folder grub)
+  "Generate a folder with the locales from GRUB."
+  (define builder
+    #~(begin
+        (use-modules (ice-9 ftw))
+        (let ((locale (string-append #$grub "/share/locale"))
+              (out    #$output))
+          (mkdir out)
+          (chdir out)
+          (for-each (lambda (lang)
+                      (let ((file (string-append locale "/" lang
+                                                 "/LC_MESSAGES/grub.mo"))
+                            (dest (string-append lang ".mo")))
+                        (when (file-exists? file)
+                          (copy-file file dest))))
+                    (scandir locale)))))
+  (computed-file "grub-boot-locale" builder))
+
 (define* (eye-candy config store-device store-mount-point
                     #:key store-directory-prefix port)
   "Return a gexp that writes to PORT (a port-valued gexp) the 'grub.cfg' part
@@ -404,18 +423,33 @@ menuentry ~s {
                  #:port #~port)))
 
   (define locale-config
-    #~(let ((locale #$(and locale
-                           (locale-definition-source
-                            (locale-name->definition locale)))))
-        (when locale
-          (format port "\
+    (let* ((entry (first all-entries))
+           (device (menu-entry-device entry))
+           (mount-point (menu-entry-device-mount-point entry))
+           (bootloader (bootloader-configuration-bootloader config))
+           (grub (bootloader-package bootloader))
+           (locale-dir (normalize-file (grub-locale-folder grub)
+                                       mount-point
+                                       store-directory-prefix)))
+
+      #~(let ((locale #$(and locale
+                             (locale-definition-source
+                              (locale-name->definition locale))))
+              (locale-dir #$(and locale locale-dir)))
+          (when locale
+            (format port "\
 # Localization configuration.
-if search --file --set boot_partition /grub/grub.cfg; then
-    set locale_dir=(${boot_partition})/grub/locale
-else
-    set locale_dir=/boot/grub/locale
-fi
-set lang=~a~%" locale))))
+~asearch --file --set ~a/es@quot.mo
+set locale_dir=~a
+set lang=~a~%"
+                    ;; Skip the search if there is an image, because it is
+                    ;; loaded before this fragment, to avoid the extra search.
+                    #$(if (grub-theme-image (bootloader-theme config))
+                          "# "
+                          "")
+                    locale-dir
+                    locale-dir
+                    locale)))))
 
   (define keyboard-layout-config
     (let* ((layout (bootloader-configuration-keyboard-layout config))
-- 
2.28.0

bug#44196: [PATCH 4/3] system: Fix dependency for grub.cfg generation.

[Danny Milosavljevic]

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

This patch LGTM!

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

bug#44196: [PATCH 1/3] system: Fix grub keymap with store in btrfs subvolume.

[Danny Milosavljevic]

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

Is it possible for there to be no entries in all-entries at all?

If not, LGTM!

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

bug#44196: [PATCH 4/3] system: Fix dependency for grub.cfg generation.

[Miguel Ángel Arruga Vivas]

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

Danny Milosavljevic <dannym@scratchpost.org> writes:
> This patch LGTM!

Thanks for your review, pushed as 222a630e9e.

Happy hacking!
Miguel

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

bug#44196: [PATCH 1/3] system: Fix grub keymap with store in btrfs subvolume.

[Miguel Ángel Arruga Vivas]

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

Hi Danny,

Thank you a lot for taking a look into this. :-)

Danny Milosavljevic <dannym@scratchpost.org> writes:
> Is it possible for there to be no entries in all-entries at all?

This is the idiom used in the rest of the file, and that would be a bug
in the calling code.  The entries are the operating system generations +
the entries configured through the bootloader-configuration, and that
would lead to a grub.cfg without any system to boot.

> If not, LGTM!

Pushed as c69a1c27ee.

Happy hacking!
Miguel

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

bug#44196: [PATCH v6 3/4] system: Do not depend on locale folder generated by

[Miguel Ángel Arruga Vivas]

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

I hope this is the last one, at least for bug fixes.  This version uses
the right name for the autogenerated file en@quot.mo when no image is
provided.

Miguel Ángel Arruga Vivas <rosen644835@gmail.com> writes:
> Changes w.r.t. previous version:
>   - Call normalize-file (the parameters were there but I lost the call).
>   - Only call search when there is no image configured.


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

From 5886bdf74bda59649b3d17b691132d9d030e0fb4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
 <rosen644835@gmail.com>
Date: Sat, 24 Oct 2020 20:36:21 +0200
Subject: [PATCH] system: Do not depend on locale folder generated by
 grub-install.

* gnu/bootloader/grub.scm (define-module): Use (guix packages).
(grub-locale-folder): New computed folder.
(grub-configuration-file)[locale-config]: Use grub-locale-folder only when a
locale is needed.
---
 gnu/bootloader/grub.scm | 56 +++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index 5508319d3b..faf319b747 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -25,6 +25,7 @@
 
 (define-module (gnu bootloader grub)
   #:use-module (guix build union)
+  #:use-module (guix packages)
   #:use-module (guix records)
   #:use-module (guix store)
   #:use-module (guix utils)
@@ -142,6 +143,24 @@ file with the resolution provided in CONFIG."
             (image->png image #:width width #:height height))
            (_ #f)))))
 
+(define (grub-locale-folder grub)
+  "Generate a folder with the locales from GRUB."
+  (define builder
+    #~(begin
+        (use-modules (ice-9 ftw))
+        (let ((locale (string-append #$grub "/share/locale"))
+              (out    #$output))
+          (mkdir out)
+          (chdir out)
+          (for-each (lambda (lang)
+                      (let ((file (string-append locale "/" lang
+                                                 "/LC_MESSAGES/grub.mo"))
+                            (dest (string-append lang ".mo")))
+                        (when (file-exists? file)
+                          (copy-file file dest))))
+                    (scandir locale)))))
+  (computed-file "grub-boot-locale" builder))
+
 (define* (eye-candy config store-device store-mount-point
                     #:key store-directory-prefix port)
   "Return a gexp that writes to PORT (a port-valued gexp) the 'grub.cfg' part
@@ -404,18 +423,33 @@ menuentry ~s {
                  #:port #~port)))
 
   (define locale-config
-    #~(let ((locale #$(and locale
-                           (locale-definition-source
-                            (locale-name->definition locale)))))
-        (when locale
-          (format port "\
+    (let* ((entry (first all-entries))
+           (device (menu-entry-device entry))
+           (mount-point (menu-entry-device-mount-point entry))
+           (bootloader (bootloader-configuration-bootloader config))
+           (grub (bootloader-package bootloader))
+           (locale-dir (normalize-file (grub-locale-folder grub)
+                                       mount-point
+                                       store-directory-prefix)))
+
+      #~(let ((locale #$(and locale
+                             (locale-definition-source
+                              (locale-name->definition locale))))
+              (locale-dir #$(and locale locale-dir)))
+          (when locale
+            (format port "\
 # Localization configuration.
-if search --file --set boot_partition /grub/grub.cfg; then
-    set locale_dir=(${boot_partition})/grub/locale
-else
-    set locale_dir=/boot/grub/locale
-fi
-set lang=~a~%" locale))))
+~asearch --file --set ~a/en@quot.mo
+set locale_dir=~a
+set lang=~a~%"
+                    ;; Skip the search if there is an image, because it is
+                    ;; loaded before this fragment, to avoid the extra search.
+                    #$(if (grub-theme-image (bootloader-theme config))
+                          "# "
+                          "")
+                    locale-dir
+                    locale-dir
+                    locale)))))
 
   (define keyboard-layout-config
     (let* ((layout (bootloader-configuration-keyboard-layout config))
-- 
2.28.0

bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.

[Ludovic Courtès]

Hi!

Miguel Ángel Arruga Vivas <rosen644835@gmail.com> skribis:

> From 527a9271122f7b83f31dc0b910c6704af81bde66 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
>  <rosen644835@gmail.com>
> Date: Sat, 24 Oct 2020 18:15:53 +0200
> Subject: [PATCH 2/2] system: Add store-directory-prefix to boot-parameters.
>
> * gnu/machine/ssh.scm (roll-back-managed-host): Use
> boot-parameters-store-directory-prefix.
> * gnu/system.scm (define-module): Export
> boot-parameters-store-directory-prefix.
> (<boot-parameters>)[store-directory-prefix]: New field.
> [boot-parameters-store-directory-prefix]: New accessor.
> (read-boot-parameters): Read directory-prefix from store field.
> (operating-system-boot-parameters-file): Add directory-prefix to
> store field.
> * guix/scripts/system.scm (reinstall-bootloader): Use
> boot-parameters-store-directory-prefix.
> * test/boot-parameters.scm (%default-btrfs-subvolume,
> %default-store-directory-prefix): New variables.
> (%grub-boot-parameters): Use %default-store-directory-prefix.
> (%default-operating-system): Use %default-btrfs-subvolume.
> (test-boot-parameters): Add directory-prefix.
> (test optional fields): Add test for directory-prefix.
> (test os store-directory-prefix): New test.

[...]

> +++ b/gnu/system.scm
> @@ -148,6 +148,7 @@
>              boot-parameters-bootloader-name
>              boot-parameters-bootloader-menu-entries
>              boot-parameters-store-device
> +            boot-parameters-store-directory-prefix
>              boot-parameters-store-mount-point
>              boot-parameters-locale
>              boot-parameters-kernel
> @@ -299,6 +300,7 @@ directly by the user."
>     boot-parameters-bootloader-menu-entries)
>    (store-device     boot-parameters-store-device)
>    (store-mount-point boot-parameters-store-mount-point)
> +  (store-directory-prefix boot-parameters-store-directory-prefix)

Could you explain why we need ‘store-directory-prefix’ in addition to
‘store-mount-point’?  At first sight, looking at the fields in there,
these two can seem to be synonymous.

Also patch #3 doesn’t depend on it, does it?

Thanks,
Ludo’.

bug#44196: [PATCH v6 3/4] system: Do not depend on locale folder generated by

[Ludovic Courtès]

¡Hola!

Miguel Ángel Arruga Vivas <rosen644835@gmail.com> skribis:

> From 5886bdf74bda59649b3d17b691132d9d030e0fb4 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
>  <rosen644835@gmail.com>
> Date: Sat, 24 Oct 2020 20:36:21 +0200
> Subject: [PATCH] system: Do not depend on locale folder generated by
>  grub-install.
>
> * gnu/bootloader/grub.scm (define-module): Use (guix packages).
> (grub-locale-folder): New computed folder.
> (grub-configuration-file)[locale-config]: Use grub-locale-folder only when a
> locale is needed.

[...]

> +(define (grub-locale-folder grub)
> +  "Generate a folder with the locales from GRUB."

s/folder/directory/  :-)

> +  (define builder
> +    #~(begin
> +        (use-modules (ice-9 ftw))
> +        (let ((locale (string-append #$grub "/share/locale"))
> +              (out    #$output))
> +          (mkdir out)
> +          (chdir out)
> +          (for-each (lambda (lang)
> +                      (let ((file (string-append locale "/" lang
> +                                                 "/LC_MESSAGES/grub.mo"))
> +                            (dest (string-append lang ".mo")))
> +                        (when (file-exists? file)
> +                          (copy-file file dest))))
> +                    (scandir locale)))))
> +  (computed-file "grub-boot-locale" builder))

Maybe just “grub-locales”?

> +    (let* ((entry (first all-entries))
> +           (device (menu-entry-device entry))
> +           (mount-point (menu-entry-device-mount-point entry))
> +           (bootloader (bootloader-configuration-bootloader config))
> +           (grub (bootloader-package bootloader))
> +           (locale-dir (normalize-file (grub-locale-folder grub)
> +                                       mount-point
> +                                       store-directory-prefix)))

Nitpick: maybe s/locale-dir/locales/

> +      #~(let ((locale #$(and locale
> +                             (locale-definition-source
> +                              (locale-name->definition locale))))
> +              (locale-dir #$(and locale locale-dir)))
> +          (when locale
> +            (format port "\
>  # Localization configuration.
> -if search --file --set boot_partition /grub/grub.cfg; then
> -    set locale_dir=(${boot_partition})/grub/locale
> -else
> -    set locale_dir=/boot/grub/locale
> -fi
> -set lang=~a~%" locale))))
> +~asearch --file --set ~a/en@quot.mo
> +set locale_dir=~a
> +set lang=~a~%"

Otherwise LGTM, thanks!

Ludo’.

bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.

[Miguel Ángel Arruga Vivas]

Hi Ludo!

First of all, thanks for the review.

Ludovic Courtès <ludo@gnu.org> writes:
> Could you explain why we need ‘store-directory-prefix’ in addition to
> ‘store-mount-point’?  At first sight, looking at the fields in there,
> these two can seem to be synonymous.

The difference is subtle unless you use btrfs subvolumes... or bind
mounts, which currently probably would fail as the check is only
performed for btrfs partitions---note to self, think about this and
probably prepare a patch.  This check was already implemented, but the
parameter was only provided to grub-configuration-file during
init/reconfigure.

Should I add an explanation of/link the bug on the commit message?  Or
maybe is something like this better?
----
;; Mount point of the store device (as in /etc/fstab's second field)
(store-mount-point boot-parameters-store-mount-point)
;; Actual path of the store inside of the device at boot time.
(store-directory-prefix boot-parameters-store-directory-prefix)
----

In any case, if that doesn't clarify enough, it can be better understood
with an example:
-----
Disk configuration:
/dev/xda1: btrfs file system
  - /rootfs: subvolume mounted on /
  - /gnufs: subvolume mounted on /gnu.

Therefore the serialized boot-parameters should be:
(boot-parameters
  ...
  (store
    (device "/dev/xda1")
    (mount-point "/gnu")
    (directory-prefix "/gnufs"))
  ...)
-----
This way grub.cfg generation is able to `normalize-file' store paths and
transform the user visible file /gnu/store/xxxxx-kernel to the grub
visible path /gnufs/store/xxxx-kernel in the final file.  This is
already provided by the operating-system definition, but the other calls
to the boot configuration generator only rely on the information
provided by boot-parameters.

A simple test case that I haven't implemented yet as a marionette---but
I should anyway---only needs this steps:

1. Create initial os with that kind of disk configuration.
2. Boot on it.
3. Create a second generation.
4. Call guix system delete-generations.
5. Reboot---it doesn't as grub.cfg is wrong.
6. Usual tests.

Tomorrow I won't be able to do much, but this Monday I could try to
implement something like this too if you think it's worth.

> Also patch #3 doesn’t depend on it, does it?

All the patches are independent, as they fix separate issues, but all
are related with bugs detected with my tests with btrfs subvolumes.  I
should have specified that they were a patch set, even though that one
was getting close to become a series... hopefully convergent. :)

Thanks again,
Miguel

bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.

[Miguel Ángel Arruga Vivas]

Writing the last email I though about adding this:
------------------------------------------------------------------------------
   ;; OS's root file system, so it might be a device path like "/dev/sda3".
+  ;; The 'store-directory-prefix' field contains #f or the actual path of
+  ;; the store inside the 'store-device' as seen by GRUB, e.g. it would
+  ;; contain "/storefs" if the store is located in that subvolume of a btrfs
+  ;; partition.
   (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)
+  (store-directory-prefix boot-parameters-store-directory-prefix)
------------------------------------------------------------------------------

WDYT?

Happy hacking!
Miguel

Miguel Ángel Arruga Vivas <rosen644835@gmail.com> writes:

> Hi Ludo!
>
> First of all, thanks for the review.
>
> Ludovic Courtès <ludo@gnu.org> writes:
>> Could you explain why we need ‘store-directory-prefix’ in addition to
>> ‘store-mount-point’?  At first sight, looking at the fields in there,
>> these two can seem to be synonymous.
>
> The difference is subtle unless you use btrfs subvolumes... or bind
> mounts, which currently probably would fail as the check is only
> performed for btrfs partitions---note to self, think about this and
> probably prepare a patch.  This check was already implemented, but the
> parameter was only provided to grub-configuration-file during
> init/reconfigure.
>
> Should I add an explanation of/link the bug on the commit message?  Or
> maybe is something like this better?
> ----
> ;; Mount point of the store device (as in /etc/fstab's second field)
> (store-mount-point boot-parameters-store-mount-point)
> ;; Actual path of the store inside of the device at boot time.
> (store-directory-prefix boot-parameters-store-directory-prefix)
> ----
>
> In any case, if that doesn't clarify enough, it can be better understood
> with an example:
> -----
> Disk configuration:
> /dev/xda1: btrfs file system
>   - /rootfs: subvolume mounted on /
>   - /gnufs: subvolume mounted on /gnu.
>
> Therefore the serialized boot-parameters should be:
> (boot-parameters
>   ...
>   (store
>     (device "/dev/xda1")
>     (mount-point "/gnu")
>     (directory-prefix "/gnufs"))
>   ...)
> -----
> This way grub.cfg generation is able to `normalize-file' store paths and
> transform the user visible file /gnu/store/xxxxx-kernel to the grub
> visible path /gnufs/store/xxxx-kernel in the final file.  This is
> already provided by the operating-system definition, but the other calls
> to the boot configuration generator only rely on the information
> provided by boot-parameters.
>
> A simple test case that I haven't implemented yet as a marionette---but
> I should anyway---only needs this steps:
>
> 1. Create initial os with that kind of disk configuration.
> 2. Boot on it.
> 3. Create a second generation.
> 4. Call guix system delete-generations.
> 5. Reboot---it doesn't as grub.cfg is wrong.
> 6. Usual tests.
>
> Tomorrow I won't be able to do much, but this Monday I could try to
> implement something like this too if you think it's worth.
>
>> Also patch #3 doesn’t depend on it, does it?
>
> All the patches are independent, as they fix separate issues, but all
> are related with bugs detected with my tests with btrfs subvolumes.  I
> should have specified that they were a patch set, even though that one
> was getting close to become a series... hopefully convergent. :)
>
> Thanks again,
> Miguel

bug#44196: [PATCH v6 3/4] system: Do not depend on locale folder generated by

[Miguel Ángel Arruga Vivas]

¡Hola, Ludo!

Thanks for your comments.  Reviewing them I found some more issues that
I address inline:

Ludovic Courtès <ludo@gnu.org> writes:
> Miguel Ángel Arruga Vivas <rosen644835@gmail.com> skribis:
>
>> From 5886bdf74bda59649b3d17b691132d9d030e0fb4 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Miguel=20=C3=81ngel=20Arruga=20Vivas?=
>>  <rosen644835@gmail.com>
>> Date: Sat, 24 Oct 2020 20:36:21 +0200
>> Subject: [PATCH] system: Do not depend on locale folder generated by
>>  grub-install.

I've changed the title to "system: Generate grub locale directory for
grub.cfg.", which is positive, shorter and more descriptive.  I have to
work on my communicative skills a lot...

>>
>> * gnu/bootloader/grub.scm (define-module): Use (guix packages).

This was a leftover from the modification of Grub itself instead of
using computed-file and not needed anymore, removed.

>> (grub-locale-folder): New computed folder.
>> (grub-configuration-file)[locale-config]: Use grub-locale-folder only when a
>> locale is needed.

Changed with the new names and added an explanation about the search.
>
> [...]
>
>> +(define (grub-locale-folder grub)
>> +  "Generate a folder with the locales from GRUB."
>
> s/folder/directory/  :-)

Applied, both in the function name and the documentation string.

>> +  (define builder
>> +    #~(begin
>> +        (use-modules (ice-9 ftw))
>> +        (let ((locale (string-append #$grub "/share/locale"))
>> +              (out    #$output))
>> +          (mkdir out)
>> +          (chdir out)
>> +          (for-each (lambda (lang)
>> +                      (let ((file (string-append locale "/" lang
>> +                                                 "/LC_MESSAGES/grub.mo"))
>> +                            (dest (string-append lang ".mo")))
>> +                        (when (file-exists? file)
>> +                          (copy-file file dest))))
>> +                    (scandir locale)))))
>> +  (computed-file "grub-boot-locale" builder))
>
> Maybe just “grub-locales”?

This name sounds right, applied too. :-)

>> +    (let* ((entry (first all-entries))
>> +           (device (menu-entry-device entry))
>> +           (mount-point (menu-entry-device-mount-point entry))
>> +           (bootloader (bootloader-configuration-bootloader config))
>> +           (grub (bootloader-package bootloader))
>> +           (locale-dir (normalize-file (grub-locale-folder grub)
>> +                                       mount-point
>> +                                       store-directory-prefix)))
>
> Nitpick: maybe s/locale-dir/locales/

Applied too, but I moved this call from the let* to ...

>> +      #~(let ((locale #$(and locale
>> +                             (locale-definition-source
>> +                              (locale-name->definition locale))))
>> +              (locale-dir #$(and locale locale-dir)))

... here, to avoid the generation when there is no locale, which
shouldn't be a common use case, but it shouldn't depend on the
generation of that directory because it won't be used.

>> +          (when locale
>> +            (format port "\
>>  # Localization configuration.
>> -if search --file --set boot_partition /grub/grub.cfg; then
>> -    set locale_dir=(${boot_partition})/grub/locale
>> -else
>> -    set locale_dir=/boot/grub/locale
>> -fi
>> -set lang=~a~%" locale))))
>> +~asearch --file --set ~a/en@quot.mo
>> +set locale_dir=~a
>> +set lang=~a~%"
>
> Otherwise LGTM, thanks!

Thank you again for the review.  Pushed as
f445bc65764ffad2ae9f3b382ddb8feb4eeea2fb with these fixes, after running
again make check and a subset of make check-system.

Happy hacking!
Miguel

bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.

[Miguel Ángel Arruga Vivas]

I've pushed this last patch as 582cf9257cd1f9c969fbba5eb1c336ac8b975cde
with the following additions:
 - A pointer to this issue in the commit message.
 - A description of store-directory-prefix on the commit message.
 - A better version (I hope) of the comment inside <boot-parameters>.

Thank you very much!
Miguel

bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.

[Ludovic Courtès]

Hi!

Miguel Ángel Arruga Vivas <rosen644835@gmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>> Could you explain why we need ‘store-directory-prefix’ in addition to
>> ‘store-mount-point’?  At first sight, looking at the fields in there,
>> these two can seem to be synonymous.
>
> The difference is subtle unless you use btrfs subvolumes... or bind
> mounts, which currently probably would fail as the check is only
> performed for btrfs partitions---note to self, think about this and
> probably prepare a patch.  This check was already implemented, but the
> parameter was only provided to grub-configuration-file during
> init/reconfigure.
>
> Should I add an explanation of/link the bug on the commit message?  Or
> maybe is something like this better?
> ----
> ;; Mount point of the store device (as in /etc/fstab's second field)
> (store-mount-point boot-parameters-store-mount-point)
> ;; Actual path of the store inside of the device at boot time.
> (store-directory-prefix boot-parameters-store-directory-prefix)
> ----
>
> In any case, if that doesn't clarify enough, it can be better understood
> with an example:
> -----
> Disk configuration:
> /dev/xda1: btrfs file system
>   - /rootfs: subvolume mounted on /
>   - /gnufs: subvolume mounted on /gnu.
>
> Therefore the serialized boot-parameters should be:
> (boot-parameters
>   ...
>   (store
>     (device "/dev/xda1")
>     (mount-point "/gnu")
>     (directory-prefix "/gnufs"))
>   ...)
> -----

(Btrfs no0b here.)  Does that mean that /gnu is like a bind-mount of
/gnufs in this case?

Anyway, I think I got it now, but I feel I’ll have to search again for
this example next time I stumble upon it.  ;-)

> Writing the last email I though about adding this:
> ------------------------------------------------------------------------------
>    ;; OS's root file system, so it might be a device path like "/dev/sda3".
> +  ;; The 'store-directory-prefix' field contains #f or the actual path of
> +  ;; the store inside the 'store-device' as seen by GRUB, e.g. it would
> +  ;; contain "/storefs" if the store is located in that subvolume of a btrfs
> +  ;; partition.
>    (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)
> +  (store-directory-prefix boot-parameters-store-directory-prefix)
> ------------------------------------------------------------------------------

s/path/file name/, but otherwise LGTM.

Thanks for working on these changes!

Ludo’.

bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.

[Miguel Ángel Arruga Vivas]

Hello,

Ludovic Courtès <ludo@gnu.org> writes:
> (Btrfs no0b here.)  Does that mean that /gnu is like a bind-mount of
> /gnufs in this case?

Yes, it's exactly like that for the end user, but that use case isn't
implemented yet---it should be useful too for the Hurd and
translators---so I have it under my radar.

> Anyway, I think I got it now, but I feel I’ll have to search again for
> this example next time I stumble upon it.  ;-)

I also linked this report on the commit message, as it's a non-trivial
use case and everybody could need as much info as possible if an error
hits---fingers crossed.

>> Writing the last email I though about adding this:
>> ------------------------------------------------------------------------------
>>    ;; OS's root file system, so it might be a device path like "/dev/sda3".
>> +  ;; The 'store-directory-prefix' field contains #f or the actual path of
>> +  ;; the store inside the 'store-device' as seen by GRUB, e.g. it would
>> +  ;; contain "/storefs" if the store is located in that subvolume of a btrfs
>> +  ;; partition.
>>    (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)
>> +  (store-directory-prefix boot-parameters-store-directory-prefix)
>> ------------------------------------------------------------------------------
>
> s/path/file name/, but otherwise LGTM.

I've pushed this change as 2df44e934c9ba14a89d9245d1a4f7cf18e8cfdaa with
changes on the rest of the comment as well, as the wording was the same,
as I agree that path should refer to a route (where there could be
several concrete locations/file names in unix dialect), and file name
should be the correct term.

> Thanks for working on these changes!

Thanks to you for the review, for me this is useful in my daily life (as
now I can manage btrfs better too, hehe) and much fun! :-)

Happy hacking!
Miguel

bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.

[Ludovic Courtès]

Hi,

Miguel Ángel Arruga Vivas <rosen644835@gmail.com> skribis:

> I've pushed this change as 2df44e934c9ba14a89d9245d1a4f7cf18e8cfdaa with
> changes on the rest of the comment as well, as the wording was the same,
> as I agree that path should refer to a route (where there could be
> several concrete locations/file names in unix dialect), and file name
> should be the correct term.

Thanks!  Regarding “file name”, it’s terminology that’s part of the GNU
Coding Standards and I think it’s nice to be consistent throughout the
project.

Ludo’.