[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 at 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 at 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 at prevas.dk>
> > > Cc: Simon Glass <sjg at chromium.org>
> > > Signed-off-by: Tom Rini <trini at 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