diff mbox series

[RFC,6/9] mkimage_fit_atf.sh: produce working binaries by default

Message ID 20200318095757.9365-7-ynezz@true.cz
State New
Headers show
Series produce working binaries by default | expand

Commit Message

Petr Štetiar March 18, 2020, 9:57 a.m. UTC
At this moment unusable binaries are produced if bl31.bin file is
missing in order to allow passing of various CI tests. This intention of
broken binaries has to be now explicitly confirmed via new
BUILDBOT_BROKEN_BINARIES config option, so usable binaries are produced
by default from now on.

Signed-off-by: Petr ?tetiar <ynezz at true.cz>
---
 arch/arm/mach-zynqmp/mkimage_fit_atf.sh | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Michal Simek March 20, 2020, 10:20 a.m. UTC | #1
Hi Petr,

On 18. 03. 20 10:57, Petr ?tetiar wrote:
> At this moment unusable binaries are produced if bl31.bin file is
> missing in order to allow passing of various CI tests. This intention of
> broken binaries has to be now explicitly confirmed via new
> BUILDBOT_BROKEN_BINARIES config option, so usable binaries are produced
> by default from now on.
> 
> Signed-off-by: Petr ?tetiar <ynezz at true.cz>
> ---
>  arch/arm/mach-zynqmp/mkimage_fit_atf.sh | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-zynqmp/mkimage_fit_atf.sh b/arch/arm/mach-zynqmp/mkimage_fit_atf.sh
> index 1e770ba111d3..5effe05abdee 100755
> --- a/arch/arm/mach-zynqmp/mkimage_fit_atf.sh
> +++ b/arch/arm/mach-zynqmp/mkimage_fit_atf.sh
> @@ -29,11 +29,15 @@ else
>  fi
>  
>  if [ ! -f $BL31 ]; then
> -	echo "WARNING: BL31 file $BL31 NOT found, resulting binary is non-functional" >&2
> -	BL31=/dev/null
> -	# But U-Boot proper could be loaded in EL3 by specifying
> -	# firmware = "uboot";
> -	# instead of "atf" in config node
> +	if [ "$BUILDBOT_BROKEN_BINARIES" = "y" ]; then
> +		BL31=/dev/null
> +		# But U-Boot proper could be loaded in EL3 by specifying
> +		# firmware = "uboot";
> +		# instead of "atf" in config node
> +	else
> +		echo "ERROR: BL31 file $BL31 NOT found, resulting binary is non-functional" >&2
> +		exit 1
> +	fi
>  fi
>  
>  cat << __HEADER_EOF
> 

I think instead of fixing it on several places we should merge things
together and fix this issue there.

Take a look at thread where we discussed it with Tom.
https://lists.denx.de/pipermail/u-boot/2019-December/393556.html

Thanks,
Michal
Petr Štetiar March 21, 2020, 3:19 p.m. UTC | #2
Michal Simek <michal.simek at xilinx.com> [2020-03-20 11:20:17]:

> I think instead of fixing it on several places we should merge things
> together and fix this issue there.

