Hi Tom, On Thu, 12 Sept 2024 at 19:44, Tom Rini <tr...@konsulko.com> wrote: > > On Wed, Sep 11, 2024 at 07:01:37PM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Tue, 10 Sept 2024 at 16:07, Tom Rini <tr...@konsulko.com> wrote: > > > > > > 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 <tr...@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 <tr...@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 <tr...@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 <tr...@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 > > > > > > > > > > > > <s...@chromium.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tom, > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, 16 Aug 2024 at 11:22, Tom Rini > > > > > > > > > > > > > <tr...@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 <s...@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? > > > > OK thank you. Perhaps we should file an issue for that? I am trying to > > do one issue a week, although I have failed the last few weeks. > > Can't hurt, yeah.
https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/24 https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/25 Regards, Simon
