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

[Tom Rini]

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 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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20221012/7bd71d6c/attachment.sig>