| Message ID | 20221110021455.1004335-11-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | buildman: Correct various issues with missing blobs | expand |
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.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
(no changes since v1)
tools/buildman/builderthread.py | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Applied to u-boot-dm, thanks!
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. Peter > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > (no changes since v1) > > tools/buildman/builderthread.py | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py > index 6240e08c767..065d836d68c 100644 > --- a/tools/buildman/builderthread.py > +++ b/tools/buildman/builderthread.py > @@ -288,10 +288,14 @@ class BuilderThread(threading.Thread): > args.append('cfg') > result = self.Make(commit, brd, 'build', cwd, *args, > env=env) > + 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 > if adjust_cfg: > errs = cfgutil.check_cfg_file(cfg_file, adjust_cfg) > if errs: > - print('errs', errs) > result.stderr += errs > result.return_code = 1 > result.stderr = result.stderr.replace(src_dir + '/', '') > -- > 2.38.1.431.g37b22c650d-goog >
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 and it needs (I've been assuming, since I'm also passing in SCP) BL31 as 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 now..
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. > 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/ I thought binman was basically default for this now. I tried for the last line the following, it changed the error output but not the failure: BINMAN_ALLOW_MISSING=1 make CROSS_COMPILE="/usr/bin/aarch64-linux-gnu-" O=builds/pine64_plus/ Peter
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) > 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!
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 :-/ Peter
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 . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \ -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \ $(foreach f,$(BINMAN_INDIRS),-I $(f)) \ Which means that this then works: $ (export CROSS_COMPILE=~/.buildman-toolchains/gcc-11.1.0-nolibc/aarch64-linux/bin/aarch64-linux- ; make pine64_plus_defconfig O=builds/pine64_plus ; cp /home/trini/work/u-boot/external-binaries/pine64_plus/bl31.bin builds/pine64_plus ; BINMAN_ALLOW_MISSING=1 make O=builds/pine64_plus -sj$(nproc); echo $?) make[1]: Entering directory '/home/trini/work/u-boot/u-boot/builds/pine64_plus' GEN Makefile # # configuration written to .config # make[1]: Leaving directory '/home/trini/work/u-boot/u-boot/builds/pine64_plus' Image 'main-section' is missing external blobs and is non-functional: scp /binman/u-boot-sunxi-with-spl/fit/images/scp/scp: SCP firmware is required for system suspend, but is otherwise optional. Please read the section on SCP firmware in board/sunxi/README.sunxi64 Some images are invalid 0 $ And restores the old behavior, if you insist that you want binaries to be missing, which in your case you do on allwinner (but not elswhere, right?)
On Mon, Dec 5, 2022 at 11:46 PM 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 . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \ > -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \ > $(foreach f,$(BINMAN_INDIRS),-I $(f)) \ > > Which means that this then works: > $ (export CROSS_COMPILE=~/.buildman-toolchains/gcc-11.1.0-nolibc/aarch64-linux/bin/aarch64-linux- ; make pine64_plus_defconfig O=builds/pine64_plus ; cp /home/trini/work/u-boot/external-binaries/pine64_plus/bl31.bin builds/pine64_plus ; BINMAN_ALLOW_MISSING=1 make O=builds/pine64_plus -sj$(nproc); echo $?) > make[1]: Entering directory '/home/trini/work/u-boot/u-boot/builds/pine64_plus' > GEN Makefile > # > # configuration written to .config > # > make[1]: Leaving directory '/home/trini/work/u-boot/u-boot/builds/pine64_plus' > Image 'main-section' is missing external blobs and is non-functional: scp > > /binman/u-boot-sunxi-with-spl/fit/images/scp/scp: > SCP firmware is required for system suspend, but is otherwise optional. > Please read the section on SCP firmware in board/sunxi/README.sunxi64 > > Some images are invalid > 0 > $ > > And restores the old behavior, if you insist that you want binaries to be > missing, which in your case you do on allwinner (but not elswhere, right?) Yep, I believe that's the case AFAIA. P
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? > -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \ > -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \ > $(foreach f,$(BINMAN_INDIRS),-I $(f)) \ > > Which means that this then works: > $ (export CROSS_COMPILE=~/.buildman-toolchains/gcc-11.1.0-nolibc/aarch64-linux/bin/aarch64-linux- ; make pine64_plus_defconfig O=builds/pine64_plus ; cp /home/trini/work/u-boot/external-binaries/pine64_plus/bl31.bin builds/pine64_plus ; BINMAN_ALLOW_MISSING=1 make O=builds/pine64_plus -sj$(nproc); echo $?) > make[1]: Entering directory '/home/trini/work/u-boot/u-boot/builds/pine64_plus' > GEN Makefile > # > # configuration written to .config > # > make[1]: Leaving directory '/home/trini/work/u-boot/u-boot/builds/pine64_plus' > Image 'main-section' is missing external blobs and is non-functional: scp > > /binman/u-boot-sunxi-with-spl/fit/images/scp/scp: > SCP firmware is required for system suspend, but is otherwise optional. > Please read the section on SCP firmware in board/sunxi/README.sunxi64 > > Some images are invalid > 0 > $ > > And restores the old behavior, if you insist that you want binaries to be > missing, which in your case you do on allwinner (but not elswhere, right?) > > -- > Tom Regards, SImon
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.
Hi Tom, On Tue, 6 Dec 2022 at 12:56, Tom Rini <trini@konsulko.com> wrote: > > 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. Things that use mkimage to generate stuff will need the faked blobs. Regards, Simon
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 6240e08c767..065d836d68c 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -288,10 +288,14 @@ class BuilderThread(threading.Thread): args.append('cfg') result = self.Make(commit, brd, 'build', cwd, *args, env=env) + 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 if adjust_cfg: errs = cfgutil.check_cfg_file(cfg_file, adjust_cfg) if errs: - print('errs', errs) result.stderr += errs result.return_code = 1 result.stderr = result.stderr.replace(src_dir + '/', '')
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. Signed-off-by: Simon Glass <sjg@chromium.org> --- (no changes since v1) tools/buildman/builderthread.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)