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

[Tom Rini]

On Sat, Dec 17, 2022 at 02:40:36PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 13 Dec 2022 at 09:39, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Mon, Dec 12, 2022 at 10:59:01PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 12 Dec 2022 at 16:43, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Tue, Dec 06, 2022 at 10:03:37AM -0500, Tom Rini wrote:
> > > > > 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?
> > > >
> > > > Simon, can you test this last case by chance? I think that's the one
> > > > that's unclear about if this patch breaks or not and I want to know if I
> > > > need to respin to still include the fake blobs flag still too or not.
> > > > Or does anyone else use this case and can test?
> > >
> > > Yes I have your email printed out but travel and family things have
> > > made it hard to get to it. I do think you need the flag, but the extra
> > > case you mention need another look and probably a test, as you say.
> >
> > Yeah, understandable. My plan is to take this patch before the next -rc,
> > so Fedora won't be broken and if we need the other flag (and I'll check
> > in before release) we can follow up there and have a fixes tag on top of
> > this one here.
> 
> I just sent a patch that allows entries (with external blobs) to be
> marked as optional. I believe this covers the remaining case.
> 
> You still need to keep --fake-ext-blobs (in this case) since otherwise
> boards that rely on faked blobs will not build - e.g. will get an
> error in the mkimage step if an input file is missing.

OK thanks, we'll pick that up after the next rc.

-- 
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/20221218/eb362ef6/attachment.sig>