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 <tr...@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 <tr...@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 <tr...@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.villem...@prevas.dk>
> > > > > > Cc: Simon Glass <s...@chromium.org>
> > > > > > Signed-off-by: Tom Rini <tr...@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
signature.asc
Description: PGP signature
