[v2,2/2] Makefile: Only build dtc if needed

Message ID 20200427002929.239379-2-sjg@chromium.org
State New
Headers show
Series
  • [v2,1/2] Revert "kbuild: remove unused dtc-version.sh script"
Related show

Commit Message

Simon Glass April 27, 2020, 12:29 a.m.
At present U-Boot always builds dtc if CONFIG_OF_CONTROL is defined. This
is wasteful when the system already has a suitable version available.

Update the Makefile logic to build dtc only if the version available is
too old.

This saves about 2.5 seconds of elapsed time on a clean build for me.

- Add a patch to bring back the dtc-version.sh script
- Update the check to make sure libfdt is available if needed

Signed-off-by: Simon Glass <sjg at chromium.org>
---

 Makefile               | 21 ++++++++++++++++++---
 dts/Kconfig            |  4 ----
 scripts/Kbuild.include |  5 ++++-
 scripts/Makefile       |  1 -
 scripts/dtc-version.sh | 36 +++++++++++++++++++++++++++++++-----
 5 files changed, 53 insertions(+), 14 deletions(-)

Comments

Heinrich Schuchardt April 27, 2020, 12:58 a.m. | #1
Am April 27, 2020 12:29:29 AM UTC schrieb Simon Glass <sjg at chromium.org>:
>At present U-Boot always builds dtc if CONFIG_OF_CONTROL is defined.
>This
>is wasteful when the system already has a suitable version available.
>
>Update the Makefile logic to build dtc only if the version available is
>too old.
>
>This saves about 2.5 seconds of elapsed time on a clean build for me.
>
>- Add a patch to bring back the dtc-version.sh script
>- Update the check to make sure libfdt is available if needed

U -Boot has been set up to create reproducible builds. With this patch dtc will have to be made a build dependency to provide reproducibility. Cf. https://www.debian.org/doc/debian-policy/ch-source.html#reproducibility

This may require changes in the packaging of U-Boot in Linux distributions. Nothing to stop this patch, just something to keep in mind.

You presume that future versions of dtc will always be backward compatible with  U-Boot. Ok, we do the same for other tools like gcc too (with surprises at each new major release).

Cc: Vagrant

Best regards

Heinrich

