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

[Simon Glass]

Hi Tom,

On Wed, 22 Feb 2023 at 14:45, Tom Rini <trini at konsulko.com> wrote:
>
> 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 at 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 at 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 at 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 at 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.

Perhaps just take my word for it? The feature you are asking for here
is exactly what the -M flag does, but you are trying to make it so
that a 'file not found' error is not reported immediately, but appears
later. That makes it hard to debug, since the 'exception' is supressed
and only later do you see there is a problem...so hard to debug.

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

Without -M binman is just opening the files you tell it, failing if it
cannot, exactly as things used to be before binman.

Yes --allow-missing might not be the best name, since we don't really
allow it, just collect the information and report it. Binman still
fails the build in that case. Perhaps we need:

(nothing) - die as soon as any missing tool or file is
--collect-missing   - collect all the missing blobs and tools, rather
than dying right away, returning 103 if there are any problems
--allow-missing   - report success (with warnings output) even if
there are missing blobs / tools

Regards,
Simon