What do you mean exactly? Checking for the deps one layer up, like this for
example in sunxi?

  diff --git a/Makefile b/Makefile
  index 44776b8efcc4..1f4bff4374cf 100644
  --- a/Makefile
  +++ b/Makefile
  @@ -1276,6 +1276,19 @@ ifndef CONFIG_SYS_UBOOT_START
   CONFIG_SYS_UBOOT_START := $(CONFIG_SYS_TEXT_BASE)
   endif
   
  +define check_its_dep
  +	@if ! test -f "$(1)"; then \
  +		if test "$(CONFIG_BUILDBOT_BROKEN_BINARIES)" = "y"; then \
  +			touch "$(1)"; \
  +		else \
  +			echo "ERROR: $(2) file $(1) NOT found. If you want to build without " >&2; \
  +			echo "a $(2) file (creating a NON-FUNCTIONAL binary), then enable" >&2; \
  +			echo "config option CONFIG_BUILDBOT_BROKEN_BINARIES." >&2; \
  +			false; \
  +		fi \
  +	fi
  +endef
  +
   # Boards with more complex image requirements can provide an .its source file
   # or a generator script
   ifneq ($(CONFIG_SPL_FIT_SOURCE),"")
  @@ -1289,6 +1302,13 @@ endif
   ifeq ($(CONFIG_SPL_FIT_GENERATOR),"arch/arm/mach-rockchip/make_fit_atf.py")
   U_BOOT_ITS_DEPS += u-boot
   endif
  +ifeq ($(CONFIG_SPL_FIT_GENERATOR),"board/sunxi/mksunxi_fit_atf.sh")
  +BL31 := $(CURDIR)/bl31.bin
  +export BL31
  +$(BL31):
  +	$(call check_its_dep,$@,BL31)
  +U_BOOT_ITS_DEPS += $(BL31)
  +endif
   $(U_BOOT_ITS): $(U_BOOT_ITS_DEPS) FORCE
	  $(srctree)/$(CONFIG_SPL_FIT_GENERATOR) \
	  $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST))) > $@
  diff --git a/board/sunxi/mksunxi_fit_atf.sh b/board/sunxi/mksunxi_fit_atf.sh
  index 88ad71974706..1b0e5ee7a77a 100755
  --- a/board/sunxi/mksunxi_fit_atf.sh
  +++ b/board/sunxi/mksunxi_fit_atf.sh
  @@ -5,14 +5,6 @@
   #
   # usage: $0 <dt_name> [<dt_name> [<dt_name] ...]
   
  -[ -z "$BL31" ] && BL31="bl31.bin"
  -
  -if [ ! -f $BL31 ]; then
  -	echo "WARNING: BL31 file $BL31 NOT found, resulting binary is non-functional" >&2
  -	echo "Please read the section on ARM Trusted Firmware (ATF) in board/sunxi/README.sunxi64" >&2
  -	BL31=/dev/null
  -fi
  -
   if grep -q "^CONFIG_MACH_SUN50I_H6=y" .config; then
	  BL31_ADDR=0x104000
   else

> Take a look at thread where we discussed it with Tom.
> https://lists.denx.de/pipermail/u-boot/2019-December/393556.html

Ok, but that N'th variation was merged anyway. So whats the plan? It's not
clear from that discussion or I don't get it.

-- ynezz
Michal Simek March 23, 2020, 1:58 p.m. UTC | #3
On 21. 03. 20 16:19, Petr ?tetiar wrote:
> Michal Simek <michal.simek at xilinx.com> [2020-03-20 11:20:17]:
> 
>> I think instead of fixing it on several places we should merge things
>> together and fix this issue there.
> 
> What do you mean exactly? Checking for the deps one layer up, like this for
> example in sunxi?
> 
>   diff --git a/Makefile b/Makefile
>   index 44776b8efcc4..1f4bff4374cf 100644
>   --- a/Makefile
>   +++ b/Makefile
>   @@ -1276,6 +1276,19 @@ ifndef CONFIG_SYS_UBOOT_START
>    CONFIG_SYS_UBOOT_START := $(CONFIG_SYS_TEXT_BASE)
>    endif
>    
>   +define check_its_dep
>   +	@if ! test -f "$(1)"; then \
>   +		if test "$(CONFIG_BUILDBOT_BROKEN_BINARIES)" = "y"; then \
>   +			touch "$(1)"; \
>   +		else \
>   +			echo "ERROR: $(2) file $(1) NOT found. If you want to build without " >&2; \
>   +			echo "a $(2) file (creating a NON-FUNCTIONAL binary), then enable" >&2; \
>   +			echo "config option CONFIG_BUILDBOT_BROKEN_BINARIES." >&2; \
>   +			false; \
>   +		fi \
>   +	fi
>   +endef
>   +
>    # Boards with more complex image requirements can provide an .its source file
>    # or a generator script
>    ifneq ($(CONFIG_SPL_FIT_SOURCE),"")
>   @@ -1289,6 +1302,13 @@ endif
>    ifeq ($(CONFIG_SPL_FIT_GENERATOR),"arch/arm/mach-rockchip/make_fit_atf.py")
>    U_BOOT_ITS_DEPS += u-boot
>    endif
>   +ifeq ($(CONFIG_SPL_FIT_GENERATOR),"board/sunxi/mksunxi_fit_atf.sh")
>   +BL31 := $(CURDIR)/bl31.bin

