On 2023-07-08, Maxim Cournoyer wrote:
> Vagrant Cascadian <vagrant@debian.org> writes:
> [...]
>> The one thing I would probably prefer is to split this into one package
>> per line, but I tried to aim for a smaller diff:
>>
>> -       (prepend python-coverage python-pycryptodomex python-pytest sdl2)))
>> +       (prepend python-coverage python-filelock python-pycryptodomex
>> +python-pytest python-pytest-xdist sdl2)))
>
> Odd indentation; please use something like:
>
>           (prepend package1
>                    package2
>                    ...)

Yup, that is the aproach I would propose for the next and/or final
patch(es)!


>> From d734cc541f920963e8cf8d68061d5329c9712d00 Mon Sep 17 00:00:00 2001
>> From: Vagrant Cascadian <vagrant@debian.org>
>> Date: Sun, 2 Jul 2023 18:20:39 -0700
>> Subject: [PATCH 2/2] gnu: u-boot: Update to 2023.07-rc6.
>>
>> * gnu/packages/patches/u-boot-infodocs-target.patch: Remove file.
>> * gnu/packages/patches/u-boot-patman-guix-integration.patch: Remove
>> file.
>
> Nitpick: I'd use "Delete" here instead of "Remove".

Not my style, but not strongly opinionated either. :)

>> * gnu/local.mk: Remove patches.
>
> Nitpick: I'd use "De-register" instead of remove.

Even less my style, but also not strongly opinionated. :)


>> @@ -778,6 +778,9 @@ (define-public u-boot-tools
>>                             ;; details.
>>                             (("CONFIG_FIT_SIGNATURE=y")
>>                              "CONFIG_FIT_SIGNATURE=n\nCONFIG_UT_LIB_ASN1=n\nCONFIG_TOOLS_LIBCRYPTO=n")
>> +                           ;; Catch instances of implied CONFIG_FIG_SIGNATURE with VPL targets
>> +                           (("CONFIG_SANDBOX_VPL=y")
>> +                            "CONFIG_SANDBOX_VPL=y\nCONFIG_FIT_SIGNATURE=n\nCONFIG_VPL_FIT_SIGNATURE=n\nCONFIG_TOOLS_LIBCRYPTO=n")
>
> I know it's already busted on the line above, but we can format this in
> a more readable way by using something like:
>                               "\
> CONFIG1=y
> CONFIG2=n
> ...\n"

Will experiment with it, and if I can get it to work, also fix the
CONFIG_FIT_SIGNATURE stuff too.


>>                             ;; This test requires a sound system, which is un-used
>>                             ;; in u-boot-tools.
>>                             (("CONFIG_SOUND=y") "CONFIG_SOUND=n")))
>> @@ -1009,6 +1012,8 @@ (define*-public (make-u-boot-sunxi64-package board triplet
>>            #~(modify-phases #$phases
>>                (add-after 'unpack 'set-environment
>>                  (lambda* (#:key native-inputs inputs #:allow-other-keys)
>> +                  ;; Avoid dependency on crust-firmware https://issues.guix.gnu.org/48371
>
> Trick to avoid busting the 80 characters per line limit: for links, you
> can do (see: $link), which typically gets split like:
>
>                      ;; blablabla (see:
>                      ;; https://...)
>
>> +                  (setenv "SCP" "/dev/null")

Yeah, that sounds good...

Although, now that "Add crust firmware for sunxi devices"
(https://issues.guix.gnu.org/48371) finally got merged, we will have to
fix this properly. :)


>> @@ -1230,7 +1248,8 @@ (define-public u-boot-rockpro64-rk3399
>>                                                 "CONFIG_SATA_SIL=y"
>>                                                 "CONFIG_SCSI=y"
>>                                                 "CONFIG_SCSI_AHCI=y"
>> -                                               "CONFIG_DM_SCSI=y"))))
>> +                                               "CONFIG_DM_SCSI=y"
>> +                                               "# CONFIG_SPL_FIT_SIGNATURE is not set"))))
>>      (package
>>        (inherit base)
>>        (arguments
>> @@ -1240,6 +1259,13 @@ (define-public u-boot-rockpro64-rk3399
>>                (add-after 'unpack 'set-environment
>>                  (lambda* (#:key inputs #:allow-other-keys)
>>                    (setenv "BL31" (search-input-file inputs "/bl31.elf"))))
>> +              ;; Disable SPL FIT signatures, due to GPLv2 and Openssl license
>> +              ;; incompatibilities
>> +              (add-after 'unpack 'disable-spl-fit-signature
>> +                (lambda _
>> +                  (substitute* "configs/rockpro64-rk3399_defconfig"
>> +                    (("CONFIG_SPL_FIT_SIGNATURE=y")
>> +                     "# CONFIG_SPL_FIT_SIGNATURE is not set"))))
>
> Is this really needed, given we use "# CONFIG_SPL_FIT_SIGNATURE is not
> set" in #:configs above?

Only having it in #:configs resulted in a build failure (e.g. there were
conflicting entries or something). Having it in both places seems better
as it ensures it does not accidentally get enabled somehow. But we
probably could drop the part in #:configs if we wanted ... or re-write
how #:configs works, though that would be more than I want to get into
right now! :)

> The rest LGTM, thank you!

Thanks for the review!

Will try and incorporate the above suggestions and the patman patches
and get another patch series ready for 2023.07-rc6, and then hopefully
monday the actual release of 2023.07...


live well,
  vagrant