Problems with /gnu/store in a different btrfs subvolume

  • Done
  • quality assurance status badge
Details
3 participants
  • Danny Milosavljevic
  • Ludovic Courtès
  • Miguel Ángel Arruga Vivas
Owner
unassigned
Submitted by
Miguel Ángel Arruga Vivas
Severity
normal
M
M
Miguel Ángel Arruga Vivas wrote on 24 Oct 2020 19:56
(address . bug-guix@gnu.org)
878sbvh5j4.fsf@gmail.com
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.
M
M
Miguel Ángel Arruga Vivas wrote on 24 Oct 2020 20:01
[PATCH 1/3] system: Fix grub keymap with store in btrfs subvolume.
(address . 44196@debbugs.gnu.org)
873623h5aq.fsf@gmail.com
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(-)

Toggle diff (23 lines)
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
M
M
M
Miguel Ángel Arruga Vivas wrote on 24 Oct 2020 23:13
[PATCH 3/3] gnu: grub: Add output locale
(address . 44196@debbugs.gnu.org)
87tuujfhum.fsf@gmail.com
Hi!

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

Happy hacking!
Miguel
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(-)

Toggle diff (90 lines)
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
M
M
Miguel Ángel Arruga Vivas wrote on 25 Oct 2020 11:04
bug#44196: [PATCH 3/3 v2] gnu: grub: Add locale output.
(address . 44196@debbugs.gnu.org)
87imay7hax.fsf_-_@gmail.com
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.
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(-)

Toggle diff (83 lines)
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
M
M
Miguel Ángel Arruga Vivas wrote on 25 Oct 2020 11:21
control message for bug #44196
(address . control@debbugs.gnu.org)
87d0167ghm.fsf@gmail.com
tags 44196 + patch
quit
M
M
Miguel Ángel Arruga Vivas wrote on 26 Oct 2020 23:04
Re: bug#44196: [PATCH 3/3 v3] system: Do not depend on locale folder generated by grub-install.
(address . 44196@debbugs.gnu.org)
87y2jsod97.fsf_-_@gmail.com
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:

Happy hacking!
Miguel
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(-)

Toggle diff (78 lines)
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
M
M
Miguel Ángel Arruga Vivas wrote on 26 Oct 2020 23:29
Re: bug#44196: [PATCH 4/3] system: Fix dependency for grub.cfg generation.
(address . 44196@debbugs.gnu.org)
87mu08oc3a.fsf@gmail.com
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.
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(-)

Toggle diff (21 lines)
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
M
M
Miguel Ángel Arruga Vivas wrote on 28 Oct 2020 22:34
Re: bug#44196: [PATCH v4 3/4] system: Do not depend on locale folder generated by
(address . 44196@debbugs.gnu.org)
87v9eum3w2.fsf_-_@gmail.com
Sorry, the last one didn't generate correctly the locale folder for two
reasons: wrong name for the .mo name, and incomplete path.
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(-)

Toggle diff (77 lines)
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
M
M
Miguel Ángel Arruga Vivas wrote on 30 Oct 2020 19:13
Re: bug#44196: [PATCH v5 3/4] system: Do not depend on locale folder generated by
(address . 44196@debbugs.gnu.org)
87mu0360qt.fsf_-_@gmail.com
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.
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(-)

Toggle diff (84 lines)
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
D
D
Danny Milosavljevic wrote on 30 Oct 2020 20:35
Re: bug#44196: [PATCH 4/3] system: Fix dependency for grub.cfg generation.
(name . Miguel Ángel Arruga Vivas)(address . rosen644835@gmail.com)(address . 44196@debbugs.gnu.org)
20201030203527.682b4bf5@scratchpost.org
This patch LGTM!
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAl+cav8ACgkQ5xo1VCww
uqWchQf+PE+l4wMAPT/iKsDzKH5tRsnfUrztZmJnAmleODdoqs1lXfFs0pzEiCVa
Gemoj/h+xru7MJrCSX/o90z2A567gBan4xQcYYDMkCKEzclTJL3lYFAbpSgcyDtT
0LAPbV7LklxcwHAU+RQcG5FEdo7RPz7GCBp1fH2Bc0YWj+rHziJ/Ih4zsQWaXoAf
g2EY0H2LjLPFFIc1KliGIjDheRy21vpSuzYF28e1+0e909eugFlZYlwbbxLBdpDN
w0CFva0xV469y3bAe8EDoJ97DtJ00KUi0ihDmheBfbd9K3Y5LnQgXfpw4UPmEW3t
m+/qVy8xTSgaeheqw0y5/laVC7xUPg==
=bb2J
-----END PGP SIGNATURE-----


