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

[Stefan]

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