mbox series

[bug#60224,0/9] Improvements to our u-boot tooling

Message ID 20221220165038.25114-1-maxim.cournoyer@gmail.com
Headers show
Series Improvements to our u-boot tooling | expand

Message

Maxim Cournoyer Dec. 20, 2022, 4:50 p.m. UTC 
This series include a few changes that were useful or needed to build U-Boot
for the i.MX6, notably installation of the u-boot.imx image.  It also cleans up
things for cross-compilation, no longer explicitly adding cross-gcc and
cross-binutils, leaving the build system taking care of that.

The two first commits of this series were previously submitted as #59761, now
extracted and submitted here for transparency, with fixes for impacted u-boot
packages that broke because the move to use gexps.


Maxim Cournoyer (9):
  gnu: make-u-boot-package: Add a u-boot argument and use gexps.
  gnu: make-u-boot-package: Install .imx files.
  gnu: make-uboot-package: Simplify build.
  gnu: u-boot-pinebook-pro-rk3399: Remove input labels and use gexps.
  gnu: u-boot-firefly-rk3399: Use gexps and fix cross-build.
  gnu: make-u-boot-sunxi64-package: Use gexps and adjust file name.
  gnu: u-boot-rock64-rk3328: Fix build.
  gnu: u-boot-sifive-unmatched: Use gexps and remove inputs.
  gnu: u-boot-puma-rk3399: Use make-u-boot-sunxi64-package.

 gnu/packages/bootloaders.scm | 295 +++++++++++++++++------------------
 1 file changed, 144 insertions(+), 151 deletions(-)


base-commit: 1a3d8b922863c22f612ea679d9419bb457874fdf

Comments

Ricardo Wurmus Dec. 29, 2022, 7:18 p.m. UTC | #1 
Hi Maxim,

this looks reasonable to me.  Some comments below.

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.

[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?

[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.

[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?

[PATCH 8/9] removes a reference to “firware”; this answers my question
to patch 4/9, but perhaps other such references remain?
Maxim Cournoyer Jan. 2, 2023, 12:27 a.m. UTC | #2 
Hi Ricardo!

Ricardo Wurmus <rekado@elephly.net> 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.