[PATCH 1/2] global: Do not default to faking missing binaries for buildman

[PATCH 1/2] global: Do not default to faking missing binaries for buildman

[Tom Rini]

While it is possible and documented on how to re-run buildman to replace
faked required binary files after the fact, this behavior ends up being
more confusing than helpful in practice. Switch to requiring
BINMAN_ALLOW_MISSING=1 to be passed on the 'make' line to enable this
behavior.

Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Simon Glass <sjg@chromium.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 45f10759a137..535e52443ebf 100644
--- a/Makefile
+++ b/Makefile
@@ -1345,8 +1345,8 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
 		$(foreach f,$(BINMAN_TOOLPATHS),--toolpath $(f)) \
                 --toolpath $(objtree)/tools \
 		$(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
-		build -u -d u-boot.dtb -O . -m --allow-missing \
-		--fake-ext-blobs \
+		build -u -d u-boot.dtb -O . -m \
+		$(if $(BINMAN_ALLOW_MISSING),--allow-missing --fake-ext-blobs) \
 		-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

[PATCH 2/2] buildman: Add --allow-missing-binaries flag to build with BINMAN_ALLOW_MISSING=1

[Tom Rini]

Add a new flag to buildman so that we will in turn pass
BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.

Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Simon Glass <sjg@chromium.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 .azure-pipelines.yml            | 2 +-
 .gitlab-ci.yml                  | 6 +++---
 tools/buildman/builder.py       | 5 ++++-
 tools/buildman/builderthread.py | 2 ++
 tools/buildman/cmdline.py       | 3 +++
 tools/buildman/control.py       | 3 ++-
 6 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
index f200b40dbb24..c932c2b3c619 100644
--- a/.azure-pipelines.yml
+++ b/.azure-pipelines.yml
@@ -553,7 +553,7 @@ stages:
           cat << "EOF" >> build.sh
           if [[ "${BUILDMAN}" != "" ]]; then
               ret=0;
-              tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
+              tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;
               if [[ $ret -ne 0 ]]; then
                   tools/buildman/buildman -o /tmp -seP ${BUILDMAN};
                   exit $ret;
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 7052a6061cb3..d9fb6518a0af 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -81,7 +81,7 @@ build all 32bit ARM platforms:
   stage: world build
   script:
     - ret=0;
-      ./tools/buildman/buildman -o /tmp -P -E -W arm -x aarch64 || ret=$?;
+      ./tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries arm -x aarch64 || ret=$?;
       if [[ $ret -ne 0 ]]; then
         ./tools/buildman/buildman -o /tmp -seP;
         exit $ret;
@@ -94,7 +94,7 @@ build all 64bit ARM platforms:
     - . /tmp/venv/bin/activate
     - pip install pyelftools
     - ret=0;
-      ./tools/buildman/buildman -o /tmp -P -E -W aarch64 || ret=$?;
+      ./tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries aarch64 || ret=$?;
       if [[ $ret -ne 0 ]]; then
         ./tools/buildman/buildman -o /tmp -seP;
         exit $ret;
@@ -114,7 +114,7 @@ build all other platforms:
   stage: world build
   script:
     - ret=0;
-      ./tools/buildman/buildman -o /tmp -P -E -W -x arm,powerpc || ret=$?;
+      ./tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries -x arm,powerpc || ret=$?;
       if [[ $ret -ne 0 ]]; then
         ./tools/buildman/buildman -o /tmp -seP;
         exit $ret;
diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index 76252b90792a..c07e2caab9a3 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -252,7 +252,8 @@ class Builder:
                  mrproper=False, per_board_out_dir=False,
                  config_only=False, squash_config_y=False,
                  warnings_as_errors=False, work_in_output=False,
-                 test_thread_exceptions=False, adjust_cfg=None):
+                 test_thread_exceptions=False, adjust_cfg=None,
+                 allow_missing_binaries=False):
         """Create a new Builder object
 
         Args:
@@ -290,6 +291,7 @@ class Builder:
                     ~C to disable C
                     C=val to set the value of C (val must have quotes if C is
                         a string Kconfig
+            allow_missing_binaries: Run build with BINMAN_ALLOW_MISSING=1
 
         """
         self.toolchains = toolchains
@@ -327,6 +329,7 @@ class Builder:
         self.config_filenames = BASE_CONFIG_FILENAMES
         self.work_in_output = work_in_output
         self.adjust_cfg = adjust_cfg
+        self.allow_missing_binaries = allow_missing_binaries
         self._ide = False
 
         if not self.squash_config_y:
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 6240e08c7670..722ee17f0a03 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -253,6 +253,8 @@ class BuilderThread(threading.Thread):
                     args.extend(['-j', str(self.builder.num_jobs)])
                 if self.builder.warnings_as_errors:
                     args.append('KCFLAGS=-Werror')
+                if self.builder.allow_missing_binaries:
+                    args.append('BINMAN_ALLOW_MISSING=1')
                 config_args = ['%s_defconfig' % brd.target]
                 config_out = ''
                 args.extend(self.builder.toolchains.GetMakeArguments(brd))
diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
index b29c1eb5ee72..18ef1902c7cd 100644
--- a/tools/buildman/cmdline.py
+++ b/tools/buildman/cmdline.py
@@ -15,6 +15,9 @@ def ParseArgs():
     parser = OptionParser()
     parser.add_option('-a', '--adjust-cfg', type=str, action='append',
           help='Adjust the Kconfig settings in .config before building')
