[PATCH] Makefile: add -W for BINMAN_ALLOW_MISSING

[PATCH] Makefile: add -W for BINMAN_ALLOW_MISSING

[Nikita Shubin]

From: Nikita Shubin <n.shubin@yadro.com>

Otherwise make will produce an error even with --allow-missing and
--fake-ext-blobs set.

Fixes: b38da15a054 ("binman: Use an exit code when blobs are missing")
Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index de5746399a6..3fec08e7081 100644
--- a/Makefile
+++ b/Makefile
@@ -1334,7 +1334,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
                 --toolpath $(objtree)/tools \
 		$(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
 		build -u -d u-boot.dtb -O . -m \
-		$(if $(BINMAN_ALLOW_MISSING),--allow-missing --fake-ext-blobs) \
+		$(if $(BINMAN_ALLOW_MISSING),--allow-missing --fake-ext-blobs -W) \
 		-I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
 		-I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
 		$(foreach f,$(BINMAN_INDIRS),-I $(f)) \
-- 
2.37.4

Re: [PATCH] Makefile: add -W for BINMAN_ALLOW_MISSING

[Simon Glass]

Hi,

+Tom Rini

On Fri, 16 Dec 2022 at 02:27, Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> From: Nikita Shubin <n.shubin@yadro.com>
>
> Otherwise make will produce an error even with --allow-missing and
> --fake-ext-blobs set.
>
> Fixes: b38da15a054 ("binman: Use an exit code when blobs are missing")
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index de5746399a6..3fec08e7081 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1334,7 +1334,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
>                  --toolpath $(objtree)/tools \
>                 $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
>                 build -u -d u-boot.dtb -O . -m \
> -               $(if $(BINMAN_ALLOW_MISSING),--allow-missing --fake-ext-blobs) \
> +               $(if $(BINMAN_ALLOW_MISSING),--allow-missing --fake-ext-blobs -W) \
>                 -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
>                 -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
>                 $(foreach f,$(BINMAN_INDIRS),-I $(f)) \
> --
> 2.37.4
>

+Tom Rini

We do actually want to report the failure, since it means that the
image will not function. This was a recent change requested by a few
people.

Note that buildman looks for the message 'Some images are invalid' and
either returning 103, or 0 if -W is given.

There is no attempt to produce a special exit code from the Makefile.
It generally returns 2 (as per 'man make'), which is why buildman has
this extra processing.

Regards,
Simon

Re: [PATCH] Makefile: add -W for BINMAN_ALLOW_MISSING

[Nikita Shubin]

Hello Tom and Simon!

On Sat, 17 Dec 2022 14:38:30 -0700
Simon Glass <sjg@chromium.org> wrote:

> +Tom Rini
> 
> We do actually want to report the failure, since it means that the
> image will not function. This was a recent change requested by a few
> people.

It doesn't make sense to me - if i am passing "--allow-missing" than
binman shouldn't fail drastically, cause i literally told him "It's okay
if files are missing". What purpose does it have now, it we are failing
regardless we are providing this flag or not ?

This breaks old behaviour by the way, when passing "--allow-missing"
for missing blobs produced a warning instead of error.

> 
> Note that buildman looks for the message 'Some images are invalid' and
> either returning 103, or 0 if -W is given.
> 
> There is no attempt to produce a special exit code from the Makefile.
> It generally returns 2 (as per 'man make'), which is why buildman has
> this extra processing.

Well, there are only 3 codes for make and 2 indicates any failure:

"A status of two will be returned if any errors were encountered." (c)

This new behaviour looks the same with or without BINMAN_ALLOW_MISSING
flag from top point of view:

With BINMAN_ALLOW_MISSING=1:
Some images are invalid
make[1]: *** [Makefile:1114: .binman_stamp] Error 103
make[1]: Leaving directory '/home/maquefel/workshop/overlord/u-boot'
make: *** [Makefile:271: u-boot/u

$ echo $?
2-boot-nodtb.bin] Error 2

Without BINMAN_ALLOW_MISSING:

binman: Filename 'fw_dynamic.bin' not found in input path
(.,.,./board/syntacore/scr7_elct,arch/riscv/dts)
(cwd='/home/maquefel/workshop/overlord/u-boot') make[1]: ***
[Makefile:1114: .binman_stamp] Error 1 make[1]: Leaving directory
'/home/maquefel/workshop/overlord/u-boot' make: *** [Makefile:271:
u-boot/u-boot-nodtb.bin] Error 2

$ echo $?
2

So that's the difference if build is failing either way ?

Yours,
Nikita Shubin.

> 
> Regards,
> Simon

Re: [PATCH] Makefile: add -W for BINMAN_ALLOW_MISSING

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2568 bytes --]

