diff mbox series

[bug#48314,v3] Install guix system on Raspberry Pi

Message ID l6WqHiPe2KyGlSJh6P0GC-HgB2FtQ9KNr64YZNUv581fODPnfdcujxm0cpxYTR6QL9sz85YavhYa9oaerpfhNQn1UMqe8GbGT4GRoYnN_6c=@protonmail.com
State New
Headers show
Series [bug#48314,v3] Install guix system on Raspberry Pi | expand

Checks

Context Check Description
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue

Commit Message

phodina Aug. 12, 2022, 2:27 p.m. UTC
Hi Stefan!

Good news. I managed to run Guix libre kernel on the raspberry pi. Thanks for the help along the way!

pi@raspberrypi-guix ~$ uname -a
Linux raspberrypi-guix 5.18.16-gnu #1 SMP PREEMPT 1 aarch64 GNU/Linux

The issue was in the firmware, therefore I'll post the hashes of the files from the EFI partition:

pi@raspberrypi-guix ~$ sha256sum  /boot/efi/*
010beacf073dbf7a4be24288a5c8b93001f0d852387dce50bf50de51a7412cd6  /boot/efi/bcm2711-rpi-400.dtb
489645357820f2e7e8f13841c901ba9571b779c07b3203f1627538d04ce45ad3  /boot/efi/bcm2711-rpi-4-b.dtb
c0f057eea9e357341265910000e56dab94b3200465b0556deb1eda3af117d3c9  /boot/efi/bcm2837-rpi-3-a-plus.dtb
df83b6dc6cda7e8eae62e8316b02a4c1659a6b0cf874c6caa075be9413a00b98  /boot/efi/bcm2837-rpi-3-b.dtb
c008e84ac57aa9c35aedabd1ed2cb4290088e33d85d1ef8ca56c5ef9b5f0d13c  /boot/efi/bcm2837-rpi-3-b-plus.dtb
6a1cc758d38edcf9f9213a8fcbc75d4bf06fbd86806b4430c15742b6ab427de9  /boot/efi/bcm2837-rpi-cm3-io3.dtb
69309823da13dc96b89e3d82b44f820e4f84efa79d207adad2c8784559794f03  /boot/efi/bootcode.bin
1ac38b353f924c56c5d5a587971f3f81d09c433787b99f889368fd342c4336da  /boot/efi/bootloader.txt
df0ac4af19615f13ff7ffa395ae553c70813ef9d2a82fab0e1175adf80ed1294  /boot/efi/cmdline.txt
9d4975d57f5eb54b08a430cb3d677e5dbf23ed48c73fe33a4e3efdfc35f8d41e  /boot/efi/config.txt
bcc22553ef64d361270103e84c80ab5362bdb2ffba3c9eea13ee3de60f6cbaff  /boot/efi/dtb.txt
22db24c621c326d907c7b8c5975b1730e6ce78dea680bec3958d907093031638  /boot/efi/fixup4cd.dat
7d28775bff4781bda065f37fbb64c88ed1b56d1f4af79d85f792032f55bb6de7  /boot/efi/fixup4.dat
b0d299dd46ddecd2c4eeeca61f42d114a0c464dcd6165861a662d118daa3afc0  /boot/efi/fixup4db.dat
495076ed0488703ba59bd0d43bc577ad1695470379263854197a752cd1989330  /boot/efi/fixup4x.dat
22db24c621c326d907c7b8c5975b1730e6ce78dea680bec3958d907093031638  /boot/efi/fixup_cd.dat
8e90c8a379f3f99ee59370c50e48853db82669dbd6515d4ce08a24307d3dc4c5  /boot/efi/fixup.dat
4f5d2433956f64cec640ae91dc47bdea3aa2c6c7cc579faafb779826bd7e554c  /boot/efi/fixup_db.dat
82bbf5a3f86f73a41e9f8975deb75e50ab4d7581500a43ec7f0fcc42214948dd  /boot/efi/fixup_x.dat
9756eca19be1a443fc6a6cd5f8ffb0759d8b5d68f24248829d39abdc59388490  /boot/efi/manifest
9e1474d3b3078bb80e87f65a5122fd4a8a828b563de5652b37d8b40019b3a51a  /boot/efi/start4cd.elf
2dfb27b876ec00e54857e199e939def019b148010f03ce1947e4b48fa226e4f7  /boot/efi/start4db.elf
a607afd09523830bd524eafa2eb3f530d70cef643eb8eca14a80dcbd6830ec8f  /boot/efi/start4.elf
7ffa3ce1f93f61737fee68f61747b876fd1b15203553ebc096d2b64d79fb7da2  /boot/efi/start4x.elf
b21aeceec40aff935f70dfe3adc4d96963e61d4696f909ad910f299da978d8bb  /boot/efi/start_cd.elf
378e84616f28e63cb33c794e3430b7deeca5bb84067f79e71408f6aab587e0e7  /boot/efi/start_db.elf
e4a374d78f31d333b20ab128a614d3549f9dfe4103781c0938104fc0dd45d3fc  /boot/efi/start.elf
dbdeb7c679566419035e70db8aff90954ca6ba45a579e990886ddc0b964b2621  /boot/efi/start_x.elf
f9c7d5534c787479601f1e70b4b108cd765445aecbc154296d0d35f423045ca4  /boot/efi/sysconf.txt
d47f45c6221ccaaf1ca0e66d01b90fca350a0b06b4c7a44a81b6f68a99f36607  /boot/efi/u-boot.bin

I'll test it also on RPi3 and I'll change the storage media to SSD on my RPi4 to provide more space and faster reliable storage.

- I just have question regarding the example if it wouldn't be better to prepare the whole image - partition the SD card and just copy it to the actual media?

- Should there be some manual how to prepare the firmware files or format the SD card in case we prepare just the root filesystem and bootloader?

IMHO having ISO image for Raspberry Pi 3,4 (aarch64) would be great as it would allow them to run Guix easily without need to build it on some other aarch64 machine or resort to crosscompile. What do you think?


Also there's small patch to fix deprecated calls in os-defintion files.

----
Petr

Comments

Stefan Aug. 13, 2022, 10:48 a.m. UTC | #1
Hi Petr!

> Good news. I managed to run Guix libre kernel on the raspberry pi.

Woot! Great!

> The issue was in the firmware

Good to know!

> I'll test it also on RPi3

In the meantime I got everything compiled and installed for my Raspberry Pi 3b as well. But there is some issue with the U-Boot. If I replace U-Boot with an older version, it boots and the system is running fine. However, during boot the Linux-Libre-Gnus and early kernel messages are not visible, but after boot all seems to be fine.

Maybe the troubles stem from the firmware, as in your case – I use a different one –, or maybe the U-Boot only works on a Pi 4.

I’m using the Linux-Libre kernel with the %bcmrpi3-defconfig from the Raspberry Pi Linux sources (removing a long list of unsupported configurations)

stefan@guix ~$ uname -a
Linux guix 5.18.12-bcmrpi3-v8 #1 SMP PREEMPT 1 aarch64 GNU/Linux

> I just have question regarding the example if it wouldn't be better to prepare the whole image

Yes, certainly. As you may know I started with Guix on void with an NFS root file-system and patched Guix to get to this point, still on NFS. So I have no experience yet to build an image. 

From what I saw in the code, I guess that more work needs to be done to generate an image. My personal focus is to first get the patches merged.

> Should there be some manual how to prepare the firmware files or format the SD card in case we prepare just the root filesystem and bootloader?

I think the comments in e.g. raspberry-pi-64.tmpl could be improved. A hint to the non-free firmware is certainly problematic. Not sure, if we should or even can mention the Raspberry Pi in the manual.

> IMHO having ISO image for Raspberry Pi 3,4 (aarch64) would be great as it would allow them to run Guix easily without need to build it on some other aarch64 machine or resort to crosscompile. What do you think?

Yes, I totally agree. I also think that it could help to spread Guix. It would be great to see something like e.g. Pi-hole in the future to be based on Guix System.

But first things first, the merge is still pending.

> Also there's small patch to fix deprecated calls in os-defintion files.

Thanks, much appreciated!


Bye

Stefan
phodina Aug. 14, 2022, 9:59 a.m. UTC | #2
Hi Stefan!

> > I'll test it also on RPi3

I got it working also there. I do have RPi2 but I doubt it will run there as it's just not that powerful.

> > I just have question regarding the example if it wouldn't be better to prepare the whole image
> 
> 
> Yes, certainly. As you may know I started with Guix on void with an NFS root file-system and patched Guix to get to this point, still on NFS. So I have no experience yet to build an image.
> 
> From what I saw in the code, I guess that more work needs to be done to generate an image. My personal focus is to first get the patches merged.
> 

Yes, the focus should be on mainlining these changes first.


> > IMHO having ISO image for Raspberry Pi 3,4 (aarch64) would be great as it would allow them to run Guix easily without need to build it on some other aarch64 machine or resort to crosscompile. What do you think?
> 
> 
> Yes, I totally agree. I also think that it could help to spread Guix. It would be great to see something like e.g. Pi-hole in the future to be based on Guix System.
> 

I know there is some open source bootloader which is already packaged in Guix `raspi-arm64-chainloader`. Maybe this would be the way in the future :-)

Good luck

----
Petr
Stefan Aug. 14, 2022, 11:35 a.m. UTC | #3
Hi Petr!

> I got it working also there.

Great!

Which Pi is it exactly? Did you use the same build as for your Pi 4? And the same firmware? Did you see the Linux-Libre gnus and early kernel messages after GRUB?

> I know there is some open source bootloader which is already packaged in Guix `raspi-arm64-chainloader`. Maybe this would be the way in the future :-)

I never tried it. It may be possible.


Bye

Stefan
Stefan Sept. 1, 2022, 11:55 p.m. UTC | #4
Hi Petr!

The firmware I’m using is the same, but all dtb files differ.

Did you use the same set of dtb files on our Pi 3? Especially I have a /boot/efi/bcm2710-rpi-3-b.dtb – which is loaded at boot due to the upstream_kernel=0 setting in dtb.txt –, which you seem to be missing.

I guess that your Pi 3 will not load any dtb file.

My current but not working U-Boot has a different hash.


Bye

Stefan
phodina Sept. 2, 2022, 5:49 a.m. UTC | #5
Hi Stefan!

I'll check and post the config here but I don't have Raspberry Pi currently with me.

----
Petr

-------- Original Message --------
On Sep 2, 2022, 1:55 AM, Stefan wrote:

> Hi Petr!
>
> The firmware I’m using is the same, but all dtb files differ.
>
> Did you use the same set of dtb files on our Pi 3? Especially I have a /boot/efi/bcm2710-rpi-3-b.dtb – which is loaded at boot due to the upstream_kernel=0 setting in dtb.txt –, which you seem to be missing.
>
> I guess that your Pi 3 will not load any dtb file.
>
> My current but not working U-Boot has a different hash.
>
> Bye
>
> Stefan
Stefan Sept. 4, 2022, 6:41 p.m. UTC | #6
Hi Petr!

> I'll check and post the config here but I don't have Raspberry Pi currently with me.

Thanks, for the offer, but it isn’t necessary any more, I found the reason.

Actually I’m net-booting my Raspberry Pi 3b over network. But due to a known bug in the ROM code, I still have a microSD card inserted with only the bootcode.bin file.

If I boot the Pi with that card inserted, then GRUB has a problem:

error: variable `root' isn't set.
Entering rescue mode...
grub rescue>

If I remove the microSD card, then GRUB has no problem with its root variable, and Guix System is started.

Interestingly this does not happen, if I use the older U-Boot version 2020.10.

The current U-Boot version 2022.04 prints an error message – with and without the mircoSD card inserted:

libfdt fdt_check_header(): FDT_ERR_BADMAGIC

But I’m not sure, if this error message is related.


Bye

Stefan
Stefan Sept. 22, 2022, 4:18 p.m. UTC | #7
Hi!

I did a rebase onto commit 2e8b4f9bfa00489fd3acff305837a79af236e183.

Vagrant, there was a comment left about removing "CONFIG_BOOTDELAY=1" for the u-boot, this is now done. I think all review comments have been applied.

There is a new u-boot-rockpro64-rk3399 which I adapted as well to use the #:configs keyword argument.

The function modify-defconfig in guix/build/kconfig.scm no longer interprets "CONFIG_XY=" like "# CONFIG_XY is not set".


Bye

Stefan
Ludovic Courtès Oct. 5, 2022, 1:02 p.m. UTC | #8
Hi,

Vagrant, Danny: could you take a look at these patches?  They seem to
have fallen through the cracks.

TIA.  :-)

Ludo’.

Stefan <stefan-guix@vodafonemail.de> skribis:

> Hi!
>
> I did a rebase onto commit 2e8b4f9bfa00489fd3acff305837a79af236e183.
>
> Vagrant, there was a comment left about removing "CONFIG_BOOTDELAY=1" for the u-boot, this is now done. I think all review comments have been applied.
>
> There is a new u-boot-rockpro64-rk3399 which I adapted as well to use the #:configs keyword argument.
>
> The function modify-defconfig in guix/build/kconfig.scm no longer interprets "CONFIG_XY=" like "# CONFIG_XY is not set".
>
>
> Bye
>
> Stefan
Vagrant Cascadian Oct. 8, 2022, 4:22 p.m. UTC | #9
On 2022-09-22, Stefan wrote:
> I did a rebase onto commit 2e8b4f9bfa00489fd3acff305837a79af236e183.

Thanks! It definitely looks like great progress.

> Vagrant, there was a comment left about removing "CONFIG_BOOTDELAY=1"
> for the u-boot, this is now done. I think all review comments have
> been applied.

Great!

> There is a new u-boot-rockpro64-rk3399 which I adapted as well to use
> the #:configs keyword argument.

I like how that works.

> The function modify-defconfig in guix/build/kconfig.scm no longer
> interprets "CONFIG_XY=" like "# CONFIG_XY is not set".

Nervous about how that actually works, but hopefully it plays out correctly.


This whole patch series is large and overwhelming and at least a bit
beyond my abilities to wrap my head around (which has certainly caused
me to wait a bit to review)... so I cannot possibly comment on weather
the series as a whole is "good", through no fault of the patch
author(s)! I will try and comment where I can, but really need help to
review it in any meaningful way.

Also from what I recall on earlier iterations of this patch series,
different reviewers seemed to have differing style recommendations
around weather to split patches into smaller commits or merge patches
into combined commits, which can surely be frustrating. I don't *want*
to be frustrating, but I lean towards splitting patches into as small a
commit as possible to wrap my head around the distinct ideas.

I also like to refactor out anything that can be applied directly to
master as soon as possible (e.g. the description-appending patches look
promising for that) with the hope to get the remaining patch series
smaller and smaller with each iteration. Some people may want to do an
all-or-nothing merge. I don't know what's "right" for the guix
community...

With all that out of the way... here goes my attempt to review!


> gnu: linux: Fix the extra-version parameter in make-linux-libre*.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> * gnu/packages/linux.scm (make-linux-libre*) ['set-environment]: Make
> the Makefile accept EXTRAVERSION from the environment. Fix the usage of
> an empty extra-version string. Split this new phase out of and adding
> if before …
> ['configure]: … to make the phases more hackable.

Overall, this looks good, to me, though have one question...

> diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
> index 306c18e398..1a35e857c3 100644
> --- a/gnu/packages/linux.scm
> +++ b/gnu/packages/linux.scm
...
> @@ -824,8 +825,8 @@ for ARCH and optionally VARIANT, or #f if there is no such configuration."
>                   (lambda _
>                     (substitute* (find-files "." "^Makefile(\\.include)?$")
>                       (("/bin/pwd") "pwd"))))
> -               (replace 'configure
> -                 (lambda* (#:key inputs target #:allow-other-keys)
> +               (add-before 'configure 'set-environment
> +                 (lambda* (#:key target #:allow-other-keys)
>                     ;; Avoid introducing timestamps.
>                     (setenv "KCONFIG_NOTIMESTAMP" "1")
>                     (setenv "KBUILD_BUILD_TIMESTAMP"
> @@ -847,13 +848,16 @@ for ARCH and optionally VARIANT, or #f if there is no such configuration."
>                         (setenv "CROSS_COMPILE" (string-append target "-"))
>                         (format #t "`CROSS_COMPILE' set to `~a'~%"
>                                 (getenv "CROSS_COMPILE"))))
> -
> +                   ;; Allow EXTRAVERSION to be set via the environment.
> +                   (substitute* "Makefile"
> +                     (("^ *EXTRAVERSION[[:blank:]]*=") "EXTRAVERSION ?="))
>                     (setenv "EXTRAVERSION"
>                             #$(and extra-version
> -                                  (string-append "-" extra-version)))
> -
> +                                  (not (string-null? extra-version))
> +                                  (string-append "-" extra-version)))))
> +               (replace 'configure
> +                 (lambda* (#:key inputs #:allow-other-keys)
>                     (let ((config (assoc-ref inputs "kconfig")))
> -
>                       ;; Use a custom kernel configuration file or a default
>                       ;; configuration file.
>                       (if config
> @@ -871,7 +875,7 @@ for ARCH and optionally VARIANT, or #f if there is no such configuration."
>  
>                       (invoke "make" "oldconfig"))))
>                 (replace 'install
> -                 (lambda* (#:key inputs native-inputs #:allow-other-keys)
> +                 (lambda* (#:key inputs #:allow-other-keys)
>                     (let ((moddir (string-append #$output "/lib/modules"))
>                           (dtbdir (string-append #$output "/lib/dtbs")))
>                       ;; Install kernel image, kernel configuration and link map.

Why is native-inputs removed from the 'install phase? Is it no longer
needed? Was it not actually needed before? I see no mention of this
change in the comment.


> gnu: bootloader: Rework chaining, add grub-efi-netboot-removable-bootloader.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> * doc/guix.texi (Bootloader Configuration): Describe the new
> ‘grub-efi-netboot-removable-bootloader’.  Mention used sub-directories and
> that the UEFI Boot Manager is not modified.  Advice to disable write-access
> over TFTP.
> * gnu/bootloader.scm (efi-bootloader-profile): Allow a list of packages and
> collect everything directly in the profile, avoiding a separate collection
> directory.  Renamed the profile from "bootloader-profile" to
> "efi-bootloader-profile".
> [bootloader-collection]: Renamed to …
> [efi-bootloader-profile-hook]: … this and removed unused modules and the
> creation of the now unneeded collection directory.
> (efi-bootloader-chain): Added packages and disk-image-installer arguments.
> Removed handling of the collection directory, now only calling the given
> installer procedure.
> * gnu/bootloader/grub.scm (make-grub-efi-netboot-installer): New helper.
> (make-grub-configuration): New helper based on (grub-configuration-file).
> Adding grub argument, fixed indentation, removend code to get grub.
> (grub-configuration-file): Now using (make-grub-configuration).
> (grub-efi-configuration-file): New function using (make-grub-configuration).
> Instead of getting the grub-efi package from the bootloader-configuration
> this function refers to the grub-efi package directly.
> (grub-cfg): New variable to replace "/boot/grub/grub.cfg".
> (install-grub-efi-netboot): Removed, the functionality got moved.
> (make-grub-efi-netboot-installer): New helper function to return a customized
> installer for a certain efi-sub-directory.  The installer basically copies
> a pre-installed efi-bootloader-profile, and adds needed symlinks for booting
> over network, or – on an ESP – an intermediate grub-cfg to load the final
> grub-cfg file.
> (grub-bootloader): Now using the grub-cfg variable.
> (grub-efi-bootloader): Now using the grub-cfg variable.  Removed inheritance,
> giving complete set of fields.
> (make-grub-efi-netboot-bootloader): New helper function.
> (grub-efi-netboot-bootloader): Now using the helper.
> (grub-efi-netboot-removable-bootloader): New bootloader using the helper.
> It uses the efi-sub-directory "efi/boot" for removable media.
> * gnu/packages/bootloaders.scm (make-grub-efi-netboot): New function to return
> a grub-efi package pre-installed via grub-mknetdir, customized for an
> efi-sub-directory and able to boot via network and local storage.
>
> The rework allows to use an (efi-bootloader-chain) like this, which is able
> to boot over network or local storage, depending on the symlink-support at
> the bootloader-target:
>
> (operating-system
>  (bootloader
>    (bootloader-configuration
>      (bootloader
>        (efi-bootloader-chain
>          grub-efi-netboot-removable-bootloader
>          #:packages (list my-firmware-package
>                           my-u-boot-package)
>          #:files (list (plain-file "config.txt"
>                                    "kernel=u-boot.bin"))
>          #:hooks my-special-bootloader-profile-manipulator))
>      (target "/booti/efi")
>      …))
>  …)
> )
> ---
>  doc/guix.texi                |   58 ++++++++---
>  gnu/bootloader.scm           |  104 ++++++++++----------
>  gnu/bootloader/grub.scm      |  215 ++++++++++++++++++++++++++----------------
>  gnu/packages/bootloaders.scm |   90 ++++++++++++++++++
>  4 files changed, 318 insertions(+), 149 deletions(-)

There is too much going on here for me to follow, but it is perhaps just
doing a lot of big changes... help? :)


> build: kconfig: Add new module to modify defconfig files.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> * guix/build/kconfig.scm (config-string->pair, (pair->config-string,
> defconfig->alist, modify-defconfig, verify-config): New file with
> functions for handling of defconfig and .config files.
> * gnu/packages/bootloaders.scm (make-u-boot-package,
> make-u-boot-sunxi64-package): Adding new keyword arguments to pass and/
> or modify a defconfig file.
> (u-boot-{am335x-boneblack,pinebook,u-boot-novena,rockpro64-rk3399}):
> Simplify packages by using the new keyword arguments of the former
> functions.
> * Makefile.am: Adding guix/build/kconfig.scm to MODULES.
> ---
>  Makefile.am                  |    1 
>  gnu/packages/bootloaders.scm |  111 +++++++++++--------------
>  guix/build/kconfig.scm       |  185 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 234 insertions(+), 63 deletions(-)
>  create mode 100644 guix/build/kconfig.scm

I like how this simplifies the various u-boot-* package definitions!


> gnu: bootloader: Add U-Boot packages for Raspberry Pi models.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> * gnu/packages/bootloader.scm (make-u-boot-package): Add keyword
> parameters 'name-suffix' and 'append-description'.

This seems good to me.

> (make-u-boot-bin-package): New function to make minimal packages.
> (%u-boot-rpi-efi-configs): New helper list with config strings.
> (%u-boot-rpi-description-32-bit, %u-boot-rpi-description-64-bit,
> %u-boot-rpi-efi-description, %u-boot-rpi-efi-description-32-bit):
> New helper strings.
> (u-boot-rpi-2{,-efi,-bin,-efi-bin},
> u-boot-rpi-3-32b{,-efi,-bin,-efi-bin},
> u-boot-rpi-4-32b{,-efi,-bin,-efi-bin},
> u-boot-rpi-arm64{,-efi,-bin,-efi-bin}): New packages.
> (u-boot-tools): Reuse the description of u-boot.
> (u-boot-{am335x-boneblack,am335x-evm,nintendo-nes-classic-edition,
> novena}): Make use of new keyword parameters of make-u-boot-package.

It would be nice to first switch the existing u-boot-* packages to use
the new append-description feature one commit (I think this could even
be applied directly to master?), and then add the new functionality
(e.g. make-u-boot-bin-package, *u-boot-rpi-*, etc.) in another commit or
a couple commits. At least, that would make it a little easier for me to
read.


> gnu: linux: New function to modify the configuration of a Linux kernel.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> * gnu/packages/linux.scm (system->linux-srcarch): New function to return the
> relevent folder name below arch/ in the Linux source code.
> (modify-linux): New function to make a customized Linux package inherited
> from another Linux package, which will be build with an own defconfig or
> configuration changes.
> (make-defconfig): Function to get a defconfig from an uri.
> ---
>  gnu/packages/linux.scm |  123 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 123 insertions(+)

Looks ok to me, though to say I fully understand it would be a stretch. :)


> gnu: raspberry-pi: Add defconfig objects to build customized Linux kernels.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> gnu/packages/raspberry-pi.scm (make-raspi-defconig): New function to make
> downloaded defconfig objects from the Linux repository of the Raspberry Pi
> Foundation.
> (%bcm2709-defconfig, %bcm2711-defconfig, %bcm2711-defconfig-64,
> %bcmrpi3-defconfig): New variables containing defconfig objects to build
> Linux kernels customized for Raspberry Pi single board computers.
> ---
>  gnu/packages/raspberry-pi.scm |   37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)

Seems good. I think I even understand this one!


> gnu: raspberry-pi: Add helpers for config.txt file generation.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> * gnu/packages/raspberry-pi.scm (raspi-config-file, raspi-custom-txt):
> New functions.
> (%raspi-config-txt, %raspi-bcm27-dtb-txt, %raspi-bcm28-dtb-txt
> %raspi-u-boot-bootloader-txt): New variables.
> ---
>  gnu/packages/raspberry-pi.scm |   53 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)

Seems good.


> From: Stefan <stefan-guix@vodafonemail.de>
>
> * gnu/packages/raspberry-pi.scm (make-raspi-bcm28-dtbs): New function to make
> a package with device-tree files for Raspberry Pi models from the kernel given
> as argument.
> ---
>  gnu/packages/raspberry-pi.scm |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/gnu/packages/raspberry-pi.scm b/gnu/packages/raspberry-pi.scm
> index 12a919d5c6..92f5d22677 100644
> --- a/gnu/packages/raspberry-pi.scm
> +++ b/gnu/packages/raspberry-pi.scm
> @@ -30,6 +30,7 @@
>    #:use-module (gnu packages file)
>    #:use-module (gnu packages gcc)
>    #:use-module (gnu packages linux)
> +  #:use-module (guix build-system copy)
>    #:use-module (guix build-system gnu)
>    #:use-module (guix download)
>    #:use-module (guix git-download)
> @@ -291,6 +292,26 @@ CONTENT can be a list of strings, which are concatenated with a newline
>  character.  Alternatively CONTENT can be a string with the full file content."
>    (raspi-config-file "custom.txt" content))
>  
> +(define-public (make-raspi-bcm28-dtbs linux)
> +  "Make a package with the device-tree files for Raspberry Pi models from the
> +kernel LINUX."
> +  (package
> +    (inherit linux)
> +    (name "raspi-bcm28-dtbs")
> +    (source #f)
> +    (build-system copy-build-system)
> +    (arguments
> +     `(#:phases (modify-phases %standard-phases (delete 'unpack))
> +       #:install-plan
> +       (list (list (string-append (assoc-ref %build-inputs "linux")
> +                                  "/lib/dtbs/broadcom/")
> +                   "." #:include-regexp '("/bcm....-rpi.*\\.dtb")))))
> +    (inputs `(("linux" ,linux)))
> +    (synopsis "Device-tree files for a Raspberry Pi")
> +    (description
> +     (simple-format #f "The device-tree files for Raspberry Pi models from ~a."
> +             (package-name linux)))))
> +
>  (define (make-raspi-defconfig arch defconfig sha256-as-base32)
>    "Make for the architecture ARCH a file-like object from the DEFCONFIG file
>  with the hash SHA256-AS-BASE32.  This object can be used as the #:defconfig
> gnu: raspberry-pi: Add a bootloader-chain for the Raspberry Pi and os examples.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> * gnu/packages/raspberry-pi.scm (grub-efi-bootloader-chain-raspi-64): New
> bootloader variable, capable to boot a Raspberry Pi over network or from a
> local storage.
> * gnu/system/examples/raspberry-pi-64.tmpl: New operating-system example.
> * gnu/system/examples/raspberry-pi-64-nfs-root.tmpl: New operating-system
> example for booting over network.

I'd split this into two commits, one adding
grub-efi-bootloader-chain-raspi-64, and one adding examples using it,
but that is really a judgement call.


live well,
  vagrant
Stefan Oct. 9, 2022, 1:41 p.m. UTC | #10
Hi Vagrant!

>> The function modify-defconfig in guix/build/kconfig.scm no longer
>> interprets "CONFIG_XY=" like "# CONFIG_XY is not set".
> 
> Nervous about how that actually works, but hopefully it plays out correctly.

This is described in the doc-string of config-string->pair in the module (guix build kconfig), which contains a regular expression for the parsing.

Basically any “# CONFIG_X is not set” or “CONFIG_X” is treated as unset and produces a ("CONFIG_X" . #f). The latter is just for convenience, as the first one is hard to remember and easy to get wrong. Any “CONFIG=…” is treated as set and produces a ("CONFIG_X" . "…").

Anything else except comments with “#…” or empty lines will throw an error.

In a previous patch “CONFIG_X=“ was also treated as unset, which was confusing, as this is actually a valid makefile assignment.

The function pair->config-string produces a “# CONFIG_X is not set” for any #f value, or a proper assignment otherwise.

> This whole patch series is large and overwhelming and at least a bit
> beyond my abilities to wrap my head around (which has certainly caused
> me to wait a bit to review)... so I cannot possibly comment on weather
> the series as a whole is "good", through no fault of the patch
> author(s)! I will try and comment where I can, but really need help to
> review it in any meaningful way.

I’ll try to help.

> Also from what I recall on earlier iterations of this patch series,
> different reviewers seemed to have differing style recommendations
> around weather to split patches into smaller commits or merge patches
> into combined commits, which can surely be frustrating. I don't *want*
> to be frustrating, but I lean towards splitting patches into as small a
> commit as possible to wrap my head around the distinct ideas.
> 
> I also like to refactor out anything that can be applied directly to
> master as soon as possible (e.g. the description-appending patches look
> promising for that) with the hope to get the remaining patch series
> smaller and smaller with each iteration. Some people may want to do an
> all-or-nothing merge. I don't know what's "right" for the guix
> community…

The single patch files can be applied separately, they don’t break anything. Some reordering is also possible. In particular, 02-gnu-bootloader-rework-chaining.patch (the monster) can be applied later but before 09-gnu-raspberry-pi-add-a.patch.

> With all that out of the way... here goes my attempt to review!
> 
> 
>> gnu: linux: Fix the extra-version parameter in make-linux-libre*.

> Overall, this looks good, to me, though have one question…

Great! :-)

> Why is native-inputs removed from the 'install phase? Is it no longer
> needed? Was it not actually needed before? I see no mention of this
> change in the comment.

Exactly, it was even not needed before, an obsolete argument, which I removed. I figured this out by chance when selecting the needed arguments for the 'set-environment phase.

>> gnu: bootloader: Rework chaining, add grub-efi-netboot-removable-bootloader.

> There is too much going on here for me to follow, but it is perhaps just
> doing a lot of big changes… help? :)

As noted, this patch can be delayed before 09-gnu-raspberry-pi-add-a.patch is to be applied.

Before this patch, the bootloader-installer of efi-bootloader-chain called grub-mknetdir (actually the installer of grub-efi-netboot-bootloader) and copied the content of a special collection folder of the bootloader-profile. That collection folder was very special and did not fit well to the bootloader-profile idea. Well, actually the grub-efi package with all its tools being part of the profile was the problem, making the collection folder necessary. 

With this patch the packages of grub-efi-netboot-bootloader and grub-efi-netboot-removable-bootloader are already pre-installed – grub-mknetdir is already called during package creation. Their installer just copies the whole package/profile into the target directory. The efi-bootloader-chain only creates a profile with the bootloader (e.g. grub-efi-netboot-bootloader) and additional packages or files. The collection folder is not needed any more. Most complexity got moved from the bootloader installation time to the package build time.

Other patches generate more “pre-installed“ files like u-boot.bin, config.txt, device-tree files, etc., which now all fit much bettor to the bootloader-profile idea, to just be copied to a target directory.

This patch is also inspired by older comments. Maybe take a look at the comments below <https://issues.guix.gnu.org/issue/41066#25> and especially at <https://issues.guix.gnu.org/issue/41066#28-lineno18>

I don’t think splitting this into smaller parts is possible without a breakage.  

>> build: kconfig: Add new module to modify defconfig files.

> I like how this simplifies the various u-boot-* package definitions!

Great! :-)

>> gnu: bootloader: Add U-Boot packages for Raspberry Pi models.

> This seems good to me.

Great! :-)

> It would be nice to first switch the existing u-boot-* packages to use
> the new append-description feature one commit (I think this could even
> be applied directly to master?), and then add the new functionality
> (e.g. make-u-boot-bin-package, *u-boot-rpi-*, etc.) in another commit or
> a couple commits. At least, that would make it a little easier for me to
> read.

Splitting is possible.

>> gnu: linux: New function to modify the configuration of a Linux kernel.

> Looks ok to me, though to say I fully understand it would be a stretch. :)

The idea here is very similar to the use for u-boot: Take some Linux, pretend some defconfig being used, do simple modifications to it, and verify the result.

The defconfig can be provided via #:defconfig (any file-like-object or a file-name) or will otherwise be generated with “make savedefconfig”.

The function (make-defconfig) is a helper to use a defconfig file from some url. 

The function system->linux-srcarch is needed to locate existing defconfig files in the Linux sources like arch/arm/configs/bcm2835_defconfig, so that you can just pass the filename “bcm2835_defconfig” to #:defconfig. This enables the use of defconfig files existing in the Linux sources. As Kbuild expects defconfig files at a certain location, this function is also needed to copy an own defconfig file there. 

There was once a blog post about a custom Linux kernel stating “Suggestions and contributions toward working toward a satisfactory custom initrd and kernel are welcome!”, see <https://guix.gnu.org/de/blog/2019/creating-and-using-a-custom-linux-kernel-on-guix-system/>. This is my take including a verification for the kernel. :-)

>> gnu: raspberry-pi: Add defconfig objects to build customized Linux kernels.

> Seems good. I think I even understand this one!

Great! :-)

>> gnu: raspberry-pi: Add helpers for config.txt file generation.

> Seems good.

Great! :-)

>> gnu: raspberry-pi: Add a bootloader-chain for the Raspberry Pi and os examples.

> I'd split this into two commits, one adding
> grub-efi-bootloader-chain-raspi-64, and one adding examples using it,
> but that is really a judgement call.

True. Combining the example with the new function was my choice. If you don't mind, I'd keep it this way.

Thanks a lot for the review, Vagrant!

How to proceed from here? 

I'd suggest to postpone 02-gnu-bootloader-rework-chaining.patch and 09-gnu-raspberry-pi-add-a.patch, until all others got merged.

To me it seems possible to commit these patches as they are in this order:

01-gnu-linux-fix-extra-version.patch
03-build-kconfig-add-new-module.patch
05-gnu-linux-new-function-to.patch
06-gnu-raspberry-pi-add-defconfig.patch
07-gnu-raspberry-pi-add-helpers.patch
08-gnu-raspberry-pi-new-function.patch

Then I could send a reduced patch-series with:

splitted 04-gnu-bootloader-add-u-boot.patch
02-gnu-bootloader-rework-chaining.patch
09-gnu-raspberry-pi-add-a.patch

What do you think?


Bye

Stefan
phodina Oct. 30, 2022, 12:39 p.m. UTC | #11
Hi Stefan,

I've recently encoutered an issue on Raspberry Pi 4 when attaching USB Hard drive.

I formatted it with fat32 and ext4. Ran `guix system init config.scm /init`.

I selected `(initrd-modules (list "xhci_pci" "pcie_brcmstb"))` in the config and rebooted.

Unfortunately I get error about missing file:

```
error: file /gnu/store/xxx-raw-initrd/initrd.cpio.gz not found
Press any key to continue ...
```

After that I get kernel panic.

Shoudn't the initrd be placed in the EFI partition? That way you get the modules in order to  mount the rootfs.

Should I copy it and modify the Grub entry?

The goal here is to run completly from USB then there won't be any need for SD card.

----
Petr
Stefan Oct. 30, 2022, 5:08 p.m. UTC | #12
Hi Petr!

> Shoudn't the initrd be placed in the EFI partition?

No. As on a PC, the /boot/grub/grub.cfg lists the location of the initrd in the store, as it does for the kernel as well. Grub is able to read from most file-systems and load the initrd.

> Should I copy it and modify the Grub entry?

No.

> Unfortunately I get error about missing file:
> 
> ```
> error: file /gnu/store/xxx-raw-initrd/initrd.cpio.gz not found
> Press any key to continue ...
> ```

Is this error from GRUB? Is that file actually existing?

> I formatted it with fat32 and ext4. Ran `guix system init config.scm /init`.
> 
> I selected `(initrd-modules (list "xhci_pci" "pcie_brcmstb"))` in the config and rebooted.

Was there maybe some error message about missing kernel-modules?

Was the GRUB screen as usual with a background graphic? Because that is loaded from the store as well.

If not, then GRUB is not able to access the store. Check the content of /boot/efi/efi/boot/grub.cfg. It contains two lines: one to search for the right file-system containing the /boot/grub/grub.cfg, and another to load it.

The search line is usually referring to an fs-uuid determined during the bootloader installation with the help of the grub-probe command. However, there is the risk grub-probe does not find an fs-uuid. In that case as a fallback the search command will look for a /boot/grub/grub.cfg on any readable file-system. Maybe it found one on a wrong file-system?

> The goal here is to run completly from USB then there won't be any need for SD card.


From what you wrote, U-Boot is able to boot from USB and load GRUB. I’m just not absolutely sure, if GRUB is able to boot from USB on the RPi 4, but I would think so.


Bye

Stefan
Stefan Oct. 30, 2022, 5:31 p.m. UTC | #13
Hi Petr!

It must be that your initrd is missing in the store.

The /boot/grub/grub.cfg is the only file referring to the initrd and GRUB is the only program reading it. So an error message referring to the initrd must be from GRUB and then GRUB was able to access your root-file-system, which should contain /boot and /gnu/store.


Bye

Stefan

> Am 30.10.2022 um 18:08 schrieb Stefan <stefan-guix@vodafonemail.de>:
> 
> Hi Petr!
> 
>> Shoudn't the initrd be placed in the EFI partition?
> 
> No. As on a PC, the /boot/grub/grub.cfg lists the location of the initrd in the store, as it does for the kernel as well. Grub is able to read from most file-systems and load the initrd.
> 
>> Should I copy it and modify the Grub entry?
> 
> No.
> 
>> Unfortunately I get error about missing file:
>> 
>> ```
>> error: file /gnu/store/xxx-raw-initrd/initrd.cpio.gz not found
>> Press any key to continue ...
>> ```
> 
> Is this error from GRUB? Is that file actually existing?
> 
>> I formatted it with fat32 and ext4. Ran `guix system init config.scm /init`.
>> 
>> I selected `(initrd-modules (list "xhci_pci" "pcie_brcmstb"))` in the config and rebooted.
> 
> Was there maybe some error message about missing kernel-modules?
> 
> Was the GRUB screen as usual with a background graphic? Because that is loaded from the store as well.
> 
> If not, then GRUB is not able to access the store. Check the content of /boot/efi/efi/boot/grub.cfg. It contains two lines: one to search for the right file-system containing the /boot/grub/grub.cfg, and another to load it.
> 
> The search line is usually referring to an fs-uuid determined during the bootloader installation with the help of the grub-probe command. However, there is the risk grub-probe does not find an fs-uuid. In that case as a fallback the search command will look for a /boot/grub/grub.cfg on any readable file-system. Maybe it found one on a wrong file-system?
> 
>> The goal here is to run completly from USB then there won't be any need for SD card.
> 
> 
> From what you wrote, U-Boot is able to boot from USB and load GRUB. I’m just not absolutely sure, if GRUB is able to boot from USB on the RPi 4, but I would think so.
> 
> 
> Bye
> 
> Stefan
Maxim Cournoyer Dec. 1, 2022, 2:25 p.m. UTC | #14
Hi Stefan!

Some comments/question for this proposed change.

Stefan <stefan-guix@vodafonemail.de> writes:

[...]

> gnu: linux: Fix the extra-version parameter in make-linux-libre*.

This first commit LGTM.  I'll push it shortly.

[...]

> gnu: bootloader: Rework chaining, add grub-efi-netboot-removable-bootloader.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> * doc/guix.texi (Bootloader Configuration): Describe the new
> ‘grub-efi-netboot-removable-bootloader’.  Mention used sub-directories and
> that the UEFI Boot Manager is not modified.  Advice to disable write-access
> over TFTP.
> * gnu/bootloader.scm (efi-bootloader-profile): Allow a list of packages and
> collect everything directly in the profile, avoiding a separate collection
> directory.  Renamed the profile from "bootloader-profile" to
> "efi-bootloader-profile".
> [bootloader-collection]: Renamed to …
> [efi-bootloader-profile-hook]: … this and removed unused modules and the
> creation of the now unneeded collection directory.
> (efi-bootloader-chain): Added packages and disk-image-installer arguments.
> Removed handling of the collection directory, now only calling the given
> installer procedure.
> * gnu/bootloader/grub.scm (make-grub-efi-netboot-installer): New helper.
> (make-grub-configuration): New helper based on (grub-configuration-file).
> Adding grub argument, fixed indentation, removend code to get grub.
> (grub-configuration-file): Now using (make-grub-configuration).
> (grub-efi-configuration-file): New function using (make-grub-configuration).
> Instead of getting the grub-efi package from the bootloader-configuration
> this function refers to the grub-efi package directly.
> (grub-cfg): New variable to replace "/boot/grub/grub.cfg".
> (install-grub-efi-netboot): Removed, the functionality got moved.
> (make-grub-efi-netboot-installer): New helper function to return a customized
> installer for a certain efi-sub-directory.  The installer basically copies
> a pre-installed efi-bootloader-profile, and adds needed symlinks for booting
> over network, or – on an ESP – an intermediate grub-cfg to load the final
> grub-cfg file.
> (grub-bootloader): Now using the grub-cfg variable.
> (grub-efi-bootloader): Now using the grub-cfg variable.  Removed inheritance,
> giving complete set of fields.
> (make-grub-efi-netboot-bootloader): New helper function.
> (grub-efi-netboot-bootloader): Now using the helper.
> (grub-efi-netboot-removable-bootloader): New bootloader using the helper.
> It uses the efi-sub-directory "efi/boot" for removable media.
> * gnu/packages/bootloaders.scm (make-grub-efi-netboot): New function to return
> a grub-efi package pre-installed via grub-mknetdir, customized for an
> efi-sub-directory and able to boot via network and local storage.
>
> The rework allows to use an (efi-bootloader-chain) like this, which is able
> to boot over network or local storage, depending on the symlink-support at
> the bootloader-target:
>
> (operating-system
>  (bootloader
>    (bootloader-configuration
>      (bootloader
>        (efi-bootloader-chain
>          grub-efi-netboot-removable-bootloader
>          #:packages (list my-firmware-package
>                           my-u-boot-package)
>          #:files (list (plain-file "config.txt"
>                                    "kernel=u-boot.bin"))
>          #:hooks my-special-bootloader-profile-manipulator))
>      (target "/booti/efi")
>      …))
>  …)
> )

That's *a lot* of text :-). For the future, some of the things there are
improvements rather than necessary changes it seems, so could have been
split into something different, smaller & easier to review.  I've
standardized to use the imperative tense in the change log message (Added
-> Add for example).

[...]

> +(define (grub-configuration-file config . args)
> +  (let* ((bootloader (bootloader-configuration-bootloader config))
> +         (grub (bootloader-package bootloader)))
> +    (apply make-grub-configuration grub config args)))
> +
> +(define (grub-efi-configuration-file . args)
> +  (apply make-grub-configuration grub-efi args))
> +
> +(define grub-cfg "/boot/grub/grub.cfg")

In GRUB-EFI-CONFIGURATION-FILE above, why do we hard-code grub-efi
instead of retrieving it from config the same as for
GRUB-CONFIGURATION-FILE?  It seems that'd be preferable, as otherwise
someone cannot override GRUB-EFI with their own variant, no?

>  
>  
>  ;;;
> @@ -674,42 +681,31 @@ fi~%"))))
>                                ((target-arm?) "--target=arm-efi"))
>                          "--efi-directory" target-esp)))))
>  
> -(define (install-grub-efi-netboot subdir)
> -  "Define a grub-efi-netboot bootloader installer for installation in SUBDIR,
> -which is usually efi/Guix or efi/boot."
> -  (let* ((system (string-split (nix-system->gnu-triplet
> -                                (or (%current-target-system)
> -                                    (%current-system)))
> -                               #\-))
> -         (arch (first system))
> -         (boot-efi-link (match system
> -                          ;; These are the supportend systems and the names
> -                          ;; defined by the UEFI standard for removable media.
> -                          (("i686" _ ...)        "/bootia32.efi")
> -                          (("x86_64" _ ...)      "/bootx64.efi")
> -                          (("arm" _ ...)         "/bootarm.efi")
> -                          (("aarch64" _ ...)     "/bootaa64.efi")
> -                          (("riscv" _ ...)       "/bootriscv32.efi")
> -                          (("riscv64" _ ...)     "/bootriscv64.efi")
> -                          ;; Other systems are not supported, although defined.
> -                          ;; (("riscv128" _ ...) "/bootriscv128.efi")
> -                          ;; (("ia64" _ ...)     "/bootia64.efi")
> -                          ((_ ...)               #f)))
> -         (core-efi (string-append
> -                    ;; This is the arch dependent file name of GRUB, e.g.
> -                    ;; i368-efi/core.efi or arm64-efi/core.efi.
> -                    (match arch
> -                      ("i686"    "i386")
> -                      ("aarch64" "arm64")
> -                      ("riscv"   "riscv32")
> -                      (_         arch))
> -                    "-efi/core.efi")))
> -    (with-imported-modules
> -     '((guix build union))
> -     #~(lambda (bootloader target mount-point)
> -         "Install the BOOTLOADER, which must be the package grub, as e.g.
> -bootx64.efi or bootaa64.efi into SUBDIR, which is usually efi/Guix or efi/boot,
> -below the directory TARGET for the system whose root is mounted at MOUNT-POINT.
> +(define* (make-grub-efi-netboot-installer grub-efi grub-cfg subdir)
> +  "Make a bootloader-installer for a grub-efi-netboot bootloader, which expects
> +its files in SUBDIR and its configuration file in GRUB-CFG.
> +
> +As a grub-efi-netboot package is already preinstalled by 'grub-mknetdir', the
> +installer basically copies all files from the bootloader-package (or profile)
> +into the bootloader-target directory.
> +
> +Additionally for network booting over TFTP, two relative symlinks to the store
> +and to the GRUB-CFG file are necessary.  Due to this a TFTP root directory must
> +not be located on a FAT file-system.
> +
> +If the bootloader-target does not support symlinks, then it is assumed to be a
> +kind of EFI System Partition (ESP).  In this case an intermediate configuration
> +file is created with the help of GRUB-EFI to load the GRUB-CFG.
> +
> +The installer is usable for any efi-bootloader-chain, which prepares the
> +bootloader-profile in a way ready for copying.
> +
> +The installer does not manipulate the system's 'UEFI Boot Manager'."
> +  (with-imported-modules '((guix build union))
> +    #~(lambda (bootloader target mount-point)
> +        "Copy the BOOTLOADER, which must be a preinstalled grub-efi-netboot
> +package with a SUBDIR like efi/boot or efi/Guix, below the directory
> +TARGET for the system whose root is mounted at MOUNT-POINT.
>  
>  MOUNT-POINT is the last argument in 'guix system init /etc/config.scm mnt/point'
>  or '/' for other 'guix system' commands.
> @@ -719,17 +715,18 @@ bootloader-configuration in:

I've unified the above docstring as one; otherwise it was mangled with
Scheme and it wouldn't have appeared as a whole in the online
documentation system of Guile.

I've improved the writing a bit (I think!), use gexps in some places,
and other smallish changes that amount to:

--8<---------------cut here---------------start------------->8---
4 files changed, 80 insertions(+), 76 deletions(-)
doc/guix.texi                | 17 +++++++++--------
gnu/bootloader.scm           | 11 +++++------
gnu/bootloader/grub.scm      | 77 ++++++++++++++++++++++++++++++++++++++++-------------------------------------
gnu/packages/bootloaders.scm | 51 ++++++++++++++++++++++++++-------------------------

modified   doc/guix.texi
@@ -38083,17 +38083,18 @@ NFS servers, you also need a properly configured DHCP server to make the booting
 over netboot possible.  For all this we can currently only recommend you to look
 for instructions about @acronym{PXE, Preboot eXecution Environment}.
 
-If a local EFI System Partition (ESP) or a similar partition with a FAT file
-system is mounted in @code{targets}, then symlinks cannot be created.  In this
-case everything will be prepared for booting from local storage, simialar as if
-using @code{grub-efi-bootloader}, with the difference that all GRUB binaries
-reside on @code{targets}, too, like needed for booting over network.
+If a local EFI System Partition (ESP) or a similar partition with a FAT
+file system is mounted in @code{targets}, then symlinks cannot be
+created.  In this case everything will be prepared for booting from
+local storage, matching the behavior of @code{grub-efi-bootloader}, with
+the difference that all GRUB binaries are copied to @code{targets},
+necessary for booting over the network.
 
 @vindex grub-efi-netboot-removable-bootloader
 @code{grub-efi-netboot-removable-bootloader} is identical to
-@code{grub-efi-netboot-bootloader} with the exception that the sub-directory
-@file{efi/boot} will be used instead of @file{efi/Guix} to comply to the UEFI
-specification for removable media.
+@code{grub-efi-netboot-bootloader} with the exception that the
+sub-directory @file{efi/boot} will be used instead of @file{efi/Guix} to
+comply with the UEFI specification for removable media.
 
 @quotation Note
 This @emph{will} overwrite the GRUB file from any other operating systems that
modified   gnu/bootloader.scm
@@ -361,8 +361,7 @@ (define name-ends-with-/? (cut string-suffix? "/" <>))
             (define (name-is-store-entry? name)
               "Return #t if NAME is a direct store entry and nothing inside."
               (not (string-index (strip-store-file-name name) #\/)))
-            (let* ((output #$output)
-                   (files '#$files)
+            (let* ((files '#$files)
                    (directories (filter name-ends-with-/? files))
                    (names-from-directories
                     (append-map (lambda (directory)
@@ -370,11 +369,11 @@ (define (name-is-store-entry? name)
                                 directories))
                    (names (append names-from-directories
                                   (remove name-ends-with-/? files))))
-              (mkdir-p output)
+              (mkdir-p #$output)
               (if (every file-exists? names)
                   (begin
                     (for-each (lambda (name)
-                               (symlink-to name output
+                               (symlink-to name #$output
                                             (if (name-is-store-entry? name)
                                                 strip-store-file-name
                                                 basename)))
@@ -410,7 +409,7 @@ (define* (efi-bootloader-chain final-bootloader
 The package of the FINAL-BOOTLOADER and all PACKAGES and FILES will be placed
 in an efi-bootloader-profile, which will be passed to the INSTALLER.
 
-FILES may contain file like objects produced by procedures like plain-file,
+FILES may contain file-like objects produced by procedures like plain-file,
 local-file, etc., or package contents produced with file-append.
 
 If a directory name in FILES ends with '/', then the directory content instead
@@ -424,7 +423,7 @@ (define* (efi-bootloader-chain final-bootloader
 FINAL-BOOTLOADER will be called.
 
 If the DISK-IMAGE-INSTALLER is used, then this gexp procedure will be called
-to install the efi-bootloader-profile into a disk-image.  Otherwise the
+to install the efi-bootloader-profile into a disk image.  Otherwise the
 disk-image-installer of the FINAL-BOOTLOADER will be called."
   (bootloader
     (inherit final-bootloader)
modified   gnu/bootloader/grub.scm
@@ -685,7 +685,7 @@ (define* (make-grub-efi-netboot-installer grub-efi grub-cfg subdir)
   "Make a bootloader-installer for a grub-efi-netboot bootloader, which expects
 its files in SUBDIR and its configuration file in GRUB-CFG.
 
-As a grub-efi-netboot package is already preinstalled by 'grub-mknetdir', the
+As a grub-efi-netboot package is already pre-installed by 'grub-mknetdir', the
 installer basically copies all files from the bootloader-package (or profile)
 into the bootloader-target directory.
 
@@ -700,12 +700,12 @@ (define* (make-grub-efi-netboot-installer grub-efi grub-cfg subdir)
 The installer is usable for any efi-bootloader-chain, which prepares the
 bootloader-profile in a way ready for copying.
 
-The installer does not manipulate the system's 'UEFI Boot Manager'."
-  (with-imported-modules '((guix build union))
-    #~(lambda (bootloader target mount-point)
-        "Copy the BOOTLOADER, which must be a preinstalled grub-efi-netboot
-package with a SUBDIR like efi/boot or efi/Guix, below the directory
-TARGET for the system whose root is mounted at MOUNT-POINT.
+The installer does not manipulate the system's 'UEFI Boot Manager'.
+
+The returned installer accepts the BOOTLOADER, TARGET and MOUNT-POINT
+arguments.  Its job is to copy the BOOTLOADER, which must be a pre-installed
+grub-efi-netboot package with a SUBDIR like efi/boot or efi/Guix, below the
+directory TARGET for the system whose root is mounted at MOUNT-POINT.
 
 MOUNT-POINT is the last argument in 'guix system init /etc/config.scm mnt/point'
 or '/' for other 'guix system' commands.
@@ -720,13 +720,14 @@ (define* (make-grub-efi-netboot-installer grub-efi grub-cfg subdir)
  …)
 
 TARGET is required to be an absolute directory name, usually mounted via NFS,
-and finally needs to be provided by a TFTP server as the TFTP root directory.
+and finally needs to be provided by a TFTP server as
+the TFTP root directory.
 
 Usually the installer will be used to prepare network booting over TFTP.  Then
 GRUB will load tftp://server/SUBDIR/grub.cfg and this file will instruct it to
 load more files from the store like tftp://server/gnu/store/…-linux…/Image.
 
-To make this possible two symlinks will be created.  The first symlink points
+To make this possible two symlinks are created.  The first symlink points
 relatively form MOUNT-POINT/TARGET/SUBDIR/grub.cfg to
 MOUNT-POINT/boot/grub/grub.cfg, and the second symlink points relatively from
 MOUNT-POINT/TARGET/%store-prefix to MOUNT-POINT/%store-prefix.
@@ -740,16 +741,18 @@ (define* (make-grub-efi-netboot-installer grub-efi grub-cfg subdir)
 accesses outside its TFTP root directory.  This all may need to be considered
 for security aspects.  It is advised to disable any TFTP write access!
 
-The installer can also be used to prepare booting from local storages, if the
+The installer can also be used to prepare booting from local storage, if the
 underlying file-system, like FAT on an EFI System Partition (ESP), does not
 support symlinks.  In this case the MOUNT-POINT/TARGET/SUBDIR/grub.cfg will be
 created with the help of GRUB-EFI to load the /boot/grub/grub.cfg file.  A
 symlink to the store is not needed in this case."
+  (with-imported-modules '((guix build union))
+    #~(lambda (bootloader target mount-point)
         ;; In context of a disk image creation TARGET will be #f and an
         ;; installer is expected to do necessary installations on MOUNT-POINT,
-        ;; which will become the root file system.
-        ;; If TARGET is #f, this installer has nothing to do, as it only cares
-        ;; about the EFI System Partition (ESP).
+        ;; which will become the root file system.  If TARGET is #f, this
+        ;; installer has nothing to do, as it only cares about the EFI System
+        ;; Partition (ESP).
         (when target
           (use-modules ((guix build union) #:select (symlink-relative))
                        (ice-9 popen)
@@ -779,35 +782,35 @@ (define* (make-grub-efi-netboot-installer grub-efi grub-cfg subdir)
             (mkdir-p (dirname grub-cfg-link))
             (false-if-exception (delete-file grub-cfg-link))
             (if (unspecified?
-                (false-if-exception (symlink-relative grub-cfg grub-cfg-link)))
-              ;; Symlinks are supported.
-              (begin
-                ;; Prepare the symlink to the store.
-                (mkdir-p (dirname store-link))
-                (false-if-exception (delete-file store-link))
-                (symlink-relative store store-link))
-              ;; Creating symlinks does not seem to be supported.
-              ;; Probably an ESP is used.
-              ;; Instead we can script to search and load the actual grub.cfg.
-              (let* ((probe #$(file-append grub-efi "/sbin/grub-probe"))
-                     (port
-                       (open-pipe* OPEN_READ probe "--target=fs_uuid" grub-cfg))
-                     (search-root
-                       (match (read-line port)
-                         ((? eof-object?)
+                 (false-if-exception (symlink-relative grub-cfg grub-cfg-link)))
+                ;; Symlinks are supported.
+                (begin
+                  ;; Prepare the symlink to the store.
+                  (mkdir-p (dirname store-link))
+                  (false-if-exception (delete-file store-link))
+                  (symlink-relative store store-link))
+                ;; Creating symlinks does not seem to be supported.  Probably
+                ;; an ESP is used.  Add a script to search and load the actual
+                ;; grub.cfg.
+                (let* ((probe #$(file-append grub-efi "/sbin/grub-probe"))
+                       (port (open-pipe* OPEN_READ probe "--target=fs_uuid"
+                                         grub-cfg))
+                       (search-root
+                        (match (read-line port)
+                          ((? eof-object?)
                            ;; There is no UUID available. As a fallback search
                            ;; everywhere for the grub.cfg.
                            (string-append "search --file --set " #$grub-cfg))
-                         (fs-uuid
+                          (fs-uuid
                            ;; The UUID to load the grub.cfg from is known.
                            (string-append "search --fs-uuid --set " fs-uuid))))
-                     (load-grub-cfg (string-append "configfile " #$grub-cfg)))
-                (close-pipe port)
-                (with-output-to-file grub-cfg-link
-                  (lambda ()
-                    (display (string-join (list search-root
-                                                load-grub-cfg)
-                                          "\n")))))))))))
+                       (load-grub-cfg (string-append "configfile " #$grub-cfg)))
+                  (close-pipe port)
+                  (with-output-to-file grub-cfg-link
+                    (lambda ()
+                      (display (string-join (list search-root
+                                                  load-grub-cfg)
+                                            "\n")))))))))))
 
 
 
modified   gnu/packages/bootloaders.scm
@@ -427,8 +427,8 @@ (define-public (make-grub-efi-netboot name subdir)
     (build-system trivial-build-system)
     (arguments
      (let* ((system (string-split (nix-system->gnu-triplet
-                                  (or (%current-target-system)
-                                      (%current-system)))
+                                   (or (%current-target-system)
+                                       (%current-system)))
                                   #\-))
             (arch (first system))
             (boot-efi
@@ -454,29 +454,30 @@ (define-public (make-grub-efi-netboot name subdir)
                          ("riscv"   "riscv32")
                          (_         arch))
                        "-efi/core.efi")))
-       `(#:modules ((guix build utils))
-         #:builder
-         (begin
-           (use-modules (guix build utils))
-           (let* ((bootloader (assoc-ref %build-inputs "grub-efi"))
-                  (net-dir (assoc-ref %outputs "out"))
-                  (sub-dir (string-append net-dir "/" ,subdir "/"))
-                  (boot-efi (string-append sub-dir ,boot-efi))
-                  (core-efi (string-append sub-dir ,core-efi)))
-             ;; Install GRUB, which refers to the grub.cfg, with support for
-             ;; encrypted partitions,
-             (setenv "GRUB_ENABLE_CRYPTODISK" "y")
-             (invoke/quiet (string-append bootloader "/bin/grub-mknetdir")
-                           (string-append "--net-directory=" net-dir)
-                           (string-append "--subdir=" ,subdir)
-                           ;; These modules must be preloaded to allow booting
-                           ;; from an ESP or a similar partition with a FAT
-                           ;; file system.
-                           (string-append "--modules=part_msdos part_gpt fat"))
-             ;; Move GRUB's core.efi to the removable media name.
-             (false-if-exception (delete-file boot-efi))
-             (rename-file core-efi boot-efi))))))
-    (inputs `(("grub-efi" ,grub-efi)))
+       (list
+        #:modules ((guix build utils))
+        #:builder
+        #~(begin
+            (use-modules (guix build utils))
+            (let* ((bootloader #$(this-package-input "grub-efi"))
+                   (net-dir #$output)
+                   (sub-dir (string-append net-dir "/" #$subdir "/"))
+                   (boot-efi (string-append sub-dir #$boot-efi))
+                   (core-efi (string-append sub-dir #$core-efi)))
+              ;; Install GRUB, which refers to the grub.cfg, with support for
+              ;; encrypted partitions,
+              (setenv "GRUB_ENABLE_CRYPTODISK" "y")
+              (invoke/quiet (string-append bootloader "/bin/grub-mknetdir")
+                            (string-append "--net-directory=" net-dir)
+                            (string-append "--subdir=" #$subdir)
+                            ;; These modules must be pre-loaded to allow booting
+                            ;; from an ESP or a similar partition with a FAT
+                            ;; file system.
+                            (string-append "--modules=part_msdos part_gpt fat"))
+              ;; Move GRUB's core.efi to the removable media name.
+              (false-if-exception (delete-file boot-efi))
+              (rename-file core-efi boot-efi))))))
+    (inputs (list grub-efi))
     (synopsis (package-synopsis grub-efi))
     (description (package-description grub-efi))
     (home-page (package-home-page grub-efi))
--8<---------------cut here---------------end--------------->8---

It's a pity we do not have tests for that, but I'll try to test it
manually and if it works I can push it shortly.  I'd still like feedback
on my question above.
Maxim Cournoyer Dec. 1, 2022, 3:32 p.m. UTC | #15
Hi again,

Stefan <stefan-guix@vodafonemail.de> writes:

[...]

> +(define (grub-configuration-file config . args)
> +  (let* ((bootloader (bootloader-configuration-bootloader config))
> +         (grub (bootloader-package bootloader)))
> +    (apply make-grub-configuration grub config args)))
> +
> +(define (grub-efi-configuration-file . args)
> +  (apply make-grub-configuration grub-efi args))

Another question regarding that same piece of code: why isn't
grub-efi-configuration-file used in the definition of the
grub-efi-bootloader definition?  It's still using
grub-configuration-file.
Maxim Cournoyer Dec. 1, 2022, 4:22 p.m. UTC | #16
Hi,

Stefan <stefan-guix@vodafonemail.de> writes:

[...]

> new file mode 100644
> index 0000000000..0cfaffe056
> --- /dev/null
> +++ b/guix/build/kconfig.scm

[...]

> +(define-module (guix build kconfig)
> +  #:use-module  (ice-9 rdelim)
> +  #:use-module  (ice-9 regex)
> +  #:use-module  (srfi srfi-1)
> +  #:use-module  (srfi srfi-26)
> +  #:export (modify-defconfig
> +            verify-config))
> +
> +;; Commentary:
> +;;
> +;; Builder-side code to modify configurations for the Kconfig build system as
> +;; used by Linux and U-Boot.
> +;;
> +;; Code:
> +
> +(define (config-string->pair config-string)
> +  "Parse a configuration string like \"CONFIG_EXAMPLE=m\" into a key-value pair.
> +An error is thrown for invalid configurations.
> +
> +\"CONFIG_A=y\"            -> '(\"CONFIG_A\" . \"y\")
> +\"CONFIG_B=\\\"\\\"\"         -> '(\"CONFIG_B\" . \"\\\"\\\"\")
> +\"CONFIG_C=\"             -> '(\"CONFIG_C\" . \"\")
> +\"# CONFIG_E is not set\" -> '(\"CONFIG_E\" . #f)
> +\"CONFIG_D\"              -> '(\"CONFIG_D\" . #f)
> +\"# Any comment\"         -> '(#f . \"# Any comment\")
> +\"\"                      -> '(#f . \"\")
> +\"# CONFIG_E=y\"          -> (error \"Invalid configuration\")
> +\"CONFIG_E is not set\"   -> (error \"Invalid configuration\")
> +\"Anything else\"         -> (error \"Invalid configuration\")"
> +  (define config-regexp
> +    (make-regexp
> +     ;; (match:substring (string-match "=(.*)" "=") 1) returns "", but the
> +     ;; pattern "=(.+)?" makes it return #f instead.  From a "CONFIG_A=" we like
> +     ;; to get "", which later emits "CONFIG_A=" again.
> +     "^ *(#[\\t ]*)?(CONFIG_[a-zA-Z0-9_]+)([\\t ]*=[\\t ]*(.*)|([\\t ]+is[\\t ]+not[\\t ]+set))?$"))
> +
> +  (define config-comment-regexp
> +    (make-regexp "^([\\t ]*(#.*)?)$"))
> +
> +  (let ((match (regexp-exec config-regexp (string-trim-right config-string))))
> +    (if match
> +        (let* ((comment (match:substring match 1))
> +               (key (match:substring match 2))
> +               (unset (match:substring match 5))
> +               (value (and (not comment)
> +                           (not unset)
> +                           (match:substring match 4))))
> +          (if (eq? (not comment) (not unset))
> +              ;; The key is uncommented and set or commented and unset.
> +              (cons key value)
> +              ;; The key is set or unset ambigiously.
> +              (error (format #f "Invalid configuration, did you mean \"~a\"?"
> +                             (pair->config-string (cons key #f)))
> +                     config-string)))
> +        ;; This is not a valid or ambigious config-string, but mayby a comment.
> +        (if (regexp-exec config-comment-regexp config-string)
> +            ;; We keep valid comments.
> +            (cons #f config-string)
> +            (error "Invalid configuration" config-string)))))
> +
> +(define (pair->config-string pair)
> +  "Convert a PAIR back to a config-string."
> +  (let* ((key (first pair))
> +         (value (cdr pair)))
> +    (if (string? key)
> +        (if (string? value)
> +            (string-append key "=" value)
> +            (string-append "# " key " is not set"))
> +        value)))
> +
> +(define (defconfig->alist defconfig)
> +  "Convert the content of a DEFCONFIG (or .config) file into an alist."
> +  (with-input-from-file defconfig
> +    (lambda ()
> +      (let loop ((alist '())
> +                 (line (read-line)))
> +        (if (eof-object? line)
> +            ;; Building the alist is done, now check for duplicates.
> +            (let loop ((keys (map first (filter first alist)))
                                           ^
What is this filter used for here? EDIT: saw later, it's used to filter
out comments.

[...]

> +(define (verify-config config defconfig)
> +  "Verify that the CONFIG file contains all configurations from the DEFCONFIG
> +file and return #t in this case. Otherwise throw an error with the mismatching
> +keys and their values."
> +  (let* ((config-pairs (defconfig->alist config))
> +         (defconfig-pairs (defconfig->alist defconfig))
> +         (mismatching-pairs
> +          (remove (lambda (pair)
> +                    ;; Remove all configurations, whose values are #f and whose
> +                    ;; keys are not in config-pairs, as not in config-pairs
> +                    ;; means unset, …
> +                    (and (not (cdr pair))
> +                         (not (assoc-ref config-pairs (first pair)))))
> +                  ;; … from the defconfig-pairs different to config-pairs.

So, it finds mismatched configurations that exist in both CONFIG and
DEFCONFIG, but it doesn't error when there are configs that exist in
DEFCONFIG but missing from CONFIG, right?  Should it?

> +                  (lset-difference equal?
> +                                   ;; Remove comments by filtering with first.
> +                                   (filter first defconfig-pairs)
> +                                   config-pairs))))
> +    (if (null? mismatching-pairs)
> +        #t
> +        (error (format #f
> +                       "Mismatching configurations in ~a and ~a"
> +                       config
> +                       defconfig)
> +               (map (lambda (mismatching-pair)
> +                      (let* ((key (first mismatching-pair))
> +                             (defconfig-value (cdr mismatching-pair))
> +                             (config-value (assoc-ref config-pairs key)))
> +                        (cons key (list (list config-value defconfig-value)))))
> +                    mismatching-pairs)))))
>
>

I've made the following mostly cosmetic changes:

--8<---------------cut here---------------start------------->8---
2 files changed, 43 insertions(+), 45 deletions(-)
gnu/packages/bootloaders.scm | 18 +++++++++---------
guix/build/kconfig.scm       | 70 ++++++++++++++++++++++++++++++++++------------------------------------

modified   gnu/packages/bootloaders.scm
@@ -792,9 +792,9 @@ (define*-public (make-u-boot-package board triplet #:key defconfig configs)
                                                      "_" "-")))
       (native-inputs
        `(,@(if (not (same-arch?))
-             `(("cross-gcc" ,(cross-gcc triplet))
-               ("cross-binutils" ,(cross-binutils triplet)))
-             `())
+               `(("cross-gcc" ,(cross-gcc triplet))
+                 ("cross-binutils" ,(cross-binutils triplet)))
+               `())
          ,@(package-native-inputs u-boot)))
       (arguments
        `(#:modules ((ice-9 ftw)
@@ -808,8 +808,8 @@ (define*-public (make-u-boot-package board triplet #:key defconfig configs)
          #:make-flags
          (list "HOSTCC=gcc"
                ,@(if (not (same-arch?))
-                   `((string-append "CROSS_COMPILE=" ,triplet "-"))
-                   '()))
+                     `((string-append "CROSS_COMPILE=" ,triplet "-"))
+                     '()))
          #:phases
          (modify-phases %standard-phases
            (replace 'configure
@@ -828,7 +828,7 @@ (define*-public (make-u-boot-package board triplet #:key defconfig configs)
                        (apply invoke "make" `(,@make-flags ,config-name))
                        (verify-config ".config" config-file))
                      (begin
-                       (display "Invalid board name. Valid board names are:"
+                       (display "invalid board name; valid board names are:"
                                 (current-error-port))
                        (let ((suffix-len (string-length "_defconfig"))
                              (entries (scandir "configs")))
@@ -839,7 +839,7 @@ (define*-public (make-u-boot-package board triplet #:key defconfig configs)
                                                (string-drop-right file-name
                                                                   suffix-len))))
                                    (sort entries string-ci<)))
-                       (error "Invalid boardname ~s." ,board))))))
+                       (error "invalid boardname ~s" ,board))))))
            (add-after 'configure 'disable-tools-libcrypto
              ;; Disable libcrypto due to GPL and OpenSSL license
              ;; incompatibilities
@@ -881,8 +881,8 @@ (define-public u-boot-malta

 (define-public u-boot-am335x-boneblack
   (let ((base (make-u-boot-package "am335x_evm" "arm-linux-gnueabihf"
-               ;; Patch out other device trees to build image small enough to
-               ;; fit within typical partitioning schemes where the first
+               ;; Patch out other device trees to build an image small enough
+               ;; to fit within typical partitioning schemes where the first
                ;; partition begins at sector 2048.
                #:configs '("CONFIG_OF_LIST=\"am335x-evm am335x-boneblack\""))))
     (package
modified   guix/build/kconfig.scm
@@ -50,7 +50,8 @@ (define config-regexp
      ;; (match:substring (string-match "=(.*)" "=") 1) returns "", but the
      ;; pattern "=(.+)?" makes it return #f instead.  From a "CONFIG_A=" we like
      ;; to get "", which later emits "CONFIG_A=" again.
-     "^ *(#[\\t ]*)?(CONFIG_[a-zA-Z0-9_]+)([\\t ]*=[\\t ]*(.*)|([\\t ]+is[\\t ]+not[\\t ]+set))?$"))
+     (string-append "^ *(#[\\t ]*)?(CONFIG_[a-zA-Z0-9_]+)([\\t ]*="
+                    "[\\t ]*(.*)|([\\t ]+is[\\t ]+not[\\t ]+set))?$")))

   (define config-comment-regexp
     (make-regexp "^([\\t ]*(#.*)?)$"))
@@ -67,13 +68,13 @@ (define config-comment-regexp
               ;; The key is uncommented and set or commented and unset.
               (cons key value)
               ;; The key is set or unset ambigiously.
-              (error (format #f "Invalid configuration, did you mean \"~a\"?"
+              (error (format #f "invalid configuration, did you mean \"~a\"?"
                              (pair->config-string (cons key #f)))
                      config-string)))
-        ;; This is not a valid or ambigious config-string, but mayby a comment.
+        ;; This is not a valid or ambigious config-string, but maybe a
+        ;; comment.
         (if (regexp-exec config-comment-regexp config-string)
-            ;; We keep valid comments.
-            (cons #f config-string)
+            (cons #f config-string)     ;keep valid comments
             (error "Invalid configuration" config-string)))))

 (define (pair->config-string pair)
@@ -94,6 +95,7 @@ (define (defconfig->alist defconfig)
                  (line (read-line)))
         (if (eof-object? line)
             ;; Building the alist is done, now check for duplicates.
+            ;; Note: the filter invocation is used to remove comments.
             (let loop ((keys (map first (filter first alist)))
                        (duplicates '()))
               (if (null? keys)
@@ -102,11 +104,11 @@ (define (defconfig->alist defconfig)
                   (if (null? duplicates)
                       alist
                       (error
-                       (format #f "Duplicate configurations in ~a" defconfig)
+                       (format #f "duplicate configurations in ~a" defconfig)
                        duplicates))
                   ;; Continue the search for duplicates.
                   (loop (cdr keys)
-                        (if (member (first keys) (cdr keys) equal?)
+                        (if (member (first keys) (cdr keys))
                             (cons (first keys) duplicates)
                             duplicates))))
             ;; Build the alist.
@@ -127,13 +129,13 @@ (define (modify-defconfig defconfig configs)
   \"CONFIG_D=m\"
   \"CONFIG_E=\"
   \"# CONFIG_G is not set\"
-  ;; For convinience this abbrevation can be used for not set configurations.
+  ;; For convenience this abbrevation can be used for not set configurations.
   \"CONFIG_F\")

-Instead of a list, CONFGIS can be a string with one configuration per line."
-  (let* (;; Split the configs into a list of single configuations.
-         ;; To minimize mistakes, we support a string and a list of strings,
-         ;; each with newlines to separate configurations.
+Instead of a list, CONFIGS can be a string with one configuration per line."
+  (let* (;; Split the configs into a list of single configurations.  Both a
+         ;; string and or a list of strings is supported, each with newlines
+         ;; to separate configurations.
          (config-pairs (map config-string->pair
                             (append-map (cut string-split <>  #\newline)
                                         (if (string? configs)
@@ -141,45 +143,41 @@ (define (modify-defconfig defconfig configs)
                                             configs))))
          ;; Generate a blocklist from all valid keys in config-pairs.
          (blocklist (delete #f (map first config-pairs)))
-         ;; Generate an alist from the defconifg without the keys in blocklist.
+         ;; Generate an alist from the defconfig without the keys in blocklist.
          (filtered-defconfig-pairs (remove (lambda (pair)
                                              (member (first pair) blocklist))
                                            (defconfig->alist defconfig))))
     (with-output-to-file defconfig
       (lambda ()
-        (for-each
-           (lambda (pair)
-             (display (pair->config-string pair))
-             (newline))
-           (append filtered-defconfig-pairs config-pairs))))))
+        (for-each (lambda (pair)
+                    (display (pair->config-string pair))
+                    (newline))
+                  (append filtered-defconfig-pairs config-pairs))))))

 (define (verify-config config defconfig)
   "Verify that the CONFIG file contains all configurations from the DEFCONFIG
-file and return #t in this case. Otherwise throw an error with the mismatching
-keys and their values."
+file.  When the verification fails, raise an error with the mismatching keys
+and their values."
   (let* ((config-pairs (defconfig->alist config))
          (defconfig-pairs (defconfig->alist defconfig))
          (mismatching-pairs
           (remove (lambda (pair)
-                    ;; Remove all configurations, whose values are #f and whose
-                    ;; keys are not in config-pairs, as not in config-pairs
-                    ;; means unset, …
+                    ;; Remove all configurations, whose values are #f and
+                    ;; whose keys are not in config-pairs, as not in
+                    ;; config-pairs means unset, ...
                     (and (not (cdr pair))
                          (not (assoc-ref config-pairs (first pair)))))
-                  ;; … from the defconfig-pairs different to config-pairs.
+                  ;; ... from the defconfig-pairs different to config-pairs.
                   (lset-difference equal?
                                    ;; Remove comments by filtering with first.
                                    (filter first defconfig-pairs)
                                    config-pairs))))
-    (if (null? mismatching-pairs)
-        #t
-        (error (format #f
-                       "Mismatching configurations in ~a and ~a"
-                       config
-                       defconfig)
-               (map (lambda (mismatching-pair)
-                      (let* ((key (first mismatching-pair))
-                             (defconfig-value (cdr mismatching-pair))
-                             (config-value (assoc-ref config-pairs key)))
-                        (cons key (list (list config-value defconfig-value)))))
-                    mismatching-pairs)))))
+    (unless (null? mismatching-pairs)
+      (error (format #f "Mismatching configurations in ~a and ~a"
+                     config defconfig)
+             (map (lambda (mismatching-pair)
+                    (let* ((key (first mismatching-pair))
+                           (defconfig-value (cdr mismatching-pair))
+                           (config-value (assoc-ref config-pairs key)))
+                      (cons key (list (list config-value defconfig-value)))))
+                  mismatching-pairs)))))
--8<---------------cut here---------------end--------------->8---

And streamlined the commit messages as:

--8<---------------cut here---------------start------------->8---
build: kconfig: Add new module to modify defconfig files.

* guix/build/kconfig.scm: New file.
* Makefile.am: Register it.
* gnu/packages/bootloaders.scm (make-u-boot-package)
(make-u-boot-sunxi64-package): Add DEFCONFIGS and CONFIGS arguments.
(u-boot-am335x-boneblack, u-boot-pinebook)
(u-boot-novena,u-boot-rockpro64-rk3399): Simplify packages by using the new
keyword arguments.
--8<---------------cut here---------------end--------------->8---

Explanations don't go in the GNU ChangeLog, they go ideally in comments
in the code or *before* the GNU ChangeLog, if some rationale helps
understanding the change, so you can keep things dry there.

I'll push this change shortly.
Maxim Cournoyer Dec. 1, 2022, 6:01 p.m. UTC | #17
Hi,

> gnu: linux: New function to modify the configuration of a Linux kernel.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> * gnu/packages/linux.scm (system->linux-srcarch): New function to return the
> relevent folder name below arch/ in the Linux source code.
> (modify-linux): New function to make a customized Linux package inherited
> from another Linux package, which will be build with an own defconfig or
> configuration changes.
> (make-defconfig): Function to get a defconfig from an uri.

I've renamed it to customize-linux, and streamlined the commit message
like so:

--8<---------------cut here---------------start------------->8---
gnu: linux: Add a 'customize-linux' procedure.

* gnu/packages/linux.scm (linux-srcarch): New procedure.
(customize-linux): Likewise.
(make-defconfig): Procedure to retrieve a defconfig from an URI.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Modified-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
--8<---------------cut here---------------end--------------->8---

Otherwise, I've made the following changes (exporting procedures
explicitly from the modules):

--8<---------------cut here---------------start------------->8---
1 file changed, 25 insertions(+), 25 deletions(-)
gnu/packages/linux.scm | 50 +++++++++++++++++++++++++-------------------------

modified   gnu/packages/linux.scm
@@ -190,19 +190,19 @@ (define-module (gnu packages linux)
   #:use-module (srfi srfi-2)
   #:use-module (srfi srfi-26)
   #:use-module (ice-9 match)
-  #:use-module (ice-9 optargs)
-  #:use-module (ice-9 regex))
+  #:use-module (ice-9 regex)
+  #:export (customize-linux))
 
-(define-public (linux-srcarch)
+(define (linux-srcarch)
   "Return the linux SRCARCH name, which is set in the toplevel Makefile of
-Linux and denotes the architecture specific directory name below arch/ in its
+Linux and denotes the architecture-specific directory name below arch/ in its
 source code.  Some few architectures share a common folder.  It resembles the
 definition of SRCARCH based on ARCH in the Makefile and may be used to place a
 defconfig file in the proper path."
   (let ((linux-arch (platform-linux-architecture
-                      (lookup-platform-by-target-or-system
-                        (or (%current-target-system)
-                            (%current-system))))))
+                     (lookup-platform-by-target-or-system
+                      (or (%current-target-system)
+                          (%current-system))))))
     (match linux-arch
       ("i386"    "x86")
       ("x86_64"  "x86")
@@ -213,7 +213,7 @@ (define-public (linux-srcarch)
 
 (define-public (system->defconfig system)
   "Some systems (notably powerpc-linux) require a special target for kernel
-defconfig.  Return the appropriate make target if applicable, otherwise return
+defconfig.  Return the appropriate Make target if applicable, otherwise return
 \"defconfig\"."
   (cond ((string-prefix? "powerpc-" system) "pmac32_defconfig")
         ((string-prefix? "powerpc64-" system) "ppc64_defconfig")
@@ -1271,19 +1271,19 @@ (define-public linux-libre-with-bpf
 ;;; Linux kernel customization functions.
 ;;;
 
-(define*-public (modify-linux #:key name
-                                    (linux linux-libre)
-                                    source
-                                    defconfig
-                                    (configs "")
-                                    extra-version)
-  "Make a Linux package NAME as a modification of another LINUX package.
+(define* (customize-linux #:key name
+                          (linux linux-libre)
+                          source
+                          defconfig
+                          (configs "")
+                          extra-version)
+  "Make a customized Linux package NAME derived from the LINUX package.
 
 If NAME is not given, then it defaults to the same name as the LINUX package.
 
 Unless SOURCE is given the source of LINUX is used.
 
-A DEFCONFIG file to be used can be given as an origin, as a file like object
+A DEFCONFIG file to be used can be given as an origin, as a file-like object
 (file-append, local-file etc.), or as a string with the name of a defconfig file
 available in the Linux sources.  If DEFCONFIG is not given, then a defconfig
 file will be saved from the LINUX package configuration.
@@ -1295,11 +1295,11 @@ (define*-public (modify-linux #:key name
 defconfig syntax has to be used, but there is a special extension to ease the
 removal of configurations.  Comment lines are supported as well.
 
-Here is an explaining usage example:
+Here is an example:
 
   '(;; This string defines the version tail in 'uname -r'.
     \"CONFIG_LOCALVERSION=\\\"-handcrafted\\\"
-    ;; This '# CONFIG_… is not set' syntax has to match exactly!
+    ;; This '# CONFIG_... is not set' syntax has to match exactly!
     \"# CONFIG_BOOT_CONFIG is not set\"
     \"CONFIG_NFS_SWAP=y\"
     ;; This is a multiline configuration:
@@ -1339,13 +1339,13 @@ (define*-public (modify-linux #:key name
                   #$(cond
                      ((not defconfig)
                       #~(begin
-                         ;; Call the original 'configure phase.
-                         (apply (assoc-ref #$phases 'configure) arguments)
-                         ;; Save a defconfig file.
-                         (invoke "make" "savedefconfig")
-                         ;; Move the saved defconfig to the proper location.
-                         (rename-file "defconfig"
-                                      guix_defconfig)))
+                          ;; Call the original 'configure phase.
+                          (apply (assoc-ref #$phases 'configure) arguments)
+                          ;; Save a defconfig file.
+                          (invoke "make" "savedefconfig")
+                          ;; Move the saved defconfig to the proper location.
+                          (rename-file "defconfig"
+                                       guix_defconfig)))
                      ((string? defconfig)
                       ;; Use another existing defconfig from the Linux sources.
                       #~(rename-file (string-append configs #$defconfig)
--8<---------------cut here---------------end--------------->8---

I'll push it shortly.
Maxim Cournoyer Dec. 1, 2022, 7:33 p.m. UTC | #18
Hi,

Stefan <stefan-guix@vodafonemail.de> writes:

> gnu: raspberry-pi: Add a bootloader-chain for the Raspberry Pi and os examples.
>
> From: Stefan <stefan-guix@vodafonemail.de>
>
> * gnu/packages/raspberry-pi.scm (grub-efi-bootloader-chain-raspi-64): New
> bootloader variable, capable to boot a Raspberry Pi over network or from a
> local storage.
> * gnu/system/examples/raspberry-pi-64.tmpl: New operating-system example.
> * gnu/system/examples/raspberry-pi-64-nfs-root.tmpl: New operating-system

Neat!

I've registered the new files in Makefile.am (and adjusted the commit
message), fixed their indentation and pushed!

This whole series is now in Guix.  I'll try to test it with actual
devices in the coming days and see if anything needs adjusting.

Thanks to you and to everyone else who contributed to the review.

Maxim
Vagrant Cascadian Dec. 3, 2022, 5:53 a.m. UTC | #19
On 2022-12-01, Maxim Cournoyer wrote:
> Stefan <stefan-guix@vodafonemail.de> writes:
>
>> gnu: raspberry-pi: Add a bootloader-chain for the Raspberry Pi and os examples.
>>
>> From: Stefan <stefan-guix@vodafonemail.de>
>>
>> * gnu/packages/raspberry-pi.scm (grub-efi-bootloader-chain-raspi-64): New
>> bootloader variable, capable to boot a Raspberry Pi over network or from a
>> local storage.
>> * gnu/system/examples/raspberry-pi-64.tmpl: New operating-system example.
>> * gnu/system/examples/raspberry-pi-64-nfs-root.tmpl: New operating-system

This does cause a test suite failure with tests/guix-system.sh:

+ guix system -n disk-image gnu/system/examples/raspberry-pi-64-nfs-root.tmpl
accepted connection from pid 31196, user vagrant
guix system: warning: 'disk-image' is deprecated: use 'image' instead
guix system: error: canonicalize-path: No such file or directory: "/home/vagrant/.ssh/id_ecdsa.pub"
+ rm -f t-guix-system-30549 t-guix-system-error-30549 /tmp/t-guix-system-30549/config.scm /tmp/t-guix-system-30549/my-torrc
+ rmdir /tmp/t-guix-system-30549
FAIL tests/guix-system.sh (exit status: 1)

gnu/system/examples/raspberry-pi-64-nfs-root.tmpl

  (define %my-public-key
    (local-file (string-append (getenv "HOME") "/.ssh/id_ecdsa.pub")))

Seems like using local-file for should be removed or at least commented
out in the example. Or include the full text of an example key in the
.tmpl file instead of using local-file...

live well,
  vagrant
Maxim Cournoyer Dec. 4, 2022, 6:28 a.m. UTC | #20
Hello,

Vagrant Cascadian <vagrant@debian.org> writes:

> On 2022-12-01, Maxim Cournoyer wrote:
>> Stefan <stefan-guix@vodafonemail.de> writes:
>>
>>> gnu: raspberry-pi: Add a bootloader-chain for the Raspberry Pi and os examples.
>>>
>>> From: Stefan <stefan-guix@vodafonemail.de>
>>>
>>> * gnu/packages/raspberry-pi.scm (grub-efi-bootloader-chain-raspi-64): New
>>> bootloader variable, capable to boot a Raspberry Pi over network or from a
>>> local storage.
>>> * gnu/system/examples/raspberry-pi-64.tmpl: New operating-system example.
>>> * gnu/system/examples/raspberry-pi-64-nfs-root.tmpl: New operating-system
>
> This does cause a test suite failure with tests/guix-system.sh:
>
> + guix system -n disk-image gnu/system/examples/raspberry-pi-64-nfs-root.tmpl
> accepted connection from pid 31196, user vagrant
> guix system: warning: 'disk-image' is deprecated: use 'image' instead
> guix system: error: canonicalize-path: No such file or directory: "/home/vagrant/.ssh/id_ecdsa.pub"
> + rm -f t-guix-system-30549 t-guix-system-error-30549 /tmp/t-guix-system-30549/config.scm /tmp/t-guix-system-30549/my-torrc
> + rmdir /tmp/t-guix-system-30549
> FAIL tests/guix-system.sh (exit status: 1)
>
> gnu/system/examples/raspberry-pi-64-nfs-root.tmpl
>
>   (define %my-public-key
>     (local-file (string-append (getenv "HOME") "/.ssh/id_ecdsa.pub")))
>

Thanks for the heads-up!  I've removed key usage from the templates in
08dc9f2ca2e96476aa51c906c8ba01ca5d033568.  I also had to mark the
templates to be built for aarch64-linux in
93309efdce72ac5028944d5c1f7b081a7f62b84a.
diff mbox series

Patch

diff --git a/gnu/system/examples/raspberry-pi-64-nfs-root.tmpl b/gnu/system/examples/raspberry-pi-64-nfs-root.tmpl
y-pi-64-nfs-root.tmpl
index a1e41e3399..279620b0fb 100644
--- a/gnu/system/examples/raspberry-pi-64-nfs-root.tmpl
+++ b/gnu/system/examples/raspberry-pi-64-nfs-root.tmpl
@@ -30,7 +30,7 @@ 
    (timezone "Europe/Berlin")
    (bootloader (bootloader-configuration
                 (bootloader grub-efi-bootloader-chain-raspi-64)
-                (target "/boot/efi")
+                (targets (list "/boot/efi"))
                 (theme (grub-theme (resolution '(1920 . 1080))
                        (image (file-append
                                %artwork-repository
@@ -49,7 +49,10 @@ 
                          (device ":/export/raspberrypi/guix")
                          (options "addr=10.20.30.40,vers=4.1"))
                         %base-file-systems))
-   (swap-devices '("/run/swapfile"))
+    (swap-devices
+     (list
+      (swap-space
+       (target "/run/swapfile"))))
    (users (cons* (user-account
                   (name "pi")
                   (group "users")
diff --git a/gnu/system/examples/raspberry-pi-64.tmpl b/gnu/system/examples/raspberry-pi-64.tmpl
index 7e18f00d86..0739582cf0 100644
--- a/gnu/system/examples/raspberry-pi-64.tmpl
+++ b/gnu/system/examples/raspberry-pi-64.tmpl
@@ -29,7 +29,7 @@ 
    (timezone "Europe/Berlin")
    (bootloader (bootloader-configuration
                 (bootloader grub-efi-bootloader-chain-raspi-64)
-                (target "/boot/efi")
+                (targets (list "/boot/efi"))
                 (theme (grub-theme (resolution '(1920 . 1080))
                        (image (file-append
                                %artwork-repository
@@ -53,7 +53,10 @@ 
                          (type "vfat")
                          (device (file-system-label "EFI")))
                         %base-file-systems))
-   (swap-devices '("/run/swapfile"))
+   (swap-devices
+    (list
+     (swap-space
+      (target "/run/swapfile"))))
    (users (cons* (user-account
                   (name "pi")
                   (group "users")