D
D
Danny Milosavljevic wrote on 30 Oct 2020 20:36
Re: bug#44196: [PATCH 1/3] system: Fix grub keymap with store in btrfs subvolume.
(name . Miguel Ángel Arruga Vivas)(address . rosen644835@gmail.com)(address . 44196@debbugs.gnu.org)
20201030203638.67badd33@scratchpost.org
Is it possible for there to be no entries in all-entries at all?

If not, LGTM!
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAl+ca0YACgkQ5xo1VCww
uqUIkgf8CFmidnvSTzafjtMhxneXFVo6mzl0TviBX0lFK3IEUnhHFX/5p940Emwy
6Z5787H46jGJ8YpqKI0MV/0XF4VQO43xlwb6B2ZG25v0htttvm+Z4IJgYi0dnyA+
n4MDpni/ekzmguqr/3L3rA94oL6tmXBY8YxWeb7WpnksA2mfM0AHJpF/HTXYMi6Y
dO5q5cS4CjCb0i2fSni/rRWeWF43GYbEm3bFPujFg1wuQrzLcBpkj1std08VhzC6
l+ycvQMg4//6ekC51u7vmFCg6KT0rJCuVQ6PoG4fR6ZBHwvYPnfYnKhAYUnGtGO/
BnYZF3KRkVHyadsh74ctn4OCir138w==
=vZ/Y
-----END PGP SIGNATURE-----


M
M
Miguel Ángel Arruga Vivas wrote on 30 Oct 2020 22:38
Re: bug#44196: [PATCH 4/3] system: Fix dependency for grub.cfg generation.
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)(address . 44196@debbugs.gnu.org)
87ft5v5r91.fsf@gmail.com
Danny Milosavljevic <dannym@scratchpost.org> writes:
Toggle quote (2 lines)
> This patch LGTM!

Thanks for your review, pushed as 222a630e9e.

Happy hacking!
Miguel
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEEiIeExBRZrMuD5+hMY0xuiXn6vsIFAl+ch8oACgkQY0xuiXn6
vsJ4ZAv/aP0Qib0qkRb9eMcUVsIQf6RLVZKGwxcUEPDMaK9EfCnNJg5JdYWrkVnp
ds3jVifdDDhL9FMAYb52TnaJpaF7xpGjRJ70h6w0nLDiMY+3F54WCoiFonubLNWo
qOpEFDh1GA2Ph8xZ9oW29d1amANXKFebd/CatXa3U9yxGQ5BjfYS3/SsCgl2L9Cl
lRu0W1SKXV35t1hVl3mF3c2Dkm15UemoMM3deHVDUZUg90bIL1HXZva7I3aufONz
EAzmpEqeCmy3w5Ob/AzdTXd59QvHkYipbsp61TBh6CvCaLsBMNURwIrMvJ5ndk1P
5Ncqw0+ztUZ8I2xoA85xCFttJkST9KwT38VLRtDVl49KvVX1PqlsL1wbM8amXiDm
gChQ3mdw4z9GnN5l9esEAucjCwTgcAmvhoFvm97kjmoDjUEDG2WCkqVqfNlEJlOT
Cuy9sxeWuKbDxVcTik6ZE/I0Xhc6u5+XcBBw4YsASE8kWCa/PNq+UgpnZ3/Iy385
Y9ZrZhpE
=YNYK
-----END PGP SIGNATURE-----

M
M
Miguel Ángel Arruga Vivas wrote on 30 Oct 2020 22:38
Re: bug#44196: [PATCH 1/3] system: Fix grub keymap with store in btrfs subvolume.
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)(address . 44196@debbugs.gnu.org)
87eelf5r8g.fsf@gmail.com
Hi Danny,

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

Danny Milosavljevic <dannym@scratchpost.org> writes:
Toggle quote (2 lines)
> 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.

Toggle quote (2 lines)
> If not, LGTM!

Pushed as c69a1c27ee.

Happy hacking!
Miguel
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEEiIeExBRZrMuD5+hMY0xuiXn6vsIFAl+ch98ACgkQY0xuiXn6
vsIBDgv/SqloPbXJ1sKi7MeEohEoNZ5m3DLFQOaNjC4tbVGiX5X5nRRcfwv+ue0/
0JgJKnMeZcdW3O+mTIKfuEOtkkcf1CJRZT18FpJHWUkfBdvNP2JFJrijMY6PuB/o
jOUJcTmUYZOpIVCe6OsbFLMwy4en4jO1L8N7j5ElQMQk51vtaSEiPIlpS8SzpcDA
1ZmF4a2m9HRWFFMYBLcUwYElE59n3WyjOICVivyzlRPQtWNXFEsKR+8YLL+UrWcm
NUld4nl58zvki/GHIEHXiuX9TVzBQVT6rNxtXzP6tFiF3/sWi+eBxBDYvX+BZh2j
DalYjNGKxe2hUe5dz9HhHJH978Bu36DruGTP5Xpo2SLj9HBSnjysK9KtTtO4GZXN
dEUg82AxQ0w5YVAeuhDxhNJEXG3/Ss4+VAHr7Uf0q8GXzC3USqzpvDZcrmJufDv9
BgCrzGD0c2eqXvR5C5imMSdkBhwA+JjaFlJg2vwEZOwz2tC2fi0GdQyvNugGoYy4
KBAbmEKW
=Q21z
-----END PGP SIGNATURE-----

