Subject: Re: bug#60224: [PATCH 0/9] Improvements to our u-boot tooling

From: Maxim Cournoyer <[email-masked]>
Date: Sun, 01 Jan 2023 19:27:52 -0500

Hi Ricardo!

Ricardo Wurmus <[email-masked]> writes:

> Hi Maxim,
>
> this looks reasonable to me.  Some comments below.

Sorry for the late reply, it hadn't reached my INBOX (please keep me in
CC to ensure it does :-)).

> A minor comment about the first patch: you still bind “outputs” in the
> build phases, but since you’re using #$output anyway this value is never
> used.

Fixed!

> [PATCH 3/9] introduces a comment in the definition of “native-build?”,
> which references %current-target-system, yet only %current-system is
> used.  Is this a mistake?

Fixed!

> [PATCH 4/9] — This one appends arm-trusted-firmware-rk3399 instead of
> prepending it.  This differs from how it was done with the labeled
> inputs.  Does this have any consequences?  Is the “firmware” label used
> anywhere (such as downstream packages)?  The same applies to patches
> 5/9, 7/9, and 8/9.

I don't think it matters; the base u-boot package which gets used
doesn't include any "firmware" input, and the file provided via
arm-trusted-firmware-rk3399 is searched via "search-input-file".  I've
grepped for 'assoc-ref.*"firmware"' and there doesn't seem to be any
remnants except for u-boot-rockpro64-rk3399, which I've now fixed in the
last commit.

> [PATCH 6/9] — The change from .bin to .elf confuses me.  Is this due to the
> fact that “target” is now actually set and the package build thus
> behaves differently?

I think so.  I was puzzled by it too, especially since some packages
already were searching for a .elf file rather than a .bin file.

> [PATCH 8/9] removes a reference to “firware”; this answers my question
> to patch 4/9, but perhaps other such references remain?

Answered above.

Thanks for the review!  v3 will appear shortly.

-- 
Thanks,
Maxim