+    parser.add_option('--allow-missing-binaries', action='store_true',
+          default=False, help='Tell binman to allow for external binaries to'
+              ' be missing and generate fake ones as needed'),
     parser.add_option('-A', '--print-prefix', action='store_true',
           help='Print the tool-chain prefix for a board (CROSS_COMPILE=)')
     parser.add_option('-b', '--branch', type='string',
diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 0c75466fbd3d..1289daeaef7e 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -329,7 +329,8 @@ def DoBuildman(options, args, toolchains=None, make_func=None, brds=None,
             warnings_as_errors=options.warnings_as_errors,
             work_in_output=options.work_in_output,
             test_thread_exceptions=test_thread_exceptions,
-            adjust_cfg=adjust_cfg)
+            adjust_cfg=adjust_cfg,
+            allow_missing_binaries=options.allow_missing_binaries)
     builder.force_config_on_failure = not options.quick
     if make_func:
         builder.do_make = make_func
-- 
2.25.1

Re: [PATCH 2/2] buildman: Add --allow-missing-binaries flag to build with BINMAN_ALLOW_MISSING=1

[Simon Glass]

Hi Tom,

On Mon, 10 Oct 2022 at 09:18, Tom Rini <trini@konsulko.com> wrote:
>
> Add a new flag to buildman so that we will in turn pass
> BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Cc: Simon Glass <sjg@chromium.org>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>  .azure-pipelines.yml            | 2 +-
>  .gitlab-ci.yml                  | 6 +++---
>  tools/buildman/builder.py       | 5 ++++-
>  tools/buildman/builderthread.py | 2 ++
>  tools/buildman/cmdline.py       | 3 +++
>  tools/buildman/control.py       | 3 ++-
>  6 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> index f200b40dbb24..c932c2b3c619 100644
> --- a/.azure-pipelines.yml
> +++ b/.azure-pipelines.yml
> @@ -553,7 +553,7 @@ stages:
>            cat << "EOF" >> build.sh
>            if [[ "${BUILDMAN}" != "" ]]; then
>                ret=0;
> -              tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
> +              tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;

This is fine for CI.

But having to provide a flag to do build testing seems like the tail
is wagging the dog. Boards should be discouraged to use blobs and we
don't want to make it hard for everyone else (who doesn't have the
blobs) to test whether their patch breaks a build.

Regards,
Simon

Re: [PATCH 1/2] global: Do not default to faking missing binaries for buildman

[Simon Glass]

Hi Tom,

On Mon, 10 Oct 2022 at 09:18, Tom Rini <trini@konsulko.com> wrote:
>
> While it is possible and documented on how to re-run buildman to replace
> faked required binary files after the fact, this behavior ends up being
> more confusing than helpful in practice. Switch to requiring
> BINMAN_ALLOW_MISSING=1 to be passed on the 'make' line to enable this
> behavior.
>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Cc: Simon Glass <sjg@chromium.org>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 45f10759a137..535e52443ebf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1345,8 +1345,8 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
>                 $(foreach f,$(BINMAN_TOOLPATHS),--toolpath $(f)) \
>                  --toolpath $(objtree)/tools \
>                 $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
> -               build -u -d u-boot.dtb -O . -m --allow-missing \
> -               --fake-ext-blobs \
> +               build -u -d u-boot.dtb -O . -m \
> +               $(if $(BINMAN_ALLOW_MISSING),--allow-missing --fake-ext-blobs) \
>                 -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'm not sure about this patch, but it might be good.

Basically the difference between this and the series I sent is that
here, binman is told to fail as soon as it sees a missing blob, rather
than finishing the image build and then failing.

So, for my approach, we get to see all the missing blobs and can
presumably deal with them together

With this approach, we need to do them one at a time, but a benefit is
that we do actually get that immediate failure and no messages about
missing or faked blobs.

There are pros and cons.

Regards,
Simon

Re: [PATCH 1/2] global: Do not default to faking missing binaries for buildman

[Rasmus Villemoes]

On 10/10/2022 17.18, Tom Rini wrote:
> While it is possible and documented on how to re-run buildman to replace
> faked required binary files after the fact, this behavior ends up being
> more confusing than helpful in practice. Switch to requiring
> BINMAN_ALLOW_MISSING=1 to be passed on the 'make' line to enable this
> behavior.

Recreating the bitbake scenario that bit me with this applied correctly
failed to build. Thanks.

Tested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Re: [PATCH 1/2] global: Do not default to faking missing binaries for buildman

[Simon Glass]

Hi Rasmus,

On Tue, 11 Oct 2022 at 00:41, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 10/10/2022 17.18, Tom Rini wrote:
> > While it is possible and documented on how to re-run buildman to replace
> > faked required binary files after the fact, this behavior ends up being
> > more confusing than helpful in practice. Switch to requiring
> > BINMAN_ALLOW_MISSING=1 to be passed on the 'make' line to enable this
> > behavior.
>
> Recreating the bitbake scenario that bit me with this applied correctly
> failed to build. Thanks.
>
> Tested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

If you have time, can you try this one instead (or the series):

https://patchwork.ozlabs.org/project/uboot/patch/20221010200032.73483-5-sjg@chromium.org/

Regards,
Simon

Re: [PATCH 2/2] buildman: Add --allow-missing-binaries flag to build with BINMAN_ALLOW_MISSING=1

[Tom Rini]

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

On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 10 Oct 2022 at 09:18, Tom Rini <trini@konsulko.com> wrote:
> >
> > Add a new flag to buildman so that we will in turn pass
> > BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
> >
> > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > Cc: Simon Glass <sjg@chromium.org>
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > ---
> >  .azure-pipelines.yml            | 2 +-
> >  .gitlab-ci.yml                  | 6 +++---
> >  tools/buildman/builder.py       | 5 ++++-
> >  tools/buildman/builderthread.py | 2 ++
> >  tools/buildman/cmdline.py       | 3 +++
> >  tools/buildman/control.py       | 3 ++-
> >  6 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > index f200b40dbb24..c932c2b3c619 100644
> > --- a/.azure-pipelines.yml
> > +++ b/.azure-pipelines.yml
> > @@ -553,7 +553,7 @@ stages:
> >            cat << "EOF" >> build.sh
> >            if [[ "${BUILDMAN}" != "" ]]; then
> >                ret=0;
> > -              tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
> > +              tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;
> 
> This is fine for CI.

