[PATCH v3 2/3] buildman: Allow skipping the dtc build

[Simon Glass]

Hi Tom,

On Tue, 10 Sept 2024 at 12:52, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Sep 10, 2024 at 12:45:39PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 10 Sept 2024 at 12:42, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Tue, Sep 10, 2024 at 12:41:11PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 2 Sept 2024 at 09:39, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Sun, Sep 01, 2024 at 02:09:39PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Thu, 22 Aug 2024 at 08:10, Tom Rini <trini at konsulko.com> wrote:
> > > > > > >
> > > > > > > On Wed, Aug 21, 2024 at 09:00:25PM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Fri, 16 Aug 2024 at 17:53, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Tom,
> > > > > > > > >
> > > > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Thu, Aug 15, 2024 at 01:57:45PM -0600, Simon Glass wrote:
> > > > > > > > > >
> > > > > > > > > > > For most boards, the device-tree compiler is built in-tree, ignoring the
> > > > > > > > > > > system version. Add a special option to skip this build. This can be
> > > > > > > > > > > useful when the system dtc is up-to-date, as it speeds up the build.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > (no changes since v1)
> > > > > > > > > > >
> > > > > > > > > > >  tools/buildman/builder.py       | 27 ++++++++++++++++++++++++++-
> > > > > > > > > > >  tools/buildman/builderthread.py |  4 ++--
> > > > > > > > > > >  tools/buildman/buildman.rst     |  3 +++
> > > > > > > > > > >  tools/buildman/cmdline.py       |  2 ++
> > > > > > > > > > >  tools/buildman/control.py       |  3 ++-
> > > > > > > > > > >  tools/buildman/test.py          | 31 +++++++++++++++++++++++++++++++
> > > > > > > > > > >  6 files changed, 66 insertions(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > > We should probably do this more generically, outside of buildman. We
> > > > > > > > > > have scripts/dtc-version.sh and if the system version isn't new enough
> > > > > > > > > > (and we just need to define whatever the minimum version is), then we
> > > > > > > > > > build our (not currently that new anymore) dtc instead.
> > > > > > > > >
> > > > > > > > > Yes I think I did a patch for that ages ago [1], but it was rejected.
> > > > > > > > >
> > > > > > > > > I'd be very happy for it to be applied as I think it is a better
> > > > > > > > > solution than this one.
> > > > > > > > >
> > > > > > > > > I see that some poor sod tried to do this in Linux this morning.
> > > > > > > >
> > > > > > > > Any thoughts on that patch?
> > > > > > >
> > > > > > > I'm open to re-considering [1] again, but we need to handle the warning
> > > > > > > problem first. That means...
> > > > > > >
> > > > > > > > Also I do see one problem. Newer dtc version produce a lot of
> > > > > > > > warnings, which causes CI to fail. So if we always use the newest
> > > > > > > > version, people are going to see a ton of warnings when they run
> > > > > > > > locally. Am I missing something here?
> > > > > > >
> > > > > > > Well, it would be great to get our Kbuild logic anywhere close to
> > > > > > > in-sync again with upstream. But syncing up the disabling warning flags
> > > > > > > shouldn't be too hard.
> > > > > >
> > > > > > So, coming back to this patch, the nice thing about it is that it is
> > > > > > deterministic. So people who build U-Boot and don't want funky
> > > > > > behaviour will be happy. It will use the internal dtc by default. To
> > > > > > use the external one, you must provide an option.
> > > > > >
> > > > > > This patch only affects buildman, but as you can see the mechanism it
> > > > > > uses is to set the DTC variable, which people can do without buildman.
> > > > > > It's just a convenience, but useful enough to have a flag, I believe.
> > > > >
> > > > > Wait, that's right, we have DTC as a thing that can be set in the
> > > > > environment, so why do we need something for buildman at all?
> > > >
> > > > It's a convenience. I have found that it speeds things up quite a bit,
> > > > so I want to enable it most of the time. But we can't do it by default
> > > > and need it to be optional, I believe.
> > >
> > > I wasn't clear, sorry. Why is:
> > > $ export DTC=/usr/local/bin/dtc
> > > $ tools/buildman/buildman ...
> > >
> > > Enough?
> >
> > My patch is more equivalent to:
> >
> > DTC=`which dtc` buildman...
> >
> > As I said it is a convenience which I want to use all the time,
> > including in CI. Are you worried about changing buildman? What is the
> > issue here?
>
> My question / concern is why do we need:
>
> >  6 files changed, 66 insertions(+), 4 deletions(-)
>
> When we can do it with zero code changes?

We don't need anything here...but this helps with my productivity as
and can add this flag easily when doing large builds.

Note that half of those lines are a test and another half are just
plumbing it through all the layers. Any new flag will end up with
this, even if it is a few lines of 'real' code.

> And then similar to the
> worries I set aside with the "venv" related changes,

That change does fix a bug. Without the change, we cannot run code
coverage in CI, which I very-much want to turn on once Marek can
figure out the missing Binman tests.

> you're making one
> tool be clever and work as exactly as expected (but perhaps in a "do
> what I meant, not what I said" manner?) way.

The only magic is that it locates dtc on the path, but that is the
thing that I find valuable.

> If we have "set DTC in
> environment, it's always obeyed, can be passed directly to make" but
> also "pass buildman a flag to say where dtc is" why do we need the
> latter if it should already be working, and I think already is working?

The flag doesn't tell buildman where dtc is, though. It tells buildman
to hunt for it and use that in preference to the internal build. I am
not suggesting adding anything non-deterministics.

A general point here...buildman is pretty stable but it is not perfect
and we continue to tweak it to help make it a better tool. When it is
perfect, I will certainly stop sending patches for it. I normally only
send a patch when a bug is reported or something annoys me. The dtc
thing annoys me *a lot* because it chews up ~20% of my CPU for no
purpose. I had not realised how bad this problem was.

Regards,
Simon