>
>Signed-off-by: Simon Glass <sjg at chromium.org>
>---
>
> Makefile               | 21 ++++++++++++++++++---
> dts/Kconfig            |  4 ----
> scripts/Kbuild.include |  5 ++++-
> scripts/Makefile       |  1 -
> scripts/dtc-version.sh | 36 +++++++++++++++++++++++++++++++-----
> 5 files changed, 53 insertions(+), 14 deletions(-)
>
>diff --git a/Makefile b/Makefile
>index b8a4b5058a..90cb83ed32 100644
>--- a/Makefile
>+++ b/Makefile
>@@ -410,7 +410,12 @@ PERL		= perl
> PYTHON		?= python
> PYTHON2		= python2
> PYTHON3		= python3
>-DTC		?= $(objtree)/scripts/dtc/dtc
>+
>+# DTC is automatically built if the version of $(DTC) is older that
>needed.
>+# Use the system dtc if it is new enough.
>+DTC		?= dtc
>+DTC_MIN_VERSION	:= 010406
>+
> CHECK		= sparse
> 
> CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
>@@ -1797,12 +1802,12 @@ include/config/uboot.release:
>include/config/auto.conf FORCE
> # version.h and scripts_basic is processed / created.
> 
> # Listed in dependency order
>-PHONY += prepare archprepare prepare0 prepare1 prepare2 prepare3
>+PHONY += prepare archprepare prepare0 prepare1 prepare2 prepare3
>prepare4
> 
># prepare3 is used to check if we are building in a separate output
>directory,
> # and if so do:
># 1) Check that make has not been executed in the kernel src $(srctree)
>-prepare3: include/config/uboot.release
>+prepare4: include/config/uboot.release
> ifneq ($(KBUILD_SRC),)
> 	@$(kecho) '  Using $(srctree) as source for U-Boot'
>	$(Q)if [ -f $(srctree)/.config -o -d $(srctree)/include/config ]; then
>\
>@@ -1812,6 +1817,14 @@ ifneq ($(KBUILD_SRC),)
> 	fi;
> endif
> 
>+# Checks for dtc and builds it if needed
>+prepare3: prepare4
>+	$(eval DTC := $(call
>dtc-version,010406,$(build_dtc),$(CONFIG_PYLIBFDT)))
>+	echo here $(DTC) $(build_dtc)
>+	if test "$(DTC)" = "$(build_dtc)"; then \
>+		$(MAKE) $(build)=scripts/dtc; \
>+	fi
>+
> # prepare2 creates a makefile if using a separate output directory
> prepare2: prepare3 outputmakefile cfg
> 
>@@ -1963,6 +1976,8 @@ SYSTEM_MAP = \
> System.map:	u-boot
> 		@$(call SYSTEM_MAP,$<) > $@
> 
>+build_dtc	:= $(objtree)/scripts/dtc/dtc
>+
>#########################################################################
> 
> # ARM relocations should all be R_ARM_RELATIVE (32-bit) or
>diff --git a/dts/Kconfig b/dts/Kconfig
>index 046a54a173..f8b808606c 100644
>--- a/dts/Kconfig
>+++ b/dts/Kconfig
>@@ -5,9 +5,6 @@
> config SUPPORT_OF_CONTROL
> 	bool
> 
>-config DTC
>-	bool
>-
> config PYLIBFDT
> 	bool
> 
>@@ -24,7 +21,6 @@ menu "Device Tree Control"
> 
> config OF_CONTROL
> 	bool "Run-time configuration via Device Tree"
>-	select DTC
> 	select OF_LIBFDT if !OF_PLATDATA
> 	help
> 	  This feature provides for run-time configuration of U-Boot
>diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
>index b34dedade7..8c167cef2d 100644
>--- a/scripts/Kbuild.include
>+++ b/scripts/Kbuild.include
>@@ -151,8 +151,11 @@ cc-fullversion = $(shell $(CONFIG_SHELL) \
>cc-ifversion = $(shell [ $(cc-version) $(1) $(2) ] && echo $(3) || echo
>$(4))
> 
> # added for U-Boot
>+# $1: min_version
>+# 32: build_dtc
>+# $3: need_pylibfdt
>binutils-version = $(shell $(CONFIG_SHELL)
>$(srctree)/scripts/binutils-version.sh $(AS))
>-dtc-version = $(shell $(CONFIG_SHELL)
>$(srctree)/scripts/dtc-version.sh $(DTC))
>+dtc-version = $(shell $(CONFIG_SHELL)
>$(srctree)/scripts/dtc-version.sh $(DTC) $1 $2 $3)
> 
> # cc-ldoption
> # Usage: ldflags += $(call cc-ldoption, -Wl$(comma)--hash-style=both)
>diff --git a/scripts/Makefile b/scripts/Makefile
>index e7b353f77f..cfe9fef804 100644
>--- a/scripts/Makefile
>+++ b/scripts/Makefile
>@@ -10,4 +10,3 @@ always		:= $(hostprogs-y)
> 
> # Let clean descend into subdirs
> subdir-	+= basic kconfig
>-subdir-$(CONFIG_DTC)	+= dtc
>diff --git a/scripts/dtc-version.sh b/scripts/dtc-version.sh
>index 0744c39eb0..75ba82830d 100755
>--- a/scripts/dtc-version.sh
>+++ b/scripts/dtc-version.sh
>@@ -1,12 +1,26 @@
> #!/bin/sh
> #
>-# dtc-version dtc-command
>+# dtc-version dtc_command min_version build_dtc need_pylibfdt
> #
>-# Prints the dtc version of `dtc-command' in a canonical 6-digit form
>-# such as `010404'  for dtc 1.4.4
>+# Selects which version of dtc to use
>+#
>+# If need_pylibfdt is non-empty then the script first checks that the
>Python
>+# libfdt library is available. If not it outputs $build_dtc and exits
>+#
>+# Otherwise, if the version of dtc_command is < min_version, prints
>build_dtc
>+# else prints dtc_command. The min_version is in the format MMmmpp
>where:
>+#
>+#   MM is the major version
>+#   mm is the minor version
>+#   pp is the patch level
>+#
>+# For example 010406 means 1.4.6
> #
> 
>-dtc="$*"
>+dtc="$1"
>+min_version="$2"
>+build_dtc="$3"
>+need_pylibfdt="$4"
> 
> if [ ${#dtc} -eq 0 ]; then
> 	echo "Error: No dtc command specified."
>@@ -14,8 +28,20 @@ if [ ${#dtc} -eq 0 ]; then
> 	exit 1
> fi
> 
>+if [ -n "${need_pylibfdt}" ]; then
>+	if ! echo "import libfdt" | python3 2>/dev/null; then
>+		echo $build_dtc
>+		exit 0
>+	fi
>+fi
>+
> MAJOR=$($dtc -v | head -1 | awk '{print $NF}' | cut -d . -f 1)
> MINOR=$($dtc -v | head -1 | awk '{print $NF}' | cut -d . -f 2)
>PATCH=$($dtc -v | head -1 | awk '{print $NF}' | cut -d . -f 3 | cut -d
>- -f 1)
> 
>-printf "%02d%02d%02d\\n" $MAJOR $MINOR $PATCH
>+version="$(printf "%02d%02d%02d" $MAJOR $MINOR $PATCH)"
>+if test $version -lt $min_version; then \
>+	echo $build_dtc
>+else
>+	echo $dtc
>+fi
Simon Glass April 27, 2020, 10:25 p.m. | #2
Hi Heinrich,

On Sun, 26 Apr 2020 at 18:58, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Am April 27, 2020 12:29:29 AM UTC schrieb Simon Glass <sjg at chromium.org>:
> >At present U-Boot always builds dtc if CONFIG_OF_CONTROL is defined.
> >This
> >is wasteful when the system already has a suitable version available.
> >
> >Update the Makefile logic to build dtc only if the version available is
> >too old.
> >
> >This saves about 2.5 seconds of elapsed time on a clean build for me.
> >
> >- Add a patch to bring back the dtc-version.sh script
> >- Update the check to make sure libfdt is available if needed
>
> U -Boot has been set up to create reproducible builds. With this patch dtc will have to be made a build dependency to provide reproducibility. Cf. https://www.debian.org/doc/debian-policy/ch-source.html#reproducibility
>
> This may require changes in the packaging of U-Boot in Linux distributions. Nothing to stop this patch, just something to keep in mind.
>
> You presume that future versions of dtc will always be backward compatible with  U-Boot. Ok, we do the same for other tools like gcc too (with surprises at each new major release).
>
> Cc: Vagrant

Should we disable this check (and always build dtc) when doing a
repeatable build? Is that SOURCE_DATE_EPOCH?

Regards,
SImon


Regards,
Simon
Vagrant Cascadian April 27, 2020, 11:10 p.m. | #3
On 2020-04-27, Simon Glass wrote:
> On Sun, 26 Apr 2020 at 18:58, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> Am April 27, 2020 12:29:29 AM UTC schrieb Simon Glass <sjg at chromium.org>:
>> >At present U-Boot always builds dtc if CONFIG_OF_CONTROL is defined.
>> >This
>> >is wasteful when the system already has a suitable version available.
>> >
>> >Update the Makefile logic to build dtc only if the version available is
>> >too old.
>> >
>> >This saves about 2.5 seconds of elapsed time on a clean build for me.
>> >
>> >- Add a patch to bring back the dtc-version.sh script
>> >- Update the check to make sure libfdt is available if needed
>>
>> U -Boot has been set up to create reproducible builds. With this
>> patch dtc will have to be made a build dependency to provide
>> reproducibility. Cf. https://www.debian.org/doc/debian-policy/ch-source.html#reproducibility
>>
>> This may require changes in the packaging of U-Boot in Linux
>> distributions. Nothing to stop this patch, just something to keep in
>> mind.
>>
>> You presume that future versions of dtc will always be backward
>> compatible with U-Boot. Ok, we do the same for other tools like gcc
>> too (with surprises at each new major release).

In general when packaging for Debian, the preference is to not use
embedded code copies if at all possible. This does require paying
attention to backwards and forwards compatibility issues a bit.

A simple example: The security team in Debian generally likes to fix a
problem in a single source package, rather than an arbitrary number of
source packages that happen to share some embedded copy of the code from
who knows when...

So at least from my perspective, I'd be happy to use the Debian packaged
dtc (a.k.a. device-tree-compiler), rather than the one embedded in
u-boot sources.

Silently switching to the embedded copy sounds a little scary; I would
prefer for that to be explicit... a build flag to specify one way or the
other and failing is better that being too clever about autodetecting.


> Should we disable this check (and always build dtc) when doing a
> repeatable build? Is that SOURCE_DATE_EPOCH?

And with my Reproducible Builds hat on, builds would ideally *always* be
reproducible, given the same sources and toolchain... several
distributions and software projects provide information sufficient to
reproduce the build environment:

  https://reproducible-builds.org/docs/recording/


While SOURCE_DATE_EPOCH is definitely one sign that the builder is
explicitly attempting to be reproducible; It's a bit of a kludge to try
and be more reproducible just because SOURCE_DATE_EPOCH is
set. SOURCE_DATE_EPOCH should really only affect the behavior of date or
time related things; even better would be to not embded time related
information at all!


live well,
  vagrant
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200427/c021980f/attachment.sig>
Tom Rini April 28, 2020, 2:19 p.m. | #4
On Mon, Apr 27, 2020 at 04:10:06PM -0700, Vagrant Cascadian wrote:
> On 2020-04-27, Simon Glass wrote:
> > On Sun, 26 Apr 2020 at 18:58, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> Am April 27, 2020 12:29:29 AM UTC schrieb Simon Glass <sjg at chromium.org>:
> >> >At present U-Boot always builds dtc if CONFIG_OF_CONTROL is defined.
> >> >This
> >> >is wasteful when the system already has a suitable version available.
> >> >
> >> >Update the Makefile logic to build dtc only if the version available is
> >> >too old.
> >> >
> >> >This saves about 2.5 seconds of elapsed time on a clean build for me.
> >> >
> >> >- Add a patch to bring back the dtc-version.sh script
> >> >- Update the check to make sure libfdt is available if needed
> >>
> >> U -Boot has been set up to create reproducible builds. With this
> >> patch dtc will have to be made a build dependency to provide
> >> reproducibility. Cf. https://www.debian.org/doc/debian-policy/ch-source.html#reproducibility
> >>
> >> This may require changes in the packaging of U-Boot in Linux
> >> distributions. Nothing to stop this patch, just something to keep in
> >> mind.
> >>
> >> You presume that future versions of dtc will always be backward
> >> compatible with U-Boot. Ok, we do the same for other tools like gcc
> >> too (with surprises at each new major release).
> 
> In general when packaging for Debian, the preference is to not use
> embedded code copies if at all possible. This does require paying
> attention to backwards and forwards compatibility issues a bit.
> 
> A simple example: The security team in Debian generally likes to fix a
> problem in a single source package, rather than an arbitrary number of
> source packages that happen to share some embedded copy of the code from
> who knows when...
> 
> So at least from my perspective, I'd be happy to use the Debian packaged
> dtc (a.k.a. device-tree-compiler), rather than the one embedded in
> u-boot sources.
> 
> Silently switching to the embedded copy sounds a little scary; I would
> prefer for that to be explicit... a build flag to specify one way or the
> other and failing is better that being too clever about autodetecting.
> 
> 
> > Should we disable this check (and always build dtc) when doing a
> > repeatable build? Is that SOURCE_DATE_EPOCH?
> 
> And with my Reproducible Builds hat on, builds would ideally *always* be
> reproducible, given the same sources and toolchain... several
> distributions and software projects provide information sufficient to
> reproduce the build environment:
> 
>   https://reproducible-builds.org/docs/recording/
> 
> 
> While SOURCE_DATE_EPOCH is definitely one sign that the builder is
> explicitly attempting to be reproducible; It's a bit of a kludge to try
> and be more reproducible just because SOURCE_DATE_EPOCH is
> set. SOURCE_DATE_EPOCH should really only affect the behavior of date or
> time related things; even better would be to not embded time related

This is probably one of those cases where we should just continue to act
like the linux kernel and always use and build our own copy of dtc.
Then, when someone convinces the kernel folks to change their ways, we
can adopt that.
Simon Glass April 28, 2020, 3:41 p.m. | #5
Hi Tom.

On Tue, 28 Apr 2020 at 08:19, Tom Rini <trini at konsulko.com> wrote:
>
> On Mon, Apr 27, 2020 at 04:10:06PM -0700, Vagrant Cascadian wrote:
> > On 2020-04-27, Simon Glass wrote:
> > > On Sun, 26 Apr 2020 at 18:58, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > >>
> > >> Am April 27, 2020 12:29:29 AM UTC schrieb Simon Glass <sjg at chromium.org>:
> > >> >At present U-Boot always builds dtc if CONFIG_OF_CONTROL is defined.
> > >> >This
> > >> >is wasteful when the system already has a suitable version available.
> > >> >
> > >> >Update the Makefile logic to build dtc only if the version available is
> > >> >too old.
> > >> >
> > >> >This saves about 2.5 seconds of elapsed time on a clean build for me.
> > >> >
> > >> >- Add a patch to bring back the dtc-version.sh script
> > >> >- Update the check to make sure libfdt is available if needed
> > >>
> > >> U -Boot has been set up to create reproducible builds. With this
> > >> patch dtc will have to be made a build dependency to provide
> > >> reproducibility. Cf. https://www.debian.org/doc/debian-policy/ch-source.html#reproducibility
> > >>
> > >> This may require changes in the packaging of U-Boot in Linux
> > >> distributions. Nothing to stop this patch, just something to keep in
> > >> mind.
> > >>
> > >> You presume that future versions of dtc will always be backward
> > >> compatible with U-Boot. Ok, we do the same for other tools like gcc
> > >> too (with surprises at each new major release).
> >
> > In general when packaging for Debian, the preference is to not use
> > embedded code copies if at all possible. This does require paying
> > attention to backwards and forwards compatibility issues a bit.
> >
> > A simple example: The security team in Debian generally likes to fix a
> > problem in a single source package, rather than an arbitrary number of
> > source packages that happen to share some embedded copy of the code from
> > who knows when...
> >
> > So at least from my perspective, I'd be happy to use the Debian packaged
> > dtc (a.k.a. device-tree-compiler), rather than the one embedded in
> > u-boot sources.
> >
> > Silently switching to the embedded copy sounds a little scary; I would
> > prefer for that to be explicit... a build flag to specify one way or the
> > other and failing is better that being too clever about autodetecting.
> >
> >
> > > Should we disable this check (and always build dtc) when doing a
> > > repeatable build? Is that SOURCE_DATE_EPOCH?
> >
> > And with my Reproducible Builds hat on, builds would ideally *always* be
> > reproducible, given the same sources and toolchain... several
> > distributions and software projects provide information sufficient to
> > reproduce the build environment:
> >
> >   https://reproducible-builds.org/docs/recording/
> >
> >
> > While SOURCE_DATE_EPOCH is definitely one sign that the builder is
> > explicitly attempting to be reproducible; It's a bit of a kludge to try
> > and be more reproducible just because SOURCE_DATE_EPOCH is
> > set. SOURCE_DATE_EPOCH should really only affect the behavior of date or
> > time related things; even better would be to not embded time related
>
> This is probably one of those cases where we should just continue to act
> like the linux kernel and always use and build our own copy of dtc.
> Then, when someone convinces the kernel folks to change their ways, we
> can adopt that.

It seems that Vagrant wants to use the system dtc by default and
require an explicit option to use the in-built dtc. I don't think that
would work well for most users though.

It is in my view somewhat mad to build dtc for every one of 1400
boards as I do today when running buildman.

Having said that it doesn't seem like we can come up with a default
behaviour that makes everyone happy, so the status quo is best.

But what about adding a build flag / envvar to select between:

- Use system default
- Build internal version
- Use system default if new enough, else build

Then we can satisfy distros as well as speed up the build for those that care.

I haven't heard from Marek on this thread, actually.

Regards,
Simon
Tom Rini April 28, 2020, 3:52 p.m. | #6
On Tue, Apr 28, 2020 at 09:41:14AM -0600, Simon Glass wrote:
> Hi Tom.
> 
> On Tue, 28 Apr 2020 at 08:19, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Mon, Apr 27, 2020 at 04:10:06PM -0700, Vagrant Cascadian wrote:
> > > On 2020-04-27, Simon Glass wrote:
> > > > On Sun, 26 Apr 2020 at 18:58, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > >>
> > > >> Am April 27, 2020 12:29:29 AM UTC schrieb Simon Glass <sjg at chromium.org>:
> > > >> >At present U-Boot always builds dtc if CONFIG_OF_CONTROL is defined.
> > > >> >This
> > > >> >is wasteful when the system already has a suitable version available.
> > > >> >
> > > >> >Update the Makefile logic to build dtc only if the version available is
> > > >> >too old.
> > > >> >
> > > >> >This saves about 2.5 seconds of elapsed time on a clean build for me.
> > > >> >
> > > >> >- Add a patch to bring back the dtc-version.sh script
> > > >> >- Update the check to make sure libfdt is available if needed
> > > >>
> > > >> U -Boot has been set up to create reproducible builds. With this
> > > >> patch dtc will have to be made a build dependency to provide
> > > >> reproducibility. Cf. https://www.debian.org/doc/debian-policy/ch-source.html#reproducibility
> > > >>
> > > >> This may require changes in the packaging of U-Boot in Linux
> > > >> distributions. Nothing to stop this patch, just something to keep in
> > > >> mind.
> > > >>
> > > >> You presume that future versions of dtc will always be backward
> > > >> compatible with U-Boot. Ok, we do the same for other tools like gcc
> > > >> too (with surprises at each new major release).
> > >
> > > In general when packaging for Debian, the preference is to not use
> > > embedded code copies if at all possible. This does require paying
> > > attention to backwards and forwards compatibility issues a bit.
> > >
> > > A simple example: The security team in Debian generally likes to fix a
> > > problem in a single source package, rather than an arbitrary number of
> > > source packages that happen to share some embedded copy of the code from
> > > who knows when...
> > >
> > > So at least from my perspective, I'd be happy to use the Debian packaged
> > > dtc (a.k.a. device-tree-compiler), rather than the one embedded in
> > > u-boot sources.
> > >
> > > Silently switching to the embedded copy sounds a little scary; I would
> > > prefer for that to be explicit... a build flag to specify one way or the
> > > other and failing is better that being too clever about autodetecting.
> > >
> > >
> > > > Should we disable this check (and always build dtc) when doing a
> > > > repeatable build? Is that SOURCE_DATE_EPOCH?
> > >
> > > And with my Reproducible Builds hat on, builds would ideally *always* be
> > > reproducible, given the same sources and toolchain... several
> > > distributions and software projects provide information sufficient to
> > > reproduce the build environment:
> > >
> > >   https://reproducible-builds.org/docs/recording/
> > >
> > >
> > > While SOURCE_DATE_EPOCH is definitely one sign that the builder is
> > > explicitly attempting to be reproducible; It's a bit of a kludge to try
> > > and be more reproducible just because SOURCE_DATE_EPOCH is
> > > set. SOURCE_DATE_EPOCH should really only affect the behavior of date or
> > > time related things; even better would be to not embded time related
> >
> > This is probably one of those cases where we should just continue to act
> > like the linux kernel and always use and build our own copy of dtc.
> > Then, when someone convinces the kernel folks to change their ways, we
> > can adopt that.
> 
> It seems that Vagrant wants to use the system dtc by default and
> require an explicit option to use the in-built dtc. I don't think that
> would work well for most users though.

Right, and this is where I disagree and point to the kernel.  Get that
changed first.

> It is in my view somewhat mad to build dtc for every one of 1400
> boards as I do today when running buildman.

This is a different funny case.  Perhaps ccache could be helpful here?
I think the way it's used in OpenEmbedded, such that you have a cache
that's more local to what's building vs global cache, could be helpful
here too.  A ccache instance per CI job / world build could help.  A
flag to buildman to support that could help, yes?  Thanks!
Simon Glass April 28, 2020, 10:40 p.m. | #7
Hi Tom,

On Tue, 28 Apr 2020 at 09:52, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Apr 28, 2020 at 09:41:14AM -0600, Simon Glass wrote:
> > Hi Tom.
> >
> > On Tue, 28 Apr 2020 at 08:19, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Mon, Apr 27, 2020 at 04:10:06PM -0700, Vagrant Cascadian wrote:
> > > > On 2020-04-27, Simon Glass wrote:
> > > > > On Sun, 26 Apr 2020 at 18:58, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > > >>
> > > > >> Am April 27, 2020 12:29:29 AM UTC schrieb Simon Glass <sjg at chromium.org>:
> > > > >> >At present U-Boot always builds dtc if CONFIG_OF_CONTROL is defined.
> > > > >> >This
> > > > >> >is wasteful when the system already has a suitable version available.
> > > > >> >
> > > > >> >Update the Makefile logic to build dtc only if the version available is
> > > > >> >too old.
> > > > >> >
> > > > >> >This saves about 2.5 seconds of elapsed time on a clean build for me.
> > > > >> >
> > > > >> >- Add a patch to bring back the dtc-version.sh script
> > > > >> >- Update the check to make sure libfdt is available if needed
> > > > >>
> > > > >> U -Boot has been set up to create reproducible builds. With this
> > > > >> patch dtc will have to be made a build dependency to provide
> > > > >> reproducibility. Cf. https://www.debian.org/doc/debian-policy/ch-source.html#reproducibility
> > > > >>
> > > > >> This may require changes in the packaging of U-Boot in Linux
> > > > >> distributions. Nothing to stop this patch, just something to keep in
> > > > >> mind.
> > > > >>
> > > > >> You presume that future versions of dtc will always be backward
> > > > >> compatible with U-Boot. Ok, we do the same for other tools like gcc
> > > > >> too (with surprises at each new major release).
> > > >
> > > > In general when packaging for Debian, the preference is to not use
> > > > embedded code copies if at all possible. This does require paying
> > > > attention to backwards and forwards compatibility issues a bit.
> > > >
> > > > A simple example: The security team in Debian generally likes to fix a
> > > > problem in a single source package, rather than an arbitrary number of
> > > > source packages that happen to share some embedded copy of the code from
> > > > who knows when...
> > > >
> > > > So at least from my perspective, I'd be happy to use the Debian packaged
> > > > dtc (a.k.a. device-tree-compiler), rather than the one embedded in
> > > > u-boot sources.
> > > >
> > > > Silently switching to the embedded copy sounds a little scary; I would
> > > > prefer for that to be explicit... a build flag to specify one way or the
> > > > other and failing is better that being too clever about autodetecting.
> > > >
> > > >
> > > > > Should we disable this check (and always build dtc) when doing a
> > > > > repeatable build? Is that SOURCE_DATE_EPOCH?
> > > >
> > > > And with my Reproducible Builds hat on, builds would ideally *always* be
> > > > reproducible, given the same sources and toolchain... several
> > > > distributions and software projects provide information sufficient to
> > > > reproduce the build environment:
> > > >
> > > >   https://reproducible-builds.org/docs/recording/
> > > >
> > > >
> > > > While SOURCE_DATE_EPOCH is definitely one sign that the builder is
> > > > explicitly attempting to be reproducible; It's a bit of a kludge to try
> > > > and be more reproducible just because SOURCE_DATE_EPOCH is
> > > > set. SOURCE_DATE_EPOCH should really only affect the behavior of date or
> > > > time related things; even better would be to not embded time related
> > >
> > > This is probably one of those cases where we should just continue to act
> > > like the linux kernel and always use and build our own copy of dtc.
> > > Then, when someone convinces the kernel folks to change their ways, we
> > > can adopt that.
> >
> > It seems that Vagrant wants to use the system dtc by default and
> > require an explicit option to use the in-built dtc. I don't think that
> > would work well for most users though.
>
> Right, and this is where I disagree and point to the kernel.  Get that
> changed first.
>
> > It is in my view somewhat mad to build dtc for every one of 1400
> > boards as I do today when running buildman.
>
> This is a different funny case.  Perhaps ccache could be helpful here?
> I think the way it's used in OpenEmbedded, such that you have a cache
> that's more local to what's building vs global cache, could be helpful
> here too.  A ccache instance per CI job / world build could help.  A
> flag to buildman to support that could help, yes?  Thanks!

So not allow using the system dtc? Or are you OK with a build option?

The thing is I would prefer to avoid another level of cache for a
problem that only exists because of our kernel design decision.

As to changing the kernel, I cannot imagine that happening as they
change dtc all the time.

Regards,
Simon
Tom Rini April 30, 2020, 3:05 p.m. | #8
On Tue, Apr 28, 2020 at 04:40:47PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 28 Apr 2020 at 09:52, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Tue, Apr 28, 2020 at 09:41:14AM -0600, Simon Glass wrote:
> > > Hi Tom.
> > >
> > > On Tue, 28 Apr 2020 at 08:19, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Mon, Apr 27, 2020 at 04:10:06PM -0700, Vagrant Cascadian wrote:
> > > > > On 2020-04-27, Simon Glass wrote:
> > > > > > On Sun, 26 Apr 2020 at 18:58, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > > > >>
> > > > > >> Am April 27, 2020 12:29:29 AM UTC schrieb Simon Glass <sjg at chromium.org>:
> > > > > >> >At present U-Boot always builds dtc if CONFIG_OF_CONTROL is defined.
> > > > > >> >This
> > > > > >> >is wasteful when the system already has a suitable version available.
> > > > > >> >
> > > > > >> >Update the Makefile logic to build dtc only if the version available is
> > > > > >> >too old.
> > > > > >> >
> > > > > >> >This saves about 2.5 seconds of elapsed time on a clean build for me.
> > > > > >> >
> > > > > >> >- Add a patch to bring back the dtc-version.sh script
> > > > > >> >- Update the check to make sure libfdt is available if needed
> > > > > >>
> > > > > >> U -Boot has been set up to create reproducible builds. With this
> > > > > >> patch dtc will have to be made a build dependency to provide
> > > > > >> reproducibility. Cf. https://www.debian.org/doc/debian-policy/ch-source.html#reproducibility
> > > > > >>
> > > > > >> This may require changes in the packaging of U-Boot in Linux
> > > > > >> distributions. Nothing to stop this patch, just something to keep in
> > > > > >> mind.
> > > > > >>
> > > > > >> You presume that future versions of dtc will always be backward
> > > > > >> compatible with U-Boot. Ok, we do the same for other tools like gcc
> > > > > >> too (with surprises at each new major release).
> > > > >
> > > > > In general when packaging for Debian, the preference is to not use
> > > > > embedded code copies if at all possible. This does require paying
> > > > > attention to backwards and forwards compatibility issues a bit.
> > > > >
> > > > > A simple example: The security team in Debian generally likes to fix a
> > > > > problem in a single source package, rather than an arbitrary number of
> > > > > source packages that happen to share some embedded copy of the code from
> > > > > who knows when...
> > > > >
> > > > > So at least from my perspective, I'd be happy to use the Debian packaged
> > > > > dtc (a.k.a. device-tree-compiler), rather than the one embedded in
> > > > > u-boot sources.
> > > > >
> > > > > Silently switching to the embedded copy sounds a little scary; I would
> > > > > prefer for that to be explicit... a build flag to specify one way or the
> > > > > other and failing is better that being too clever about autodetecting.
> > > > >
> > > > >
> > > > > > Should we disable this check (and always build dtc) when doing a
> > > > > > repeatable build? Is that SOURCE_DATE_EPOCH?
> > > > >
> > > > > And with my Reproducible Builds hat on, builds would ideally *always* be
> > > > > reproducible, given the same sources and toolchain... several
> > > > > distributions and software projects provide information sufficient to
> > > > > reproduce the build environment:
> > > > >
> > > > >   https://reproducible-builds.org/docs/recording/
> > > > >
> > > > >
> > > > > While SOURCE_DATE_EPOCH is definitely one sign that the builder is
> > > > > explicitly attempting to be reproducible; It's a bit of a kludge to try
> > > > > and be more reproducible just because SOURCE_DATE_EPOCH is
> > > > > set. SOURCE_DATE_EPOCH should really only affect the behavior of date or
> > > > > time related things; even better would be to not embded time related
> > > >
> > > > This is probably one of those cases where we should just continue to act
> > > > like the linux kernel and always use and build our own copy of dtc.
> > > > Then, when someone convinces the kernel folks to change their ways, we
> > > > can adopt that.
> > >
> > > It seems that Vagrant wants to use the system dtc by default and
> > > require an explicit option to use the in-built dtc. I don't think that
> > > would work well for most users though.
> >
> > Right, and this is where I disagree and point to the kernel.  Get that
> > changed first.
> >
> > > It is in my view somewhat mad to build dtc for every one of 1400
> > > boards as I do today when running buildman.
> >
> > This is a different funny case.  Perhaps ccache could be helpful here?
> > I think the way it's used in OpenEmbedded, such that you have a cache
> > that's more local to what's building vs global cache, could be helpful
> > here too.  A ccache instance per CI job / world build could help.  A
> > flag to buildman to support that could help, yes?  Thanks!
> 
> So not allow using the system dtc? Or are you OK with a build option?

I'd rather not have a build option as that's going to encourage people
to use it, and then that'll lead to problems down the line.

> The thing is I would prefer to avoid another level of cache for a
> problem that only exists because of our kernel design decision.
> 
> As to changing the kernel, I cannot imagine that happening as they
> change dtc all the time.

We really need to stay better in sync with the kernel here too.  Trying
to get more syncs of kbuild/kconfig/dtc/etc with kernel releases is on
my list.
Simon Glass May 1, 2020, 4:04 a.m. | #9
Hi Tom,

On Thu, 30 Apr 2020 at 09:05, Tom Rini <trini at konsulko.com> wrote:

> On Tue, Apr 28, 2020 at 04:40:47PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 28 Apr 2020 at 09:52, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Tue, Apr 28, 2020 at 09:41:14AM -0600, Simon Glass wrote:
> > > > Hi Tom.
> > > >
> > > > On Tue, 28 Apr 2020 at 08:19, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Mon, Apr 27, 2020 at 04:10:06PM -0700, Vagrant Cascadian wrote:
> > > > > > On 2020-04-27, Simon Glass wrote:
> > > > > > > On Sun, 26 Apr 2020 at 18:58, Heinrich Schuchardt <
> xypron.glpk at gmx.de> wrote:
> > > > > > >>
> > > > > > >> Am April 27, 2020 12:29:29 AM UTC schrieb Simon Glass <
> sjg at chromium.org>:
> > > > > > >> >At present U-Boot always builds dtc if CONFIG_OF_CONTROL is
> defined.
> > > > > > >> >This
> > > > > > >> >is wasteful when the system already has a suitable version
> available.
> > > > > > >> >
> > > > > > >> >Update the Makefile logic to build dtc only if the version
> available is
> > > > > > >> >too old.
> > > > > > >> >
> > > > > > >> >This saves about 2.5 seconds of elapsed time on a clean
> build for me.
> > > > > > >> >
> > > > > > >> >- Add a patch to bring back the dtc-version.sh script
> > > > > > >> >- Update the check to make sure libfdt is available if needed
> > > > > > >>
> > > > > > >> U -Boot has been set up to create reproducible builds. With
> this
> > > > > > >> patch dtc will have to be made a build dependency to provide
> > > > > > >> reproducibility. Cf.
> https://www.debian.org/doc/debian-policy/ch-source.html#reproducibility
> > > > > > >>
> > > > > > >> This may require changes in the packaging of U-Boot in Linux
> > > > > > >> distributions. Nothing to stop this patch, just something to
> keep in
> > > > > > >> mind.
> > > > > > >>
> > > > > > >> You presume that future versions of dtc will always be
> backward
> > > > > > >> compatible with U-Boot. Ok, we do the same for other tools
> like gcc
> > > > > > >> too (with surprises at each new major release).
> > > > > >
> > > > > > In general when packaging for Debian, the preference is to not
> use
> > > > > > embedded code copies if at all possible. This does require paying
> > > > > > attention to backwards and forwards compatibility issues a bit.
> > > > > >
> > > > > > A simple example: The security team in Debian generally likes to
> fix a
> > > > > > problem in a single source package, rather than an arbitrary
> number of
> > > > > > source packages that happen to share some embedded copy of the
> code from
> > > > > > who knows when...
> > > > > >
> > > > > > So at least from my perspective, I'd be happy to use the Debian
> packaged
> > > > > > dtc (a.k.a. device-tree-compiler), rather than the one embedded
> in
> > > > > > u-boot sources.
> > > > > >
> > > > > > Silently switching to the embedded copy sounds a little scary; I
> would
> > > > > > prefer for that to be explicit... a build flag to specify one
> way or the
> > > > > > other and failing is better that being too clever about
> autodetecting.
> > > > > >
> > > > > >
> > > > > > > Should we disable this check (and always build dtc) when doing
> a
> > > > > > > repeatable build? Is that SOURCE_DATE_EPOCH?
> > > > > >
> > > > > > And with my Reproducible Builds hat on, builds would ideally
> *always* be
> > > > > > reproducible, given the same sources and toolchain... several
> > > > > > distributions and software projects provide information
> sufficient to
> > > > > > reproduce the build environment:
> > > > > >
> > > > > >   https://reproducible-builds.org/docs/recording/
> > > > > >
> > > > > >
> > > > > > While SOURCE_DATE_EPOCH is definitely one sign that the builder
> is
> > > > > > explicitly attempting to be reproducible; It's a bit of a kludge
> to try
> > > > > > and be more reproducible just because SOURCE_DATE_EPOCH is
> > > > > > set. SOURCE_DATE_EPOCH should really only affect the behavior of
> date or
> > > > > > time related things; even better would be to not embded time
> related
> > > > >
> > > > > This is probably one of those cases where we should just continue
> to act
> > > > > like the linux kernel and always use and build our own copy of dtc.
> > > > > Then, when someone convinces the kernel folks to change their
> ways, we
> > > > > can adopt that.
> > > >
> > > > It seems that Vagrant wants to use the system dtc by default and
> > > > require an explicit option to use the in-built dtc. I don't think
> that
> > > > would work well for most users though.
> > >
> > > Right, and this is where I disagree and point to the kernel.  Get that
> > > changed first.
> > >
> > > > It is in my view somewhat mad to build dtc for every one of 1400
> > > > boards as I do today when running buildman.
> > >
> > > This is a different funny case.  Perhaps ccache could be helpful here?
> > > I think the way it's used in OpenEmbedded, such that you have a cache
> > > that's more local to what's building vs global cache, could be helpful
> > > here too.  A ccache instance per CI job / world build could help.  A
> > > flag to buildman to support that could help, yes?  Thanks!
> >
> > So not allow using the system dtc? Or are you OK with a build option?
>
> I'd rather not have a build option as that's going to encourage people
> to use it, and then that'll lead to problems down the line.
>

OK, let's drop this patch then. If I can think of some other way, I'll let
you know.


>
> > The thing is I would prefer to avoid another level of cache for a
> > problem that only exists because of our kernel design decision.
> >
> > As to changing the kernel, I cannot imagine that happening as they
> > change dtc all the time.
>
> We really need to stay better in sync with the kernel here too.  Trying
> to get more syncs of kbuild/kconfig/dtc/etc with kernel releases is on
> my list.
>
> --
> Tom
>
-------------- next part --------------
-----BEGIN PGP SIGNATURE-----

iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAl6q6TsACgkQFHw5/5Y0
tyweFwv/Yq/gxrCtv21GHYlvjuYf5Aj1WuF1aiciQo8VpQuBpG2XpXT9ZVBDsoXH
3lTbKWbziBvREJ9kU9LL+dXS+fDwX66qwjFItGeYgOLE+ejuL1bkOcpBmdgKDrkH
MzDIv9W22yNRbh42C3v5OB6+94D4QLbmtCTcSC8iOvIaERmTp1sI6RIat7X6I4yL
VYZAbFKxm6gsSD+sNOGDLo3aQk48h01bNI52egJNXDzedim7xBdob7zgFkePMTSH
g1Pny/fQ9jVqMJAlMPvV0Dp1JAeldiCrNjWF06tR1PNZRyX4wVo+R5NDJL8dRfZr
PRypdsrI0uxbgdkmQykBf1KhgnsAqo28rrc+Wv7hvmAtSXpd7DDaaNB7dCbn53gz
j5XlkBChYH5ufub6+bjA85kGNBEdXXgxjuPqunLiN7uhO7Wve8DAu052l7aLR8Y9
zOpcQnVplyYkWdbFqP6vJMtV6lkLF89rELeIdJ5ifUjig6zdIvoyhuLOUD6g+aS6
aMRpRT8T
=Ropl
-----END PGP SIGNATURE-----

Patch

diff --git a/Makefile b/Makefile
index b8a4b5058a..90cb83ed32 100644
--- a/Makefile
+++ b/Makefile
@@ -410,7 +410,12 @@  PERL		= perl
 PYTHON		?= python
 PYTHON2		= python2
 PYTHON3		= python3
-DTC		?= $(objtree)/scripts/dtc/dtc
+
+# DTC is automatically built if the version of $(DTC) is older that needed.
+# Use the system dtc if it is new enough.
+DTC		?= dtc
+DTC_MIN_VERSION	:= 010406
+
 CHECK		= sparse
 
 CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
@@ -1797,12 +1802,12 @@  include/config/uboot.release: include/config/auto.conf FORCE
 # version.h and scripts_basic is processed / created.
 
 # Listed in dependency order
-PHONY += prepare archprepare prepare0 prepare1 prepare2 prepare3
+PHONY += prepare archprepare prepare0 prepare1 prepare2 prepare3 prepare4
 
 # prepare3 is used to check if we are building in a separate output directory,
 # and if so do:
 # 1) Check that make has not been executed in the kernel src $(srctree)
-prepare3: include/config/uboot.release
+prepare4: include/config/uboot.release
 ifneq ($(KBUILD_SRC),)
 	@$(kecho) '  Using $(srctree) as source for U-Boot'
 	$(Q)if [ -f $(srctree)/.config -o -d $(srctree)/include/config ]; then \
@@ -1812,6 +1817,14 @@  ifneq ($(KBUILD_SRC),)
 	fi;
 endif
 
+# Checks for dtc and builds it if needed
+prepare3: prepare4
+	$(eval DTC := $(call dtc-version,010406,$(build_dtc),$(CONFIG_PYLIBFDT)))
+	echo here $(DTC) $(build_dtc)
+	if test "$(DTC)" = "$(build_dtc)"; then \
+		$(MAKE) $(build)=scripts/dtc; \
+	fi
+
 # prepare2 creates a makefile if using a separate output directory
 prepare2: prepare3 outputmakefile cfg
 
@@ -1963,6 +1976,8 @@  SYSTEM_MAP = \
 System.map:	u-boot
 		@$(call SYSTEM_MAP,$<) > $@
 
+build_dtc	:= $(objtree)/scripts/dtc/dtc
+
 #########################################################################
 
 # ARM relocations should all be R_ARM_RELATIVE (32-bit) or
diff --git a/dts/Kconfig b/dts/Kconfig
index 046a54a173..f8b808606c 100644
--- a/dts/Kconfig
+++ b/dts/Kconfig
@@ -5,9 +5,6 @@ 
 config SUPPORT_OF_CONTROL
 	bool
 
-config DTC
-	bool
-
 config PYLIBFDT
 	bool
 
@@ -24,7 +21,6 @@  menu "Device Tree Control"
 
 config OF_CONTROL
 	bool "Run-time configuration via Device Tree"
-	select DTC
 	select OF_LIBFDT if !OF_PLATDATA
 	help
 	  This feature provides for run-time configuration of U-Boot
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index b34dedade7..8c167cef2d 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -151,8 +151,11 @@  cc-fullversion = $(shell $(CONFIG_SHELL) \
 cc-ifversion = $(shell [ $(cc-version) $(1) $(2) ] && echo $(3) || echo $(4))
 
 # added for U-Boot
+# $1: min_version
+# 32: build_dtc
+# $3: need_pylibfdt
 binutils-version = $(shell $(CONFIG_SHELL) $(srctree)/scripts/binutils-version.sh $(AS))
-dtc-version = $(shell $(CONFIG_SHELL) $(srctree)/scripts/dtc-version.sh $(DTC))
+dtc-version = $(shell $(CONFIG_SHELL) $(srctree)/scripts/dtc-version.sh $(DTC) $1 $2 $3)
 
 # cc-ldoption
 # Usage: ldflags += $(call cc-ldoption, -Wl$(comma)--hash-style=both)
diff --git a/scripts/Makefile b/scripts/Makefile
index e7b353f77f..cfe9fef804 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -10,4 +10,3 @@  always		:= $(hostprogs-y)
 
 # Let clean descend into subdirs
 subdir-	+= basic kconfig
-subdir-$(CONFIG_DTC)	+= dtc
diff --git a/scripts/dtc-version.sh b/scripts/dtc-version.sh
index 0744c39eb0..75ba82830d 100755
--- a/scripts/dtc-version.sh
+++ b/scripts/dtc-version.sh
@@ -1,12 +1,26 @@ 
 #!/bin/sh
 #
-# dtc-version dtc-command
+# dtc-version dtc_command min_version build_dtc need_pylibfdt
 #
-# Prints the dtc version of `dtc-command' in a canonical 6-digit form
-# such as `010404'  for dtc 1.4.4
+# Selects which version of dtc to use
+#
+# If need_pylibfdt is non-empty then the script first checks that the Python
+# libfdt library is available. If not it outputs $build_dtc and exits
+#
+# Otherwise, if the version of dtc_command is < min_version, prints build_dtc
+# else prints dtc_command. The min_version is in the format MMmmpp where:
+#
+#   MM is the major version
+#   mm is the minor version
+#   pp is the patch level
+#
+# For example 010406 means 1.4.6
 #
 
-dtc="$*"
+dtc="$1"
+min_version="$2"
+build_dtc="$3"
+need_pylibfdt="$4"
 
 if [ ${#dtc} -eq 0 ]; then
 	echo "Error: No dtc command specified."
@@ -14,8 +28,20 @@  if [ ${#dtc} -eq 0 ]; then
 	exit 1
 fi
 
+if [ -n "${need_pylibfdt}" ]; then
+	if ! echo "import libfdt" | python3 2>/dev/null; then
+		echo $build_dtc
+		exit 0
+	fi
+fi
+
 MAJOR=$($dtc -v | head -1 | awk '{print $NF}' | cut -d . -f 1)
 MINOR=$($dtc -v | head -1 | awk '{print $NF}' | cut -d . -f 2)
 PATCH=$($dtc -v | head -1 | awk '{print $NF}' | cut -d . -f 3 | cut -d - -f 1)
 
-printf "%02d%02d%02d\\n" $MAJOR $MINOR $PATCH
+version="$(printf "%02d%02d%02d" $MAJOR $MINOR $PATCH)"
+if test $version -lt $min_version; then \
+	echo $build_dtc
+else
+	echo $dtc
+fi