I think one of the issues here is we need to agree on the common use
cases. I don't think most people use buildman to build, they use make.
Of the people that do use buildman, how many aren't already using a
wrapper script? I know I always do. Can we also not just deal with
setting this flag in ~/.buildman ? Would it Just Work as:
[builder]
allow-missing-binaries = True

Or is more code needed?

> But having to provide a flag to do build testing seems like the tail
> is wagging the dog. Boards should be discouraged to use blobs and we
> don't want to make it hard for everyone else (who doesn't have the
> blobs) to test whether their patch breaks a build.

I'm not sure this ends up being a common case. If someone is build
testing for something they can't run test, the error is going to be
after whatever they compile-tested was compiled/linked, and if they're
testing everything it's via CI.

-- 
Tom

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

Re: [PATCH 2/2] buildman: Add --allow-missing-binaries flag to build with BINMAN_ALLOW_MISSING=1

[Simon Glass]

Hi Tom,

On Tue, 11 Oct 2022 at 10:22, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 10 Oct 2022 at 09:18, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > Add a new flag to buildman so that we will in turn pass
> > > BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
> > >
> > > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > Cc: Simon Glass <sjg@chromium.org>
> > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > ---
> > >  .azure-pipelines.yml            | 2 +-
> > >  .gitlab-ci.yml                  | 6 +++---
> > >  tools/buildman/builder.py       | 5 ++++-
> > >  tools/buildman/builderthread.py | 2 ++
> > >  tools/buildman/cmdline.py       | 3 +++
> > >  tools/buildman/control.py       | 3 ++-
> > >  6 files changed, 15 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > > index f200b40dbb24..c932c2b3c619 100644
> > > --- a/.azure-pipelines.yml
> > > +++ b/.azure-pipelines.yml
> > > @@ -553,7 +553,7 @@ stages:
> > >            cat << "EOF" >> build.sh
> > >            if [[ "${BUILDMAN}" != "" ]]; then
> > >                ret=0;
> > > -              tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
> > > +              tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;
> >
> > This is fine for CI.
>
> I think one of the issues here is we need to agree on the common use
> cases. I don't think most people use buildman to build, they use make.

OK. With the new -I/--ide flag I use buildman directly for single
builds on some machines at least.

> Of the people that do use buildman, how many aren't already using a
> wrapper script? I know I always do.

I don't know, but I always use buildman directly for build-testing
multiple boards.

> Can we also not just deal with
> setting this flag in ~/.buildman ? Would it Just Work as:
> [builder]
> allow-missing-binaries = True
>
> Or is more code needed?

Yes I think that would work, although I think we would need two flags,
one for building current source (where people might want it off) and
another for building multiple boards (when presumably you want it on).

...although I don't see the advantage of this approach over the series
I sent. Basically, the build should fail if there are missing blobs.
The only question is whether we want to handle missing blobs by:

1. failing immediately when we see the first missing blob (your approach)
2. failing at the end after all binman processing is done (my approach)

The nice thing about 2 is that the build does complete and the exit
code lets buildman decide what to do afterwards (i.e. we can make
missing blobs be an error or a warning).

Option 1 has the benefit that we don't do any of the blob handling, so
it just dies right away. Perhaps this is a philosophical question, but
it is a little subtle and I'm not actually sure people would notice
the difference so long as they get the errors as expected.

BTW we need a one-character flag if we do this as it will be a very
common requirement.

>
> > But having to provide a flag to do build testing seems like the tail
> > is wagging the dog. Boards should be discouraged to use blobs and we
> > don't want to make it hard for everyone else (who doesn't have the
> > blobs) to test whether their patch breaks a build.
>
> I'm not sure this ends up being a common case. If someone is build
> testing for something they can't run test, the error is going to be
> after whatever they compile-tested was compiled/linked, and if they're
> testing everything it's via CI.

I don't think people will be able to tell that. The 'make' fails with
an error so it looks like a failure. It happens to be at the end in
the binman step, but it is still a build failure.

My overall concern is constantly having to rerun a build because I
forgot to add a flag, when I don't have the blobs for the boards
anyway. Like the LTO thing that cost 4 seconds on every build, that
could get really annoying. Blobs should not get in the way of
development.

Regards,
Simon

Re: [PATCH 2/2] buildman: Add --allow-missing-binaries flag to build with BINMAN_ALLOW_MISSING=1

[Tom Rini]

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

On Wed, Oct 12, 2022 at 06:59:35AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 11 Oct 2022 at 10:22, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 10 Oct 2022 at 09:18, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > Add a new flag to buildman so that we will in turn pass
> > > > BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
> > > >
> > > > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > ---
> > > >  .azure-pipelines.yml            | 2 +-
> > > >  .gitlab-ci.yml                  | 6 +++---
> > > >  tools/buildman/builder.py       | 5 ++++-
> > > >  tools/buildman/builderthread.py | 2 ++
> > > >  tools/buildman/cmdline.py       | 3 +++
> > > >  tools/buildman/control.py       | 3 ++-
> > > >  6 files changed, 15 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > > > index f200b40dbb24..c932c2b3c619 100644
> > > > --- a/.azure-pipelines.yml
> > > > +++ b/.azure-pipelines.yml
> > > > @@ -553,7 +553,7 @@ stages:
> > > >            cat << "EOF" >> build.sh
> > > >            if [[ "${BUILDMAN}" != "" ]]; then
> > > >                ret=0;
> > > > -              tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
> > > > +              tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;
> > >
> > > This is fine for CI.
> >
> > I think one of the issues here is we need to agree on the common use
> > cases. I don't think most people use buildman to build, they use make.
> 
> OK. With the new -I/--ide flag I use buildman directly for single
> builds on some machines at least.
> > Of the people that do use buildman, how many aren't already using a
> > wrapper script? I know I always do.
> 
> I don't know, but I always use buildman directly for build-testing
> multiple boards.

