Hi Tom, On Tue, 13 Dec 2022 at 09:39, Tom Rini <tr...@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 <tr...@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 <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? > > > > > > 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. Regards, Simon
