Re: [PATCH 9/9] Makefile: Show binman missing blob message

[Tom Rini <trini@konsulko.com>]

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

On Wed, Feb 22, 2023 at 02:20:08PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 21 Feb 2023 at 16:09, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Feb 21, 2023 at 12:41:52PM -0700, Simon Glass wrote:
> > > Hi Jonas
> > >
> > > +Tom Rini
> > >
> > > On Sun, 19 Feb 2023 at 15:02, Jonas Karlman <jonas@kwiboo.se> wrote:
> > > >
> > > > When binman is invoked during a build of U-Boot and an external blob is
> > > > missing, the user is usually presented with a generic file not found in
> > > > input path message.
> > > >
> > > > Invoke binman with --allow-missing so that binman can show relevant
> > > > missing blob help messages. Build continue to fail with missing blobs
> > > > unless BINMAN_ALLOW_MISSING=1 is used.
> > > >
> > > > This changes the following error message:
> > > >
> > > >   binman: Filename 'atf-bl31' not found in input path (...)
> > > >
> > > > to the following:
> > > >
> > > >   Image 'itb' is missing external blobs and is non-functional: atf-blob
> > > >
> > > >   /binman/itb/fit/images/atf/atf-blob (bl31.bin):
> > > >      See the documentation for your board. You may need to build ARM Trusted
> > > >      Firmware and build with BL31=/path/to/bl31.bin
> > > >
> > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > > ---
> > > >  Makefile | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/Makefile b/Makefile
> > > > index 58f8c7a35335..c2860824f6f2 100644
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -1326,7 +1326,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 --ignore-missing) \
> > > > +               --allow-missing $(if $(BINMAN_ALLOW_MISSING),--ignore-missing) \
> > > >                 -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
> > > >                 -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
> > > >                 $(foreach f,$(BINMAN_INDIRS),-I $(f)) \
> > > > --
> > > > 2.39.2
> > > >
> > >
> > > I agree this is better, but we should see what Tom thinks.
> > >
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > This sounds like a binman bug. We shouldn't need to say --allow-missing
> > to then make use of the missing-msg node.
> 
> Without --allow-missing binman just dies when it cannot find
> something. This is useful because it allows debugging of problems.
> 
> I'm not sure how easy it would be to change this. The error is
> reported by the tools module which has no knowledge of binman. We
> could catch the exception, but again that makes things non-debuggable.

Why wouldn't it be debuggable? Wouldn't it be more debuggable now that
we're saying "You're missing foo.blob, this is typically provided by the
foo project", which we can just say because the message has been filled
out already in the etype.

> At the moment we have two cases:
> - normal: missing files cause an immediate abort
> - --allow-missing: missing files are collected with a report at the end
> 
> Worse, we can have missing tools, not just missing blobs. The
> complexity of this gets completely out of hand if we try to meld the
> normal and --allow-missing cases together.
> 
> I actually don't see much of a downside to passing allow-missing
> always. But perhaps the BINMAN_ALLOW_MISSING should be renamed to
> BINMAN_IGNORE_MISSING (with a similar change in buildman). We could
> actually keep BINMAN_ALLOW_MISSING to mean to pass --allow-missing,
> pointing out in the docs that this is a bad idea as you won't see any
> blob help?

Maybe the problem is the flag is badly named? I still think this speaks
to bugs in the tooling if we can't make use of the helpful error message
that we have available without some special flags to be set.

-- 
Tom

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