I've been building for single machines with buildman for years, with
--keep being passed to my wrapper script. And always use it for multiple
boards. But this is you and I, to borrow a meme, we're builders georg
and are outliers who should perhaps not be counted.

> > Can we also not just deal with
> > setting this flag in ~/.buildman ? Would it Just Work as:
> > [builder]
> > allow-missing-binaries = True
> >
> > Or is more code needed?
> 
> Yes I think that would work, although I think we would need two flags,
> one for building current source (where people might want it off) and
> another for building multiple boards (when presumably you want it on).

I'm not sure. It's not "always use fake binaries" it's allow them. I'm
honestly not sure how to tell buildman to use external binaries
correctly, that's one of the cases where I use a different script that
just manages output directories and known-good additional binaries.

> ...although I don't see the advantage of this approach over the series
> I sent. Basically, the build should fail if there are missing blobs.
> The only question is whether we want to handle missing blobs by:
> 
> 1. failing immediately when we see the first missing blob (your approach)
> 2. failing at the end after all binman processing is done (my approach)
> 
> The nice thing about 2 is that the build does complete and the exit
> code lets buildman decide what to do afterwards (i.e. we can make
> missing blobs be an error or a warning).
> 
> Option 1 has the benefit that we don't do any of the blob handling, so
> it just dies right away. Perhaps this is a philosophical question, but
> it is a little subtle and I'm not actually sure people would notice
> the difference so long as they get the errors as expected.

The way I'm thinking of it is there's two cases. The first case is
someone who is testing on the hardware that requires these files. Stop
ASAP because they either will know they forgot to pass X/Y/Z files or
they'll re-read the board instructions page and see oh, they need to
grab binaries X/Y/Z too. Waiting to collect up missing file errors
doesn't save time. It's also still hugely likely I believe that they're
using make and not buildman.

The second case is someone building multiple boards at once to
build-check (but not run-time check) something in multiple places. This
is where passing a specific failure exit code can be helpful, yes.

> BTW we need a one-character flag if we do this as it will be a very
> common requirement.

Sadly both -a and -A are taken. We could use -M for Missing ?

> > > But having to provide a flag to do build testing seems like the tail
> > > is wagging the dog. Boards should be discouraged to use blobs and we
> > > don't want to make it hard for everyone else (who doesn't have the
> > > blobs) to test whether their patch breaks a build.
> >
> > I'm not sure this ends up being a common case. If someone is build
> > testing for something they can't run test, the error is going to be
> > after whatever they compile-tested was compiled/linked, and if they're
> > testing everything it's via CI.
> 
> I don't think people will be able to tell that. The 'make' fails with
> an error so it looks like a failure. It happens to be at the end in
> the binman step, but it is still a build failure.
> 
> My overall concern is constantly having to rerun a build because I
> forgot to add a flag, when I don't have the blobs for the boards
> anyway. Like the LTO thing that cost 4 seconds on every build, that
> could get really annoying. Blobs should not get in the way of
> development.

The counter problem is this isn't the first time someone has come up and
noted how much time they've wasted because we defaulted to fake
binaries. I think we've optimized too much for the people that build a
thousand configs all the time (us) instead of the people that build for
1 or two configs at a time (most people?).

To that end, I am really curious what Rasmus has to say here, or anyone
else that has a different workflow from you and I.

-- 
Tom

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

Re: [PATCH 2/2] buildman: Add --allow-missing-binaries flag to build with BINMAN_ALLOW_MISSING=1

[Rasmus Villemoes]

On 12/10/2022 16.52, Tom Rini wrote:

>> Option 1 has the benefit that we don't do any of the blob handling, so
>> it just dies right away. Perhaps this is a philosophical question, but
>> it is a little subtle and I'm not actually sure people would notice
>> the difference so long as they get the errors as expected.
> 
> The way I'm thinking of it is there's two cases. The first case is
> someone who is testing on the hardware that requires these files. Stop
> ASAP because they either will know they forgot to pass X/Y/Z files or
> they'll re-read the board instructions page and see oh, they need to
> grab binaries X/Y/Z too. Waiting to collect up missing file errors
> doesn't save time. 

Indeed. If I forgot to put lpddr4_pmu_train_1d_dmem_202006.bin in my
build folder, it is extremely unlikely I wouldn't remember to do the
other three lpddr*.bin files at the same time. And even if there's some
fifth file I also forgot, re-running make (yes, make) after putting the
lpddr*.bin files in place doesn't take long since the whole tree is
already built. I don't think any board really needs more than a handful
of blobs (hopefully), and certainly not more than from a few sources
(i.e., I count the imx8 lpddr*.bin as one), so there's really
practically not much difference between "stop as soon as one is missing"
or "give an error at the end and print all the missing stuff".

Personally, I usually prefer the "stop at first fatal error" model,
because that makes it easier to see the actual problem - some of the
follow-on errors/warnings could be due the first problem, and sometimes
not stopping on that first error means the same problem gets printed
over and over, making it hard to scroll back to find the first
occurrence. Somewhat hand-wavy, yes, and I can't give any good examples.