M
M
Miguel Ángel Arruga Vivas wrote on 30 Oct 2020 22:45
Re: bug#44196: [PATCH v6 3/4] system: Do not depend on locale folder generated by
(address . 44196@debbugs.gnu.org)
87a6w35qx2.fsf_-_@gmail.com
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:
Toggle quote (3 lines)
> 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.
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(-)

Toggle diff (84 lines)
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
L
L
Ludovic Courtès wrote on 31 Oct 2020 22:39
Re: bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.
(name . Miguel Ángel Arruga Vivas)(address . rosen644835@gmail.com)(address . 44196@debbugs.gnu.org)
87blgit6qr.fsf@gnu.org
Hi!

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

Toggle quote (25 lines)
> 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.

[...]

Toggle quote (15 lines)
> +++ 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’.
L
L
Ludovic Courtès wrote on 31 Oct 2020 22:44
Re: bug#44196: [PATCH v6 3/4] system: Do not depend on locale folder generated by
(name . Miguel Ángel Arruga Vivas)(address . rosen644835@gmail.com)(address . 44196@debbugs.gnu.org)
875z6qt6i2.fsf@gnu.org
¡Hola!

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

Toggle quote (12 lines)
> 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.

[...]

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

s/folder/directory/ :-)

Toggle quote (16 lines)
> + (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”?

Toggle quote (9 lines)
> + (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/

Toggle quote (17 lines)
> + #~(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’.
M
M
Miguel Ángel Arruga Vivas wrote on 1 Nov 2020 00:02
Re: bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 44196@debbugs.gnu.org)
87361u3sp5.fsf@gmail.com
Hi Ludo!

First of all, thanks for the review.

Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (4 lines)
> 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.

Toggle quote (2 lines)
> 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
M
M
Miguel Ángel Arruga Vivas wrote on 1 Nov 2020 01:01
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 44196@debbugs.gnu.org)
87y2jm2be2.fsf@gmail.com
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:

Toggle quote (71 lines)
> 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
M
M
Miguel Ángel Arruga Vivas wrote on 1 Nov 2020 01:36
Re: bug#44196: [PATCH v6 3/4] system: Do not depend on locale folder generated by
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 44196@debbugs.gnu.org)
87tuu93obt.fsf@gmail.com
¡Hola, Ludo!

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

Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (9 lines)
> 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...

Toggle quote (3 lines)
>>
>> * 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.

Toggle quote (4 lines)
>> (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.
Toggle quote (8 lines)
>
> [...]
>
>> +(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.

Toggle quote (18 lines)
>> + (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. :-)

Toggle quote (11 lines)
>> + (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 ...

Toggle quote (5 lines)
>> + #~(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.

Toggle quote (15 lines)
>> + (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
M
M
Miguel Ángel Arruga Vivas wrote on 1 Nov 2020 02:44
Re: bug#44196: [PATCH 2/3] system: Add store-directory-prefix to boot-parameters.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 44196-done@debbugs.gnu.org)
87imap3l77.fsf@gmail.com
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
Closed
L
L
Ludovic Courtès wrote on 2 Nov 2020 17:06
(name . Miguel Ángel Arruga Vivas)(address . rosen644835@gmail.com)(address . 44196@debbugs.gnu.org)
87y2jjivze.fsf@gnu.org
Hi!

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

Toggle quote (39 lines)
> 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. ;-)

Toggle quote (16 lines)
> 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’.
M
M
Miguel Ángel Arruga Vivas wrote on 2 Nov 2020 19:52
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 44196@debbugs.gnu.org)
87y2jj1ti5.fsf@gmail.com
Hello,

Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (3 lines)
> (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.

Toggle quote (3 lines)
> 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.

Toggle quote (18 lines)
>> 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.

Toggle quote (2 lines)
> 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
L
L
Ludovic Courtès wrote on 3 Nov 2020 10:26
(name . Miguel Ángel Arruga Vivas)(address . rosen644835@gmail.com)(address . 44196@debbugs.gnu.org)
874km6hju1.fsf@gnu.org
Hi,

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

Toggle quote (6 lines)
> 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’.
?