[bug#48314] [PATCH v5] Install guix system on Raspberry Pi

[Vagrant Cascadian <vagrant@debian.org>]

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

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

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