| Message ID | 20200427002929.239379-2-sjg@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] Revert "kbuild: remove unused dtc-version.sh script" | expand |
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
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
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>
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.
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
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!
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
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.
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-----
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
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(-)