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

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/