> The counter problem is this isn't the first time someone has come up and
> noted how much time they've wasted because we defaulted to fake
> binaries. I think we've optimized too much for the people that build a
> thousand configs all the time (us) instead of the people that build for
> 1 or two configs at a time (most people?).
> 
> To that end, I am really curious what Rasmus has to say here, or anyone
> else that has a different workflow from you and I.
> 

Indeed my workflow never involves building a thousand configs, I leave
that to upstream's CI.

I have roughly four different ways of building, all of them must fail if
the resulting binary is known to be unusable (and I think we all agree
on that part), and preferably without having to pass special flags to
however the build is done (i.e., failing must be default, and I also
think we're in agreement there), because otherwise I know those flags
would be missed sometimes. Just to enumerate:

(1) I do local development, building in my xterm, and testing on target
either by booting the binary with uuu or (for some other boards/SOCs)
scp'ing it to target or however it can easily be deployed and tested.

(2) Once this has stabilized, I update our bitbake metadata to point at
the new branch/commit, and do a local build with yocto (which is always
done inside a docker container based on a specific image that we and our
customers agree on using). That primarily catches things like missing
host tools or libraries that I may happen to have on my dev machine but
which are not in the docker image, or not exposed by Yocto. This can
then either mean the recipe needs to grow a new DEPENDS, or (which is
thankfully pretty rare) our docker image definition needs to be updated.

(3) When I can build with Yocto, it's time for my customer's CI to chew
on it. Depending on the project, that sometimes involves automatically
deploying the new bootloader to a test rack - which is why it's so
important that we do not pass a build if the binary is known-broken.

(4) [And we're not here yet, but pretty close, which is why I've been
rather active pushing stuff upstream the past few months] We want to
have a CI job set up for automatically merging upstream master into our
private branch, at least every -rc release, build test that and if
successful, deploy it to target; if not (or if the merge cannot be done
automatically in the first place), send an email so we're aware of it
before the next release happens. So far, I've found three bugs in
v2022.10 that could have been avoided (i.e. fixed before release) if
we/I had this in place.

Since I'm writing this wall of text anyway, let me explain how I was
bitten by the build not failing: I had added a new blob requirement (not
a "proprietary" thing, just changing the logic so that the boot script
is included in the u-boot fit image as a "loadable" and thus
automatically loaded to a known location by SPL, instead of having
U-Boot itself load it from somewhere), and locally added that
bootscript.itb to my build folder when testing; I had also duly updated
the bitbake recipe to copy bootscript.itb to "${B}" before do_compile,
but failed to remember that that was not the right place to put it
because the actual build folder is "${B}/<config name>". My own yocto
build succeeded, I deployed the binary to the board on my desk, and it
didn't work... Only then did I go look in do_compile.log and found the
warning(s).

Rasmus

Re: [PATCH 2/2] buildman: Add --allow-missing-binaries flag to build with BINMAN_ALLOW_MISSING=1

[Simon Glass]

Hi Tom,

On Wed, 12 Oct 2022 at 08:52, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Oct 12, 2022 at 06:59:35AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 11 Oct 2022 at 10:22, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 10 Oct 2022 at 09:18, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > Add a new flag to buildman so that we will in turn pass
> > > > > BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
> > > > >
> > > > > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > ---
> > > > >  .azure-pipelines.yml            | 2 +-
> > > > >  .gitlab-ci.yml                  | 6 +++---
> > > > >  tools/buildman/builder.py       | 5 ++++-
> > > > >  tools/buildman/builderthread.py | 2 ++
> > > > >  tools/buildman/cmdline.py       | 3 +++
> > > > >  tools/buildman/control.py       | 3 ++-
> > > > >  6 files changed, 15 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > > > > index f200b40dbb24..c932c2b3c619 100644
> > > > > --- a/.azure-pipelines.yml
> > > > > +++ b/.azure-pipelines.yml
> > > > > @@ -553,7 +553,7 @@ stages:
> > > > >            cat << "EOF" >> build.sh
> > > > >            if [[ "${BUILDMAN}" != "" ]]; then
> > > > >                ret=0;
> > > > > -              tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
> > > > > +              tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;
> > > >
> > > > This is fine for CI.
> > >
> > > I think one of the issues here is we need to agree on the common use
> > > cases. I don't think most people use buildman to build, they use make.
> >
> > OK. With the new -I/--ide flag I use buildman directly for single
> > builds on some machines at least.
> > > Of the people that do use buildman, how many aren't already using a
> > > wrapper script? I know I always do.
> >
> > I don't know, but I always use buildman directly for build-testing
> > multiple boards.
>
> I've been building for single machines with buildman for years, with
> --keep being passed to my wrapper script. And always use it for multiple
> boards. But this is you and I, to borrow a meme, we're builders georg
> and are outliers who should perhaps not be counted.

Yes, but we need to preserve that workflow.

>
> > > Can we also not just deal with
> > > setting this flag in ~/.buildman ? Would it Just Work as:
> > > [builder]
> > > allow-missing-binaries = True
> > >
> > > Or is more code needed?
> >
> > Yes I think that would work, although I think we would need two flags,
> > one for building current source (where people might want it off) and
> > another for building multiple boards (when presumably you want it on).
>
> I'm not sure. It's not "always use fake binaries" it's allow them. I'm
> honestly not sure how to tell buildman to use external binaries
> correctly, that's one of the cases where I use a different script that
> just manages output directories and known-good additional binaries.

To me this is a policy thing. I just don't want to get 50 boards in
and find that the builds start failing due to annoying blobs, or have
it fail on the first blob when there is a binman error beyond just the
first missing blob that is now masked.

>
> > ...although I don't see the advantage of this approach over the series
> > I sent. Basically, the build should fail if there are missing blobs.
> > The only question is whether we want to handle missing blobs by:
> >
> > 1. failing immediately when we see the first missing blob (your approach)
> > 2. failing at the end after all binman processing is done (my approach)
> >
> > The nice thing about 2 is that the build does complete and the exit
> > code lets buildman decide what to do afterwards (i.e. we can make
> > missing blobs be an error or a warning).
> >
> > Option 1 has the benefit that we don't do any of the blob handling, so
> > it just dies right away. Perhaps this is a philosophical question, but
> > it is a little subtle and I'm not actually sure people would notice
> > the difference so long as they get the errors as expected.
>
> The way I'm thinking of it is there's two cases. The first case is
> someone who is testing on the hardware that requires these files. Stop
> ASAP because they either will know they forgot to pass X/Y/Z files or
> they'll re-read the board instructions page and see oh, they need to
> grab binaries X/Y/Z too. Waiting to collect up missing file errors
> doesn't save time. It's also still hugely likely I believe that they're
> using make and not buildman.
>
> The second case is someone building multiple boards at once to
> build-check (but not run-time check) something in multiple places. This
> is where passing a specific failure exit code can be helpful, yes.

Yes that makes sense.

>
> > BTW we need a one-character flag if we do this as it will be a very
> > common requirement.
>
> Sadly both -a and -A are taken. We could use -M for Missing ?

SGTM.

If we apply some of the patches in my series then I think I could
enable that always, since the build will return an exit code if a blob
is missing, which I can disable with -W.

>
> > > > But having to provide a flag to do build testing seems like the tail
> > > > is wagging the dog. Boards should be discouraged to use blobs and we
> > > > don't want to make it hard for everyone else (who doesn't have the
> > > > blobs) to test whether their patch breaks a build.
> > >
> > > I'm not sure this ends up being a common case. If someone is build
> > > testing for something they can't run test, the error is going to be
> > > after whatever they compile-tested was compiled/linked, and if they're
> > > testing everything it's via CI.
> >
> > I don't think people will be able to tell that. The 'make' fails with
> > an error so it looks like a failure. It happens to be at the end in
> > the binman step, but it is still a build failure.
> >
> > My overall concern is constantly having to rerun a build because I
> > forgot to add a flag, when I don't have the blobs for the boards
> > anyway. Like the LTO thing that cost 4 seconds on every build, that
> > could get really annoying. Blobs should not get in the way of
> > development.
>
> The counter problem is this isn't the first time someone has come up and
> noted how much time they've wasted because we defaulted to fake
> binaries. I think we've optimized too much for the people that build a
> thousand configs all the time (us) instead of the people that build for
> 1 or two configs at a time (most people?).
>
> To that end, I am really curious what Rasmus has to say here, or anyone
> else that has a different workflow from you and I.

