| Message ID | 20200318095757.9365-7-ynezz@true.cz |
|---|---|
| State | New |
| Headers | show |
| Series | produce working binaries by default | expand |
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
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
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
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
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 --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
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(-)