Re: [PATCH v5 10/16] buildman: Detect binman reporting missing blobs

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4828 bytes --]

On Tue, Dec 06, 2022 at 12:55:08PM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 6 Dec 2022 at 12:46, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Dec 05, 2022 at 11:43:24PM +0000, Peter Robinson wrote:
> > > On Mon, Dec 5, 2022 at 11:35 PM Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Mon, Dec 05, 2022 at 11:29:30PM +0000, Peter Robinson wrote:
> > > > > On Mon, Dec 5, 2022 at 11:23 PM Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Mon, Dec 05, 2022 at 11:13:03PM +0000, Peter Robinson wrote:
> > > > > > > On Thu, Nov 10, 2022 at 2:17 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Buildman should consider a build as a success (with warnings) if missing
> > > > > > > > blobs have been dealt with by binman, even though buildman itself returns
> > > > > > > > and error code overall. This is how other warnings are dealt with.
> > > > > > > >
> > > > > > > > We cannot easily access the 103 exit code, so detect the problem in the
> > > > > > > > output.
> > > > > > > >
> > > > > > > > With this change, missing blobs result in an exit code of 101, although
> > > > > > > > they still indicate failure.
> > > > > > >
> > > > > > > So either this or Tom's change of "buildman: Add --allow-missing flag
> > > > > > > to allow missing blobs" has broken rc3 builds for Allwinner boards on
> > > > > > > Fedora. Tom's isn't a clean revert and I've not had time to test that
> > > > > > > but either way the SCP firmware is optional and it works just fine,
> > > > > > > ATM we don't have the SCP firmware available to Fedora builds.
> > > > > > >
> > > > > > > Maybe that sort of of change to the build is expected but which ever
> > > > > > > patch it is, and adding "BINMAN_ALLOW_MISSING=1" changes the error but
> > > > > > > doesn't change the overall failure, I wouldn't expect this sort of
> > > > > > > breakage so late in the cycle.
> > > > > > >
> > > > > > > Do either of you know which one does the hard breakage here? I thought
> > > > > > > I'd highlight it now because I don't have time over the next two weeks
> > > > > > > to fully investigate the regression.
> > > > > >
> > > > > > So, is this for 32bit or 64bit? I only have a 64bit allwinner in my lab
> > > > >
> > > > > 64 bit, 32 bit is EOL in Fedora as of F-36.
> > > > >
> > > > > > and it needs (I've been assuming, since I'm also passing in SCP) BL31 as
> > > > >
> > > > > BL31 isn't the same as SCP, the later is a firmware for the onboard
> > > > > PMIC co-processor where as BL31 is Arm Trusted Firmware.
> > > >
> > > > Right, yes.
> > > >
> > > > > > well.  And since you're mentioning buildman, I assume Fedora IS using
> > > > > > that rather than make to build everything. I'll go and think about this
> > > > >
> > > > > I'm using:
> > > > > make pine64_plus_defconfig O=builds/pine64_plus/
> > > > > cp /usr/share/arm-trusted-firmware/sun50i_a64/bl31.bin builds/pine64_plus/
> > > > > make CROSS_COMPILE="/usr/bin/aarch64-linux-gnu-" O=builds/pine64_plus/
> > > >
> > > > OK, that's a little different than how I run make, that's why it wasn't
> > > > caught at least.  I do:
> > > > export SCP=/home/trini/work/u-boot/external-binaries/pine64_plus/scp.bin
> > > > export BL31=/home/trini/work/u-boot/external-binaries/pine64_plus/bl31.bin
> > > > make O=/tmp/pine64_plus pine64_plus_defconfig all -sj$(nproc)
> > >
> > > We build ~90 boards so we've historically copied it to each of the
> > > board build output directories, could look at setting vars for each of
> > > the loops too.
> > >
> > > > > I thought binman was basically default for this now.
> > > >
> > > > We have too many *man tools sometimes. I thought you said buildman, yes,
> > > > binman assembles the images here, when invoking make.  Digging more now,
> > > > thanks!
> > >
> > > It could easily be me getting confused, trying to balance a lot of
> > > plates right now :-/
> >
> > OK, so yes, you've found a problem here. What I need to throw a CI loop
> > at now is:
> > diff --git a/Makefile b/Makefile
> > index d48f52f2943b..b2253ac8ecde 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 think you need to keep the old flag too, right?

Not in my first pine64_plus only test, but I just threw CI at the world,
so pass-or-fireworks in about an hour.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]