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 <tr...@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 <s...@chromium.org> > > Reported-by: Peter Robinson <pbrobin...@gmail.com> > > Signed-off-by: Tom Rini <tr...@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
signature.asc
Description: PGP signature