here should be at least ?=

>   +export BL31
>   +$(BL31):
>   +	$(call check_its_dep,$@,BL31)
>   +U_BOOT_ITS_DEPS += $(BL31)
>   +endif
>    $(U_BOOT_ITS): $(U_BOOT_ITS_DEPS) FORCE
> 	  $(srctree)/$(CONFIG_SPL_FIT_GENERATOR) \
> 	  $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST))) > $@
>   diff --git a/board/sunxi/mksunxi_fit_atf.sh b/board/sunxi/mksunxi_fit_atf.sh
>   index 88ad71974706..1b0e5ee7a77a 100755
>   --- a/board/sunxi/mksunxi_fit_atf.sh
>   +++ b/board/sunxi/mksunxi_fit_atf.sh
>   @@ -5,14 +5,6 @@
>    #
>    # usage: $0 <dt_name> [<dt_name> [<dt_name] ...]
>    
>   -[ -z "$BL31" ] && BL31="bl31.bin"
>   -
>   -if [ ! -f $BL31 ]; then
>   -	echo "WARNING: BL31 file $BL31 NOT found, resulting binary is non-functional" >&2
>   -	echo "Please read the section on ARM Trusted Firmware (ATF) in board/sunxi/README.sunxi64" >&2
>   -	BL31=/dev/null
>   -fi
>   -
>    if grep -q "^CONFIG_MACH_SUN50I_H6=y" .config; then
> 	  BL31_ADDR=0x104000
>    else
> 
>> Take a look at thread where we discussed it with Tom.
>> https://lists.denx.de/pipermail/u-boot/2019-December/393556.html
> 
> Ok, but that N'th variation was merged anyway. So whats the plan? It's not
> clear from that discussion or I don't get it.

My script was merged mostly because of fedora support and I expect it
was the last one. Plan is to merge all of these to one shell script
because most of the content in that script is very similar.

The issue which I see is that patch above is changing current behavior
which means when this is applied all travis/gitlab/azure jobs will fail
because none has setup this config option.

And truth is that even regular user doesn't know if error out is not
check that final binary won't work because nothing fails.

Anyway I have sent series which enables to run U-Boot to run EL3 that's
why at least zynqmp should be ok to build and run u-boot.itb. But better
to run it with ATF.

Thanks,
Michal
Petr Štetiar March 23, 2020, 3:24 p.m. UTC | #4
Michal Simek <michal.simek at xilinx.com> [2020-03-23 14:58:55]:

Hi,

> Plan is to merge all of these to one shell script because most of the
> content in that script is very similar.

to me it all just seems as values and templates hidden inside shell script
called generator.  Each of this generators uses/references 1-N variables, 1-N
out-of-tree dependencies and 1-N DT sections.

So instead of cooking custom shell templating engine I took different
approach[1] and simply removed the shell script generator in favor of Python
based templating engine, allowing for ITS dependency tracking and values
directly in Make.

Lets see what is preferred.

> The issue which I see is that patch above is changing current behavior
> which means when this is applied all travis/gitlab/azure jobs will fail
> because none has setup this config option.

