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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 7336 bytes --]

On Tue, Sep 10, 2024 at 02:14:35PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 10 Sept 2024 at 12:52, Tom Rini <trini@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@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@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@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@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini <trini@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@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.

Thanks for explaining. Whenever you want to pick this one up, go ahead
and send it along then. And I guess at some point we should look at (a)
re-syncing our libfdt code and (b) switching to using system dtc by
default?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]