On Mon, Dec 19, 2022 at 11:21:45AM +0300, Nikita Shubin wrote:
> Hello Tom and Simon!
> 
> On Sat, 17 Dec 2022 14:38:30 -0700
> Simon Glass <sjg@chromium.org> wrote:
> 
> > +Tom Rini
> > 
> > We do actually want to report the failure, since it means that the
> > image will not function. This was a recent change requested by a few
> > people.
> 
> It doesn't make sense to me - if i am passing "--allow-missing" than
> binman shouldn't fail drastically, cause i literally told him "It's okay
> if files are missing". What purpose does it have now, it we are failing
> regardless we are providing this flag or not ?
> 
> This breaks old behaviour by the way, when passing "--allow-missing"
> for missing blobs produced a warning instead of error.
> 
> > 
> > Note that buildman looks for the message 'Some images are invalid' and
> > either returning 103, or 0 if -W is given.
> > 
> > There is no attempt to produce a special exit code from the Makefile.
> > It generally returns 2 (as per 'man make'), which is why buildman has
> > this extra processing.
> 
> Well, there are only 3 codes for make and 2 indicates any failure:
> 
> "A status of two will be returned if any errors were encountered." (c)
> 
> This new behaviour looks the same with or without BINMAN_ALLOW_MISSING
> flag from top point of view:
> 
> With BINMAN_ALLOW_MISSING=1:
> Some images are invalid
> make[1]: *** [Makefile:1114: .binman_stamp] Error 103
> make[1]: Leaving directory '/home/maquefel/workshop/overlord/u-boot'
> make: *** [Makefile:271: u-boot/u
> 
> $ echo $?
> 2-boot-nodtb.bin] Error 2
> 
> Without BINMAN_ALLOW_MISSING:
> 
> binman: Filename 'fw_dynamic.bin' not found in input path
> (.,.,./board/syntacore/scr7_elct,arch/riscv/dts)
> (cwd='/home/maquefel/workshop/overlord/u-boot') make[1]: ***
> [Makefile:1114: .binman_stamp] Error 1 make[1]: Leaving directory
> '/home/maquefel/workshop/overlord/u-boot' make: *** [Makefile:271:
> u-boot/u-boot-nodtb.bin] Error 2
> 
> $ echo $?
> 2
> 
> So that's the difference if build is failing either way ?

So, with what is in master right now, BINMAN_ALLOW_MISSING=1 should work
as intended, while it did not at its introduction.  Please confirm if
your use cases work now, or not. The problem we needed to solve was the
one that by default previously, you could run "make fooboard_config
all", not have BL31/etc available, get a warning printed and a zero
exit code, leading to non-obvious failures if you build indirectly
(buildroot, yocto/OE, etc).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Makefile: add -W for BINMAN_ALLOW_MISSING

[Nikita Shubin]

Hello Tom!

On Mon, 19 Dec 2022 08:36:30 -0500
Tom Rini <trini@konsulko.com> wrote:

> On Mon, Dec 19, 2022 at 11:21:45AM +0300, Nikita Shubin wrote:
> > Hello Tom and Simon!
> > 
> > On Sat, 17 Dec 2022 14:38:30 -0700
> > Simon Glass <sjg@chromium.org> wrote:
> >   
> > > +Tom Rini
> > > 
> > > We do actually want to report the failure, since it means that the
> > > image will not function. This was a recent change requested by a
> > > few people.  
> > 
> > It doesn't make sense to me - if i am passing "--allow-missing" than
> > binman shouldn't fail drastically, cause i literally told him "It's
> > okay if files are missing". What purpose does it have now, it we
> > are failing regardless we are providing this flag or not ?
> > 
> > This breaks old behaviour by the way, when passing "--allow-missing"
> > for missing blobs produced a warning instead of error.
> >   
> > > 
> > > Note that buildman looks for the message 'Some images are
> > > invalid' and either returning 103, or 0 if -W is given.
> > > 
> > > There is no attempt to produce a special exit code from the
> > > Makefile. It generally returns 2 (as per 'man make'), which is
> > > why buildman has this extra processing.  
> > 
> > Well, there are only 3 codes for make and 2 indicates any failure:
> > 
> > "A status of two will be returned if any errors were encountered."
> > (c)
> > 
> > This new behaviour looks the same with or without
> > BINMAN_ALLOW_MISSING flag from top point of view:
> > 
> > With BINMAN_ALLOW_MISSING=1:
> > Some images are invalid
> > make[1]: *** [Makefile:1114: .binman_stamp] Error 103
> > make[1]: Leaving directory '/home/maquefel/workshop/overlord/u-boot'
> > make: *** [Makefile:271: u-boot/u
> > 
> > $ echo $?
> > 2-boot-nodtb.bin] Error 2
> > 
> > Without BINMAN_ALLOW_MISSING:
> > 
> > binman: Filename 'fw_dynamic.bin' not found in input path
> > (.,.,./board/syntacore/scr7_elct,arch/riscv/dts)
> > (cwd='/home/maquefel/workshop/overlord/u-boot') make[1]: ***
> > [Makefile:1114: .binman_stamp] Error 1 make[1]: Leaving directory
> > '/home/maquefel/workshop/overlord/u-boot' make: *** [Makefile:271:
> > u-boot/u-boot-nodtb.bin] Error 2
> > 
> > $ echo $?
> > 2
> > 
> > So that's the difference if build is failing either way ?  
> 
> So, with what is in master right now, BINMAN_ALLOW_MISSING=1 should
> work as intended, while it did not at its introduction.  Please
> confirm if your use cases work now, or not. 

They don't actually, a few iterations ago i didn't even needed
BINMAN_ALLOW_MISSING (now it's clear for me that i never needed it), as
make produced only a warning and not a error.

I have kernel and ramdisk sections in my binman file and FIT image is
fully functional even if they are missing.

And now there is no way to tell u-boot not to fail if some blobs are
missing.

> The problem we needed to
> solve was the one that by default previously, you could run "make
> fooboard_config all", not have BL31/etc available, get a warning
> printed and a zero exit code, leading to non-obvious failures if you
> build indirectly (buildroot, yocto/OE, etc).
> 

May be they shouldn't use BINMAN_ALLOW_MISSING when building u-boot at
all then ?

Or can we, at least, have some BINMAN_REALLY_ALLOW_MISSING option or
simply passing some flags with BINMAN_OPTS for example ?

Through:
  -W, --ignore-missing  Return success even if there are missing
blobs/bintools (requires -M)

seems to have really good synergy with:

 -M, --allow-missing   Allow external blobs and bintools to be
 missing

For BINMAN_ALLOW_MISSING.

Yours,
Nikita Shubin.

Re: [PATCH] Makefile: add -W for BINMAN_ALLOW_MISSING

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4933 bytes --]

