[PATCH] Makefile: With BINMAN_ALLOW_MISSING=1 don't error on missing

[Tom Rini]

On Tue, Dec 06, 2022 at 03:36:49PM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 6 Dec 2022 at 15:03, Tom Rini <trini at konsulko.com> wrote:
> >
> > When the user builds with BINMAN_ALLOW_MISSING=1 they're explicitly
> > setting the flag to allow for additional binaries to be missing and so
> > have acknowledged the output might not work. In this case we want to
> > default to not passing a non-zero exit code.
> >
> > Cc: Simon Glass <sjg at chromium.org>
> > Reported-by: Peter Robinson <pbrobinson at gmail.com>
> > Signed-off-by: Tom Rini <trini at konsulko.com>
> > ---
> > This passes CI as-is:
> > https://source.denx.de/u-boot/u-boot/-/pipelines/14340
> > ---
> >  Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index de5746399a63..03de1da1bfd0 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 --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.25.1
> >
> 
> I believe we need the --fake-ext-blobs flag as well, since otherwise
> boards which use tools (like mkimage) on things that don't exist will
> not work.

So, we need to list out all the use cases perhaps, as we had missed one
before. In my mind we have:
- Board requires 1 or more blobs, developer will be running this on
  hardware, expects output from U-Boot 'make' (or buildman) to boot.
  Blobs must be present or non-zero exist status by default.
  We didn't have this before, and we do now.
- Board requires 1 or more blobs AND has optional blobs, will be running
  this on hardware, expects output from U-Boot 'make' (or buildman) to
  boot. This is the case we had missed before as allwinner requires bl31
  and has optional PMIC firmware. This is the case Peter reported and we
  had missed before, which this patch allows for, with the caveat that
  if you forget BL31 you're not going to boot and make won't complain
  exit-code wise. Another way to resolve this would be a property in the
  binman node to mark a blob as optional and warn if missing, rather
  than error if missing.
- Board requires 1 or more blobs, output will NOT be run. This is the CI
  case and the compile testing lots of board cases. This is what CI
  passes still, with the above.
- Board requires 1 or more blobs, developer will be running this on
  hardware, BUT will be injecting the blobs later on. I think this is
  the use case you're talking about?

> Yes I know this passes on CI, but it will cause breakages when people
> use mkimage or other things which have missing inputs. It will be
> quite confusing too!
> 
> As to the logic, I thought you had wanted the build to fail if there
> are missing blobs, regardless of whether they were allowed or not.
> There is currently code in buildman to detect this failure and either
> report it or ignore it:
> 
>                     if (result.return_code == 2 and
>                         ('Some images are invalid' in result.stderr)):
>                         # This is handled later by the check for output in
>                         # stderr
>                         result.return_code = 0
> 
> Since buildman sets BINMAN_ALLOW_MISSING=1 if -M is given, we will not
> be able to detect missing binaries at all when built from buildman. I
> think a suitable fix would be to update the code above to detect the
> 'Some images are invalid' regardless of the return code.

Well, what we designed here first missed how the allwinner case is used
by Fedora. That needs both a zero exit code and allowing what turns out
to be an optional blob to be missing.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20221206/515edba5/attachment.sig>