The main problem I think is that 'binman' should have emitted an exit
code if there are missing blobs. That is [1] and I think was just an
oversight on my part, or not thinking through things clearly enough.

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20221011141541.538175-5-sjg@chromium.org/

Re: [PATCH 2/2] buildman: Add --allow-missing-binaries flag to build with BINMAN_ALLOW_MISSING=1

[Tom Rini]

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

On Fri, Oct 14, 2022 at 09:56:40AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 12 Oct 2022 at 08:52, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Oct 12, 2022 at 06:59:35AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 11 Oct 2022 at 10:22, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Mon, 10 Oct 2022 at 09:18, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > Add a new flag to buildman so that we will in turn pass
> > > > > > BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
> > > > > >
> > > > > > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > ---
> > > > > >  .azure-pipelines.yml            | 2 +-
> > > > > >  .gitlab-ci.yml                  | 6 +++---
> > > > > >  tools/buildman/builder.py       | 5 ++++-
> > > > > >  tools/buildman/builderthread.py | 2 ++
> > > > > >  tools/buildman/cmdline.py       | 3 +++
> > > > > >  tools/buildman/control.py       | 3 ++-
> > > > > >  6 files changed, 15 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > > > > > index f200b40dbb24..c932c2b3c619 100644
> > > > > > --- a/.azure-pipelines.yml
> > > > > > +++ b/.azure-pipelines.yml
> > > > > > @@ -553,7 +553,7 @@ stages:
> > > > > >            cat << "EOF" >> build.sh
> > > > > >            if [[ "${BUILDMAN}" != "" ]]; then
> > > > > >                ret=0;
> > > > > > -              tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
> > > > > > +              tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;
> > > > >
> > > > > This is fine for CI.
> > > >
> > > > I think one of the issues here is we need to agree on the common use
> > > > cases. I don't think most people use buildman to build, they use make.
> > >
> > > OK. With the new -I/--ide flag I use buildman directly for single
> > > builds on some machines at least.
> > > > Of the people that do use buildman, how many aren't already using a
> > > > wrapper script? I know I always do.
> > >
> > > I don't know, but I always use buildman directly for build-testing
> > > multiple boards.
> >
> > I've been building for single machines with buildman for years, with
> > --keep being passed to my wrapper script. And always use it for multiple
> > boards. But this is you and I, to borrow a meme, we're builders georg
> > and are outliers who should perhaps not be counted.
> 
> Yes, but we need to preserve that workflow.

Right.

> > > > Can we also not just deal with
> > > > setting this flag in ~/.buildman ? Would it Just Work as:
> > > > [builder]
> > > > allow-missing-binaries = True
> > > >
> > > > Or is more code needed?
> > >
> > > Yes I think that would work, although I think we would need two flags,
> > > one for building current source (where people might want it off) and
> > > another for building multiple boards (when presumably you want it on).
> >
> > I'm not sure. It's not "always use fake binaries" it's allow them. I'm
> > honestly not sure how to tell buildman to use external binaries
> > correctly, that's one of the cases where I use a different script that
> > just manages output directories and known-good additional binaries.
> 
> To me this is a policy thing. I just don't want to get 50 boards in
> and find that the builds start failing due to annoying blobs, or have
> it fail on the first blob when there is a binman error beyond just the
> first missing blob that is now masked.