On Thu, Dec 22, 2022 at 10:01:16AM +0300, Nikita Shubin wrote:
> Hello Tom!
> 
> On Mon, 19 Dec 2022 08:36:30 -0500
> Tom Rini <trini@konsulko.com> wrote:
> 
> > On Mon, Dec 19, 2022 at 11:21:45AM +0300, Nikita Shubin wrote:
> > > Hello Tom and Simon!
> > > 
> > > On Sat, 17 Dec 2022 14:38:30 -0700
> > > Simon Glass <sjg@chromium.org> wrote:
> > >   
> > > > +Tom Rini
> > > > 
> > > > We do actually want to report the failure, since it means that the
> > > > image will not function. This was a recent change requested by a
> > > > few people.  
> > > 
> > > It doesn't make sense to me - if i am passing "--allow-missing" than
> > > binman shouldn't fail drastically, cause i literally told him "It's
> > > okay if files are missing". What purpose does it have now, it we
> > > are failing regardless we are providing this flag or not ?
> > > 
> > > This breaks old behaviour by the way, when passing "--allow-missing"
> > > for missing blobs produced a warning instead of error.
> > >   
> > > > 
> > > > Note that buildman looks for the message 'Some images are
> > > > invalid' and either returning 103, or 0 if -W is given.
> > > > 
> > > > There is no attempt to produce a special exit code from the
> > > > Makefile. It generally returns 2 (as per 'man make'), which is
> > > > why buildman has this extra processing.  
> > > 
> > > Well, there are only 3 codes for make and 2 indicates any failure:
> > > 
> > > "A status of two will be returned if any errors were encountered."
> > > (c)
> > > 
> > > This new behaviour looks the same with or without
> > > BINMAN_ALLOW_MISSING flag from top point of view:
> > > 
> > > With BINMAN_ALLOW_MISSING=1:
> > > Some images are invalid
> > > make[1]: *** [Makefile:1114: .binman_stamp] Error 103
> > > make[1]: Leaving directory '/home/maquefel/workshop/overlord/u-boot'
> > > make: *** [Makefile:271: u-boot/u
> > > 
> > > $ echo $?
> > > 2-boot-nodtb.bin] Error 2
> > > 
> > > Without BINMAN_ALLOW_MISSING:
> > > 
> > > binman: Filename 'fw_dynamic.bin' not found in input path
> > > (.,.,./board/syntacore/scr7_elct,arch/riscv/dts)
> > > (cwd='/home/maquefel/workshop/overlord/u-boot') make[1]: ***
> > > [Makefile:1114: .binman_stamp] Error 1 make[1]: Leaving directory
> > > '/home/maquefel/workshop/overlord/u-boot' make: *** [Makefile:271:
> > > u-boot/u-boot-nodtb.bin] Error 2
> > > 
> > > $ echo $?
> > > 2
> > > 
> > > So that's the difference if build is failing either way ?  
> > 
> > So, with what is in master right now, BINMAN_ALLOW_MISSING=1 should
> > work as intended, while it did not at its introduction.  Please
> > confirm if your use cases work now, or not. 
> 
> They don't actually, a few iterations ago i didn't even needed
> BINMAN_ALLOW_MISSING (now it's clear for me that i never needed it), as
> make produced only a warning and not a error.

Yes, it would only ever produce a warning before.

> I have kernel and ramdisk sections in my binman file and FIT image is
> fully functional even if they are missing.
> 
> And now there is no way to tell u-boot not to fail if some blobs are
> missing.

What do you mean? That's what BINMAN_ALLOW_MISSING=1 does:
$ make CROSS_COMPILE=~/.buildman-toolchains/gcc-12.2.0-nolibc/aarch64-linux/bin/aarch64-linux- BINMAN_ALLOW_MISSING=1 -sj pine64_plus_defconfig all -sj;echo $?
...
Image 'main-section' is missing external blobs and is non-functional: atf-bl31 scp

/binman/u-boot-sunxi-with-spl/fit/images/atf/atf-bl31:
   Please read the section on ARM Trusted Firmware (ATF) in
   board/sunxi/README.sunxi64

/binman/u-boot-sunxi-with-spl/fit/images/scp/scp:
   SCP firmware is required for system suspend, but is otherwise optional.
   Please read the section on SCP firmware in board/sunxi/README.sunxi64

Some images are invalid
0
$

Is what I get on top of master right now.

> > The problem we needed to
> > solve was the one that by default previously, you could run "make
> > fooboard_config all", not have BL31/etc available, get a warning
> > printed and a zero exit code, leading to non-obvious failures if you
> > build indirectly (buildroot, yocto/OE, etc).
> > 
> 
> May be they shouldn't use BINMAN_ALLOW_MISSING when building u-boot at
> all then ?
> 
> Or can we, at least, have some BINMAN_REALLY_ALLOW_MISSING option or
> simply passing some flags with BINMAN_OPTS for example ?
> 
> Through:
>   -W, --ignore-missing  Return success even if there are missing
> blobs/bintools (requires -M)
> 
> seems to have really good synergy with:
> 
>  -M, --allow-missing   Allow external blobs and bintools to be
>  missing
> 
> For BINMAN_ALLOW_MISSING.

Yes, that's what BINMAN_ALLOW_MISSING=1 does. If top of tree master
doesn't work for you, can you please post a patch so that the rest of us
can replicate the failure and see what to do next? Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Makefile: add -W for BINMAN_ALLOW_MISSING

[Nikita Shubin]

Hello Tom!

On Thu, 22 Dec 2022 09:14:59 -0500
Tom Rini <trini@konsulko.com> wrote:

> Yes, that's what BINMAN_ALLOW_MISSING=1 does. If top of tree master
> doesn't work for you, can you please post a patch so that the rest of
> us can replicate the failure and see what to do next? Thanks.
> 

You are right - i accidentally got stuck right between "global: Do not
default to faking missing binaries for buildman" (which broke build)
and "Makefile: With
BINMAN_ALLOW_MISSING=1 don't error on missing" (which i didn't have on
my branch).

Everything is just perfect on the top of latest master!

Thank you for your patience and sorry for the noise.

Yours,
Nikita Shubin