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

[Stefan via Guix-patches via <guix-patches@gnu.org>]

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