If that is desired, then in order to keep the current behavior, I see
following options:

 1. Disable building of boards having out-of-tree dependencies on build bot/CI
 2. Enable BUILDBOT_BROKEN_BINARIES=y by default
 3. Add detection of various build bot/CI platforms and enable
    BUILDBOT_BROKEN_BINARIES=y by default, which then could cause further issues
    when one would actually run test the binaries from build bot/CI
 4. Include the out-of-tree dependencies (unlikely)

What is your suggestion?

1. https://patchwork.ozlabs.org/project/uboot/list/?series=166092


Cheers,

Petr
Tom Rini March 31, 2020, 2:58 p.m. UTC | #5
On Mon, Mar 23, 2020 at 04:24:15PM +0100, Petr ?tetiar wrote:
> Michal Simek <michal.simek at xilinx.com> [2020-03-23 14:58:55]:
> 
> Hi,
> 
> > Plan is to merge all of these to one shell script because most of the
> > content in that script is very similar.
> 
> to me it all just seems as values and templates hidden inside shell script
> called generator.  Each of this generators uses/references 1-N variables, 1-N
> out-of-tree dependencies and 1-N DT sections.
> 
> So instead of cooking custom shell templating engine I took different
> approach[1] and simply removed the shell script generator in favor of Python
> based templating engine, allowing for ITS dependency tracking and values
> directly in Make.
> 
> Lets see what is preferred.

An example of one is never enough to see if something is generic enough
for everyone.  Even if you can't functional-test other platforms an
initial conversion will help and allow others to test.  And it'll help
show if the idea is or is not cleaner.

> > The issue which I see is that patch above is changing current behavior
> > which means when this is applied all travis/gitlab/azure jobs will fail
> > because none has setup this config option.
> 
> If that is desired, then in order to keep the current behavior, I see
> following options:
> 
>  1. Disable building of boards having out-of-tree dependencies on build bot/CI
>  2. Enable BUILDBOT_BROKEN_BINARIES=y by default
>  3. Add detection of various build bot/CI platforms and enable
>     BUILDBOT_BROKEN_BINARIES=y by default, which then could cause further issues
>     when one would actually run test the binaries from build bot/CI
>  4. Include the out-of-tree dependencies (unlikely)

Cycling back here, sorry for the delay.  First, you need to make CI
still work.  And we can't remove boards that require something else from
CI as that means basically dropping all of arm64.
 
> What is your suggestion?

There's two sides to this.  First, the problem of "all of these similar
but not quite the same tools" needs a generic solution.  A template
based tool is probably the right direction here, yes.

But second, what would it take to modify the CI jobs to pass in an empty
file or whatever so they link but not function.  So that a real user
would have to provide a file too.  And whichever of Azure, GitLab or
Travis is easier for you to test / adapt for this, I can update the
others once it looks clean.  Thanks!
diff mbox series

Patch

diff --git a/arch/arm/mach-zynqmp/mkimage_fit_atf.sh b/arch/arm/mach-zynqmp/mkimage_fit_atf.sh
index 1e770ba111d3..5effe05abdee 100755
--- a/arch/arm/mach-zynqmp/mkimage_fit_atf.sh
+++ b/arch/arm/mach-zynqmp/mkimage_fit_atf.sh
@@ -29,11 +29,15 @@  else
 fi
 
 if [ ! -f $BL31 ]; then
-	echo "WARNING: BL31 file $BL31 NOT found, resulting binary is non-functional" >&2
-	BL31=/dev/null
-	# But U-Boot proper could be loaded in EL3 by specifying
-	# firmware = "uboot";
-	# instead of "atf" in config node
+	if [ "$BUILDBOT_BROKEN_BINARIES" = "y" ]; then
+		BL31=/dev/null
+		# But U-Boot proper could be loaded in EL3 by specifying
+		# firmware = "uboot";
+		# instead of "atf" in config node
+	else
+		echo "ERROR: BL31 file $BL31 NOT found, resulting binary is non-functional" >&2
+		exit 1
+	fi
 fi
 
 cat << __HEADER_EOF