Yeah, there's some policy here.  But managing policy is what config
files are for too, right? Heck, I could probably drop some of my wrapper
script and move more of it to the config file if I was confident in the
syntax / sections. Maybe that needs to be documented?

> > > ...although I don't see the advantage of this approach over the series
> > > I sent. Basically, the build should fail if there are missing blobs.
> > > The only question is whether we want to handle missing blobs by:
> > >
> > > 1. failing immediately when we see the first missing blob (your approach)
> > > 2. failing at the end after all binman processing is done (my approach)
> > >
> > > The nice thing about 2 is that the build does complete and the exit
> > > code lets buildman decide what to do afterwards (i.e. we can make
> > > missing blobs be an error or a warning).
> > >
> > > Option 1 has the benefit that we don't do any of the blob handling, so
> > > it just dies right away. Perhaps this is a philosophical question, but
> > > it is a little subtle and I'm not actually sure people would notice
> > > the difference so long as they get the errors as expected.
> >
> > The way I'm thinking of it is there's two cases. The first case is
> > someone who is testing on the hardware that requires these files. Stop
> > ASAP because they either will know they forgot to pass X/Y/Z files or
> > they'll re-read the board instructions page and see oh, they need to
> > grab binaries X/Y/Z too. Waiting to collect up missing file errors
> > doesn't save time. It's also still hugely likely I believe that they're
> > using make and not buildman.
> >
> > The second case is someone building multiple boards at once to
> > build-check (but not run-time check) something in multiple places. This
> > is where passing a specific failure exit code can be helpful, yes.
> 
> Yes that makes sense.
> 
> >
> > > BTW we need a one-character flag if we do this as it will be a very
> > > common requirement.
> >
> > Sadly both -a and -A are taken. We could use -M for Missing ?
> 
> SGTM.
> 
> If we apply some of the patches in my series then I think I could
> enable that always, since the build will return an exit code if a blob
> is missing, which I can disable with -W.

I think so?

> > > > > But having to provide a flag to do build testing seems like the tail
> > > > > is wagging the dog. Boards should be discouraged to use blobs and we
> > > > > don't want to make it hard for everyone else (who doesn't have the
> > > > > blobs) to test whether their patch breaks a build.
> > > >
> > > > I'm not sure this ends up being a common case. If someone is build
> > > > testing for something they can't run test, the error is going to be
> > > > after whatever they compile-tested was compiled/linked, and if they're
> > > > testing everything it's via CI.
> > >
> > > I don't think people will be able to tell that. The 'make' fails with
> > > an error so it looks like a failure. It happens to be at the end in
> > > the binman step, but it is still a build failure.
> > >
> > > My overall concern is constantly having to rerun a build because I
> > > forgot to add a flag, when I don't have the blobs for the boards
> > > anyway. Like the LTO thing that cost 4 seconds on every build, that
> > > could get really annoying. Blobs should not get in the way of
> > > development.
> >
> > The counter problem is this isn't the first time someone has come up and
> > noted how much time they've wasted because we defaulted to fake
> > binaries. I think we've optimized too much for the people that build a
> > thousand configs all the time (us) instead of the people that build for
> > 1 or two configs at a time (most people?).
> >
> > To that end, I am really curious what Rasmus has to say here, or anyone
> > else that has a different workflow from you and I.
> 
> The main problem I think is that 'binman' should have emitted an exit
> code if there are missing blobs. That is [1] and I think was just an
> oversight on my part, or not thinking through things clearly enough.

OK, can you take / use as inspiration what you think makes sense of my
series as well and v2 your series? I want to grab that "N:" patch asap
since that unblocks a bunch of other platforms that are ready to come
in, otherwise.

-- 
Tom

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

Re: [PATCH 2/2] buildman: Add --allow-missing-binaries flag to build with BINMAN_ALLOW_MISSING=1

[Simon Glass]

Hi Tom,

