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

[Tom Rini]

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

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
-------------- 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/20221014/3d9fb3a4/attachment.sig>