On Fri, 14 Oct 2022 at 10:44, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Oct 14, 2022 at 09:56:40AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 12 Oct 2022 at 08:52, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Oct 12, 2022 at 06:59:35AM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 11 Oct 2022 at 10:22, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Mon, Oct 10, 2022 at 05:48:55PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Mon, 10 Oct 2022 at 09:18, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > Add a new flag to buildman so that we will in turn pass
> > > > > > > BINMAN_ALLOW_MISSING=1 to 'make'. Make use of this flag in CI.
> > > > > > >
> > > > > > > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > > > ---
> > > > > > >  .azure-pipelines.yml            | 2 +-
> > > > > > >  .gitlab-ci.yml                  | 6 +++---
> > > > > > >  tools/buildman/builder.py       | 5 ++++-
> > > > > > >  tools/buildman/builderthread.py | 2 ++
> > > > > > >  tools/buildman/cmdline.py       | 3 +++
> > > > > > >  tools/buildman/control.py       | 3 ++-
> > > > > > >  6 files changed, 15 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > > > > > > index f200b40dbb24..c932c2b3c619 100644
> > > > > > > --- a/.azure-pipelines.yml
> > > > > > > +++ b/.azure-pipelines.yml
> > > > > > > @@ -553,7 +553,7 @@ stages:
> > > > > > >            cat << "EOF" >> build.sh
> > > > > > >            if [[ "${BUILDMAN}" != "" ]]; then
> > > > > > >                ret=0;
> > > > > > > -              tools/buildman/buildman -o /tmp -P -E -W ${BUILDMAN} ${OVERRIDE} || ret=$?;
> > > > > > > +              tools/buildman/buildman -o /tmp -P -E -W --allow-missing-binaries ${BUILDMAN} ${OVERRIDE} || ret=$?;
> > > > > >
> > > > > > This is fine for CI.
> > > > >
> > > > > I think one of the issues here is we need to agree on the common use
> > > > > cases. I don't think most people use buildman to build, they use make.
> > > >
> > > > OK. With the new -I/--ide flag I use buildman directly for single
> > > > builds on some machines at least.
> > > > > Of the people that do use buildman, how many aren't already using a
> > > > > wrapper script? I know I always do.
> > > >
> > > > I don't know, but I always use buildman directly for build-testing
> > > > multiple boards.
> > >
> > > I've been building for single machines with buildman for years, with
> > > --keep being passed to my wrapper script. And always use it for multiple
> > > boards. But this is you and I, to borrow a meme, we're builders georg
> > > and are outliers who should perhaps not be counted.
> >
> > Yes, but we need to preserve that workflow.
>
> Right.
>
> > > > > Can we also not just deal with
> > > > > setting this flag in ~/.buildman ? Would it Just Work as:
> > > > > [builder]
> > > > > allow-missing-binaries = True
> > > > >
> > > > > Or is more code needed?
> > > >
> > > > Yes I think that would work, although I think we would need two flags,
> > > > one for building current source (where people might want it off) and
> > > > another for building multiple boards (when presumably you want it on).
> > >
> > > I'm not sure. It's not "always use fake binaries" it's allow them. I'm
> > > honestly not sure how to tell buildman to use external binaries
> > > correctly, that's one of the cases where I use a different script that
> > > just manages output directories and known-good additional binaries.
> >
> > To me this is a policy thing. I just don't want to get 50 boards in
> > and find that the builds start failing due to annoying blobs, or have
> > it fail on the first blob when there is a binman error beyond just the
> > first missing blob that is now masked.
>
> Yeah, there's some policy here.  But managing policy is what config
> files are for too, right? Heck, I could probably drop some of my wrapper
> script and move more of it to the config file if I was confident in the
> syntax / sections. Maybe that needs to be documented?
>
> > > > ...although I don't see the advantage of this approach over the series
> > > > I sent. Basically, the build should fail if there are missing blobs.
> > > > The only question is whether we want to handle missing blobs by:
> > > >
> > > > 1. failing immediately when we see the first missing blob (your approach)
> > > > 2. failing at the end after all binman processing is done (my approach)
> > > >
> > > > The nice thing about 2 is that the build does complete and the exit
> > > > code lets buildman decide what to do afterwards (i.e. we can make
> > > > missing blobs be an error or a warning).
> > > >
> > > > Option 1 has the benefit that we don't do any of the blob handling, so
> > > > it just dies right away. Perhaps this is a philosophical question, but
> > > > it is a little subtle and I'm not actually sure people would notice
> > > > the difference so long as they get the errors as expected.
> > >
> > > The way I'm thinking of it is there's two cases. The first case is
> > > someone who is testing on the hardware that requires these files. Stop
> > > ASAP because they either will know they forgot to pass X/Y/Z files or
> > > they'll re-read the board instructions page and see oh, they need to
> > > grab binaries X/Y/Z too. Waiting to collect up missing file errors
> > > doesn't save time. It's also still hugely likely I believe that they're
> > > using make and not buildman.
> > >
> > > The second case is someone building multiple boards at once to
> > > build-check (but not run-time check) something in multiple places. This
> > > is where passing a specific failure exit code can be helpful, yes.
> >
> > Yes that makes sense.
> >
> > >
> > > > BTW we need a one-character flag if we do this as it will be a very
> > > > common requirement.
> > >
> > > Sadly both -a and -A are taken. We could use -M for Missing ?
> >
> > SGTM.
> >
> > If we apply some of the patches in my series then I think I could
> > enable that always, since the build will return an exit code if a blob
> > is missing, which I can disable with -W.
>
> I think so?
>
> > > > > > But having to provide a flag to do build testing seems like the tail
> > > > > > is wagging the dog. Boards should be discouraged to use blobs and we
> > > > > > don't want to make it hard for everyone else (who doesn't have the
> > > > > > blobs) to test whether their patch breaks a build.
> > > > >
> > > > > I'm not sure this ends up being a common case. If someone is build
> > > > > testing for something they can't run test, the error is going to be
> > > > > after whatever they compile-tested was compiled/linked, and if they're
> > > > > testing everything it's via CI.
> > > >
> > > > I don't think people will be able to tell that. The 'make' fails with
> > > > an error so it looks like a failure. It happens to be at the end in
> > > > the binman step, but it is still a build failure.
> > > >
> > > > My overall concern is constantly having to rerun a build because I
> > > > forgot to add a flag, when I don't have the blobs for the boards
> > > > anyway. Like the LTO thing that cost 4 seconds on every build, that
> > > > could get really annoying. Blobs should not get in the way of
> > > > development.
> > >
> > > The counter problem is this isn't the first time someone has come up and
> > > noted how much time they've wasted because we defaulted to fake
> > > binaries. I think we've optimized too much for the people that build a
> > > thousand configs all the time (us) instead of the people that build for
> > > 1 or two configs at a time (most people?).
> > >
> > > To that end, I am really curious what Rasmus has to say here, or anyone
> > > else that has a different workflow from you and I.
> >
> > The main problem I think is that 'binman' should have emitted an exit
> > code if there are missing blobs. That is [1] and I think was just an
> > oversight on my part, or not thinking through things clearly enough.
>
> OK, can you take / use as inspiration what you think makes sense of my
> series as well and v2 your series? I want to grab that "N:" patch asap
> since that unblocks a bunch of other platforms that are ready to come
> in, otherwise.

Yes will do this by EOW.

Regards,
Simon