Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev
Followup-For: Bug #872381
Hello.
It is good to see the main suggestion merged. Thanks!
You have not applied
0001-scripts-mk-stop-hard-coding-dpkg_datadir.patch
probably because you prefer the related parts in
f1175056 (build: Rework subst handling for built or installed artifacts).
Ironically, f1175056 seems to introduce the exact kind of human error
that dynamic generation would prevent.
0001-build-spare-an-unneeded-subst-handling-in-pkg-info.m.patch
>From 36e98fdd10b1896f8fa89733b5e0c1781c0cce4c Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Mon, 6 May 2024 10:52:49 +0200
Subject: [PATCH] build: spare an unneeded subst handling in pkg-info.mk
This commits follows f1175056.
---
scripts/mk/Makefile.am | 1 -
1 file changed, 1 deletion(-)
diff --git a/scripts/mk/Makefile.am b/scripts/mk/Makefile.am
index be6076b2c..5f086ef49 100644
--- a/scripts/mk/Makefile.am
+++ b/scripts/mk/Makefile.am
@@ -18,5 +18,4 @@ include $(top_srcdir)/build-aux/subst.am
install-data-hook:
$(subst_make_file) $(DESTDIR)$(pkgdatadir)/default.mk
$(subst_make_file) $(DESTDIR)$(pkgdatadir)/buildtools.mk
- $(subst_make_file) $(DESTDIR)$(pkgdatadir)/pkg-info.mk
$(subst_make_file) $(DESTDIR)$(pkgdatadir)/vendor.mk
--
2.39.2
>From 7daa3aca068d997c6895757cb58ba91d66bd6842 Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Mon, 6 May 2024 11:37:14 +0200
Subject: [PATCH] scripts/mk: stop hard-coding dpkg_datadir
This path differ during tests and after installation. Instead of
rewriting the file with a hardcoded path, compute it within Make.
---
build-aux/subst.am | 8
scripts/mk/Makefile.am | 10 --
scripts/mk/buildtools.mk | 4 +++-
scripts/mk/default.mk| 2 +-
scripts/mk/vendor.mk | 4 +++-
5 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/build-aux/subst.am b/build-aux/subst.am
index 7785e4af7..9c96e5ce0 100644
--- a/build-aux/subst.am
+++ b/build-aux/subst.am
@@ -45,11 +45,3 @@ SUFFIXES += .pl
@test -d `dirname $@` || $(MKDIR_P) `dirname $@`
$(AM_V_GEN) $(subst_perl_filter) <$< >$@
$(AM_V_at) chmod +x $@
-
-# Makefile support.
-
-subst_make_rules = "\
- s{dpkg_datadir\s*=\s*[^\s]*}{dpkg_datadir = $(pkgdatadir)}; \
- "
-
-subst_make_file = $(PERL) -i -p -e $(subst_make_rules)
diff --git a/scripts/mk/Makefile.am b/scripts/mk/Makefile.am
index be6076b2c..6e85e17b9 100644
--- a/scripts/mk/Makefile.am
+++ b/scripts/mk/Makefile.am
@@ -10,13 +10,3 @@ dist_pkgdata_DATA = \
pkg-info.mk \
vendor.mk \
# EOL
-
-SUFFIXES =
-
-include $(top_srcdir)/build-aux/subst.am
-
-install-data-hook:
- $(subst_make_file) $(DESTDIR)$(pkgdatadir)/default.mk
- $(subst_make_file) $(DESTDIR)$(pkgdatadir)/buildtools.mk
- $(subst_make_file) $(DESTDIR)$(pkgdatadir)/pkg-info.mk
- $(subst_make_file) $(DESTDIR)$(pkgdatadir)/vendor.mk
diff --git a/scripts/mk/buildtools.mk b/scripts/mk/buildtools.mk
index 6ce9642cd..e93319e00 100644
--- a/scripts/mk/buildtools.mk
+++ b/scripts/mk/buildtools.mk
@@ -28,7 +28,9 @@
ifndef dpkg_buildtools_mk_included
dpkg_buildtools_mk_included = yes
-dpkg_datadir = $(srcdir)/mk
+ifndef dpkg_datadir
+ dpkg_datadir := $(patsubst %/buildtools.mk,%,$(lastword $(MAKEFILE_LIST)))
+endif
include $(dpkg_datadir)/architecture.mk
# We set the TOOL_FOR_BUILD variables to the specified value, and the TOOL
diff --git a/scripts/mk/default.mk b/scripts/mk/default.mk
index c4e408b01..e1b81 100644
--- a/scripts/mk/default.mk
+++ b/scripts/mk/default.mk
@@ -4,7 +4,7 @@
ifndef dpkg_default_mk_included
dpkg_default_mk_included = yes
-dpkg_datadir = $(srcdir)/mk
+dpkg_datadir := $(patsubst %/default.mk,%,$(lastword $(MAKEFILE_LIST)))
include $(dpkg_datadir)/architecture.mk
include $(dpkg_datadir)/buildapi.mk
ifeq ($(call dpkg_build_api_ge,1),yes)
diff --git a/scripts/mk/vendor.mk b/scripts/mk/vendor.mk
index 746503a33..3cd1eed3e 100644
--- a/scripts/mk/vendor.mk
+++ b/scripts/mk/vendor.mk
@@ -36,7 +36,9 @@
ifndef dpkg_vendor_mk_included
dpkg_vendor_mk_included = yes
-dpkg_datadir = $(srcdir)/mk
+ifndef dpkg_datadir
+ dpkg_datadir := $(patsubst %/vendor.mk,%,$(lastword $(MAKEFILE_LIST)))
+endif
include $(dpkg_datadir)/buildapi.mk
dpkg_lazy_eval ?= $(eval $(1) = $(2)$$($(1)))
--
2.39.2
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
The attached version
fixes the spacing issue in tests with a more readable trick,
splits the changes inside tests into small chunks,
replaces - with _ in Make variable names,
avoids non portable -r -E sed options.
Without -r, sed in {buildflags,pkg-info}.mk should cause no
regression. It has been present in pkg-info.mk since 2011.
>From 5b3d75fb604dae497406f19073d03ea094da8d07 Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Wed, 10 Apr 2024 00:41:42 +0200
Subject: [PATCH 01/10] scripts/t/mk/buildflags.mk: fix test of _MAINT_APPEND
when TEST_ is empty
When TEST_CPPFLAGS is empty, the right hand side of the comparison
starts with a space character.
---
scripts/t/mk/buildflags.mk | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/scripts/t/mk/buildflags.mk b/scripts/t/mk/buildflags.mk
index 94d85a7e0..7cf798f73 100644
--- a/scripts/t/mk/buildflags.mk
+++ b/scripts/t/mk/buildflags.mk
@@ -1,5 +1,8 @@
DEB_CPPFLAGS_MAINT_APPEND = -DTEST_MK=test-host
+TEST_CPPFLAGS+= -DTEST_MK=test-host
+
DEB_CPPFLAGS_FOR_BUILD_MAINT_APPEND = -DTEST_MK=test-build
+TEST_CPPFLAGS_FOR_BUILD+= -DTEST_MK=test-build
include $(srcdir)/mk/buildflags.mk
@@ -8,8 +11,8 @@ test:
test "$(ASFLAGS_FOR_BUILD)" = "$(TEST_ASFLAGS_FOR_BUILD)"
test "$(CFLAGS)" = "$(TEST_CFLAGS)"
test "$(CFLAGS_FOR_BUILD)" = "$(TEST_CFLAGS_FOR_BUILD)"
- test "$(CPPFLAGS)" = "$(TEST_CPPFLAGS) -DTEST_MK=test-host"
- test "$(CPPFLAGS_FOR_BUILD)" = "$(TEST_CPPFLAGS_FOR_BUILD)-DTEST_MK=test-build"
+ test "$(CPPFLAGS)" = "$(TEST_CPPFLAGS)"
+ test "$(CPPFLAGS_FOR_BUILD)" = "$(TEST_CXXFLAGS_FOR_BUILD)"
test "$(CXXFLAGS)" = "$(TEST_CXXFLAGS)"
test "$(CXXFLAGS_FOR_BUILD)" = "$(TEST_CXXFLAGS_FOR_BUILD)"
test "$(DFLAGS)" = "$(TEST_DFLAGS)"
--
2.39.2
>From 53aeebafeb2af84369df3b7d81ff1cbcc1e13a9d Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Wed, 10 Apr 2024 00:09:43 +0200
Subject: [PATCH 02/10] scripts/t/mk: use loops instead of repetitions
---
scripts/t/mk/architecture.mk | 54 +---
scripts/t/mk/buildflags.mk | 39 --
scripts/t/mk/buildtools.mk | 54 +++-
3 files changed, 61 insertions(+), 86 deletions(-)
diff --git a/scripts/t/mk/architecture.mk b/scripts/t/mk/architecture.mk
index 2ac0222ca..b146f34b7 100644
--- a/scripts/t/mk/architecture.mk
+++ b/scripts/t/mk/architecture.mk
@@ -1,36 +1,22 @@
include $(srcdir)/mk/architecture.mk
-test:
- test "$(DEB_BUILD_ARCH)" = "$(TEST_DEB_BUILD_ARCH)"
- test "$(DEB_BUILD_ARCH_ABI)" = "$(TEST_DEB_BUILD_ARCH_ABI)"
- test "$(DEB_BUILD_ARCH_BITS)" = "$(TEST_DEB_BUILD_ARCH_BITS)"
- test "$(DEB_BUILD_ARCH_CPU)" = "$(TEST_DEB_BUILD_ARCH_CPU)"
- test "$(DEB_BUILD_ARCH_ENDIAN)" = "$(TEST_DEB_BUILD_ARCH_ENDIAN)"
- test "$(DEB_BUILD_ARCH_LIBC)" = "$(TEST_DEB_BUILD_ARCH_LIBC)"
- test "$(DEB_BUILD_ARCH_OS)" = "$(TEST_DEB_BUILD_ARCH_OS)"
- test "$(DEB_BUILD_GNU_CPU)" = "$(TEST_DEB_BUILD_GNU_CPU)"
- test "$(DEB_BUILD_GNU_SYSTEM)" = "$(TEST_DEB_BUILD_GNU_SYSTEM)"
- test "$(DEB_BUILD_GNU_TYPE)" = "$(TEST_DEB_BUILD_GNU_TYPE)"
- test "$(DEB_BUILD_MULTIARCH)" = "$(TEST_DEB_BUILD_MULTIARCH)"
- test "$(DEB_HOST_ARCH)" = "$(TEST_DEB_HOST_ARCH)"
- test "$(DEB_HOST_ARCH_ABI)" = "$(TEST_DEB_HOST_ARCH_ABI)"
- test "$(DEB_HOST_ARCH_BITS)" = "$(TEST_DEB_HOST_ARCH_BITS)"
- test "$(DEB_HOST_ARCH_CPU)" = "$(TEST_DEB_HOST_ARCH_CPU)"
- test "$(DEB_HOST_ARCH_ENDIAN)" = "$(TEST_DEB_HOST_ARCH_ENDIAN)"
- test "$(DEB_HOST_ARCH_LIBC)" = "$(TEST_DEB_HOST_ARCH_LIBC)"
- test "$(DEB_HOST_ARCH_OS)" = "$(TEST_DEB_HOST_ARCH_OS)"
- test "$(DEB_HOST_GNU_CPU)" = "$(TEST_DEB_HOST_GNU_CPU)"
- test "$(DEB_HOST_GNU_SYSTEM)" = "$(TEST_DEB_HOST_GNU_SYSTEM)"
- test "$(DEB_HOST_GNU_TYPE)" = "$(TEST_DEB_HOST_GNU_TYPE)"
- test "$(DEB_HOST_MULTIARCH)" = "$(TEST_DEB_HOST_MULTIARCH)"
- test "$(DEB_TARGET_ARCH)" = "$(TEST_DEB_TARGET_ARCH)"
- test "$(DEB_TARGET_ARCH_ABI)" = "$(TEST_DEB_TARGET_ARCH_ABI)"
- test "$(DEB_TARGET_ARCH_BITS)" = "$(TEST_DEB_TARGET_ARCH_BITS)"
- test "$(DEB_TARGET_ARCH_CPU)" = "$(TEST_DEB_TARGET_ARCH_CPU)"
- test "$(DEB_TARGET_ARCH_ENDIAN)" = "$(TEST_DEB_TARGET_ARCH_ENDIAN)"
- test "$(DEB_TARGET_ARCH_LIBC)" = "$(TEST_DEB_TARGET_ARCH_LIBC)"
- test "$(DEB_TARGET_ARCH_OS)" = "$(TEST_DEB_TARGET_ARCH_OS)"
- test "$(DEB_TARGET_GNU_CPU)" = "$(TEST_DEB_TARGET_GNU_CPU)"
- test "$(DEB_TARGET_GNU_SYSTEM)" = "$(TEST_DEB_TARGET_GNU_SYSTEM)"
- test "$(DEB_TARGET_GNU_TYPE)" = "$(TEST_DEB_TARGET_GNU_TYPE)"
- test "$(DEB_TARGET_MULTIARCH)" = "$(TEST_DEB_TARGET_MULTIARCH)"
+vars := \
+ ARCH \
+ ARCH_ABI \
+ ARCH_BITS \
+ ARCH_CPU \
+ ARCH_ENDIAN \
+ ARCH_LIBC \
+ ARCH_OS \
+ GNU_CPU \
+ GNU_SYSTEM \
+ GNU_TYPE \
+ MULTIARCH \
+ # EOL
+loop_targets := $(foreach machine,BUILD HOST TARGET,$(foreach var,$(vars),\
+ DEB_$(machine)_$(var)))
+
+test: $(loop_targets)
+
+$(loop_targets):
+ test "$($@)" = "$(TEST_$@)"
diff --git a/scripts/t/mk/buildflags.mk b/scripts/t/mk/buildflags.mk
index
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Hi!
On Mon, 2024-03-11 at 01:44:30 +0100, Nicolas Boulenguez wrote:
> Package: dpkg-dev
> Followup-For: Bug #872381
> Please consider this new patch queue instead of the old or untested
> ones. With this one applied on 279c6ccb, the package builds and
> passes all tests.
Thanks!
> * scripts/mk: only use ASCII characters
> Cosmetic independent suggestion.
I'll skip this one, these are in comments are UTF-8 should be safe to
use.
> * scripts/mk: protect files against double inclusion
> The variables are renamed as you have recommended.
> The test is fixed (ifdef fails on a defined but empty variable).
Merged, although I've renamed the variables to avoid the slashes and
«.», as even though permitted by make, can be rather surprising and
have unintended consequences if they need to be passed to a shell
(unlikely here but…). And they hardcode a pathname that might not
match the actual destination of these files in the system (which can
be rather confusing).
> * scripts/mk: stop hard-coding dpkg_datadir
> Already discussed.
I've still skipped this for now, while I think I like it, see my
previous reply, as there's still the possibility we might need to
replace other stuff, such as SED anyway.
> * scripts/mk/buildopts.mk: search once for parallel= in DEB_BUILD_OPTIONS
>
> > > [...DEB_BUILD_OPTION_PARALLEL empty instead of undefined
> > > when parallel= is missing...]
> > [kind of an API change].
>
> I have changed my patch and updated the comment.
Merged.
> However..
> The policy only describes 'parallel=N' when N is a positive integer.
> I think we should assume that the option is either missing or valid.
> For me, 'parallel=' is as incorrect as 'parallel=foo'.
Right, but I'm not sure whether there's anything now relying on this,
which could break. :/
> > I think it might perhaps make more sense to fallback to setting it
> > to 1 if it's missing, but I need to ponder about possible
> > consequences/fallout, etc.
>
> I doubt any sensible default exist.
> * 1 is safe/produces readable logs and $max_available_processors is fast.
> * the policy/debhelper/... have found no one-size-fits-all solution.
Sure, let's leave this for now, it can always be revisited later on.
> * scripts/buildflags.mk: add missing GCJFLAGS
> Fixes a bug.
This was removed with commit 19dccf2b43d07ee0cb62ac002658768dce0b33aa,
due to the gcj project being dead since 2018.
> * scripts/buildflags.mk: generate the _FOR_BUILD variant of each variable
Merged.
> * scripts/buildflags.mk: sort the flag list
> These changes hopefully prevent new missing flags in the future (the
> output of dpkg-buildflags is sorted).
While in general I also prefer sorted lists, these currently try to
match the order in the toolchain stack, which also matches the order
in the manual page and other perl modules. I'll skip this for now.
> * scripts/*.mk: reduce the number of subprocesses
I've skipped this for now, see previous discussion about sed usage.
> * scripts/t: use loops instead of repetitions, check exports and overrides
Could you split this into one commit for the loop switch, and another
for the new tests? Also I think the later commit to handle spaces
should be folded into the loop splitting and new tests additions.
> * all four combinations of existing/new scripts/mk/*.mk pass the
> existing/new tests in scripts/t/mk/*.mk.
> * comparing the time taken by tests gives a rough idea of the speed
> gain
> architecture.mk 30 times faster (probably no gain under dpkg-buildpackage)
> buildflags.mk 20 times faster
> pkg-info.mk 4 times faster
> buildtools.mk20% faster
>
> Guillem Jover
> > I've left this one out for now. I'm not entirely satisfied with the
> > sed usage here. If we keep using sed, then I think it needs to be
> > set via a SED variable, substituted from the value found at
>
> In which context do you expect GNU Make but a non recent sed?
> Should I rewrite the regular expressions without -r/-E?
BSD systems come to mind, where GNU sed might be called gsed for
example, or not present at all. And where GNU make might be called
gmake. But the proper name is detected at configure time, so if we are
using sed then we should use the detected one.
> > configure time. But then, I've been pondering whether we can have
> > better export formats, that might make the sed usage not
> > necessary. I started with a make-eval export mode for buildflags,
> > but perhaps it would be better a more generic formatting mode where
> > the caller can specify how the output should look like, akin
> > «dpkg-query --showformat». Will ponder about this.
>
> A generic format would be more maintainable in the long term.
> Something like that would be convenient for the makefiles.
>
> dpkg-architecture --print-format='${Dollar}(eval export ${key} ?= ${value})'
> dpkg-buildflags --print-format='${Dollar}(eval ${key}:=${value})'
> dpkg-parsechangelog --print-format='${Dollar}(eval
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev
Followup-For: Bug #872381
Hello.
Please consider this new patch queue instead of the old or untested
ones. With this one applied on 279c6ccb, the package builds and
passes all tests.
* scripts/mk: only use ASCII characters
Cosmetic independent suggestion.
* scripts/mk: protect files against double inclusion
The variables are renamed as you have recommended.
The test is fixed (ifdef fails on a defined but empty variable).
* scripts/mk: stop hard-coding dpkg_datadir
Already discussed.
* scripts/mk/buildopts.mk: search once for parallel= in DEB_BUILD_OPTIONS
> > [...DEB_BUILD_OPTION_PARALLEL empty instead of undefined
> > when parallel= is missing...]
> [kind of an API change].
I have changed my patch and updated the comment.
However..
The policy only describes 'parallel=N' when N is a positive integer.
I think we should assume that the option is either missing or valid.
For me, 'parallel=' is as incorrect as 'parallel=foo'.
> I think it might perhaps make more sense to fallback to setting it
> to 1 if it's missing, but I need to ponder about possible
> consequences/fallout, etc.
I doubt any sensible default exist.
* 1 is safe/produces readable logs and $max_available_processors is fast.
* the policy/debhelper/... have found no one-size-fits-all solution.
* scripts/buildflags.mk: add missing GCJFLAGS
Fixes a bug.
* scripts/buildflags.mk: generate the _FOR_BUILD variant of each variable
* scripts/buildflags.mk: sort the flag list
These changes hopefully prevent new missing flags in the future (the
output of dpkg-buildflags is sorted).
* scripts/*.mk: reduce the number of subprocesses
* scripts/t: use loops instead of repetitions, check exports and overrides
* all four combinations of existing/new scripts/mk/*.mk pass the
existing/new tests in scripts/t/mk/*.mk.
* comparing the time taken by tests gives a rough idea of the speed
gain
architecture.mk 30 times faster (probably no gain under dpkg-buildpackage)
buildflags.mk 20 times faster
pkg-info.mk 4 times faster
buildtools.mk20% faster
Guillem Jover
> I've left this one out for now. I'm not entirely satisfied with the
> sed usage here. If we keep using sed, then I think it needs to be
> set via a SED variable, substituted from the value found at
In which context do you expect GNU Make but a non recent sed?
Should I rewrite the regular expressions without -r/-E?
> configure time. But then, I've been pondering whether we can have
> better export formats, that might make the sed usage not
> necessary. I started with a make-eval export mode for buildflags,
> but perhaps it would be better a more generic formatting mode where
> the caller can specify how the output should look like, akin
> «dpkg-query --showformat». Will ponder about this.
A generic format would be more maintainable in the long term.
Something like that would be convenient for the makefiles.
dpkg-architecture --print-format='${Dollar}(eval export ${key} ?= ${value})'
dpkg-buildflags --print-format='${Dollar}(eval ${key}:=${value})'
dpkg-parsechangelog --print-format='${Dollar}(eval DEB_SOURCE:=${Source})
${Dollar}(eval export SOURCE_DATE_EPOCH?=${Timestamp}) ..'
dpkg-vendor --print-format'${Dollar}(eval DEB_VENDOR:=${Vendor}) ${Dollar}(eval
DEB_PARENT_VENDOR:=${Parent})'
* scripts/buildtools.mk: style suggestions
This arguably improves the readability, and fixes a minor issue
($(findstring nostrip,...) unwantedly matches arduinostrip).
* scripts/t/mk/buildflags.mk: fix test of _MAINT_APPEND when TEST_ is empty
This fixes a minor issue. During a test with
DEB_BUILD_OPTIONS=noopt, TEST_CXXFLAGS was empty and caused the test
of DEB_CXXFLAGS_MAINT_APPEND to fail because the correct result is
not a concatenation, Make strips a space. This issue can also be
seen with 1.22.5.
>From 37f1089c450fca16d06d586cf390a05642af25f0 Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Mon, 4 Mar 2024 13:23:56 +0100
Subject: [PATCH 01/11] scripts/mk: only use ASCII characters
The policy recommends english, so french parenthesis must be replaced.
More generally, prudence recommends ASCII in Make scripts.
---
scripts/mk/buildtools.mk | 2 +-
scripts/mk/vendor.mk | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/mk/buildtools.mk b/scripts/mk/buildtools.mk
index 933fdcfaa..7c6732210 100644
--- a/scripts/mk/buildtools.mk
+++ b/scripts/mk/buildtools.mk
@@ -20,7 +20,7 @@
# QMAKE: Qt build system generator (since dpkg 1.20.0).
#
# All the above variables have a counterpart variable for the build tool,
-# as in CC → CC_FOR_BUILD.
+# as in CC -> CC_FOR_BUILD.
#
# The variables are not exported by default. This can be changed by
# defining DPKG_EXPORT_BUILDTOOLS.
diff --git a/scripts/mk/vendor.mk b/scripts/mk/vendor.mk
index f3241a57b..8bdaa235a 100644
--- a/scripts/mk/vendor.mk
+++ b/scripts/mk/vendor.mk
@@ -1,8 +1,8 @@
# This Makefile fragment (since dpkg
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev Followup-For: Bug #872381 This new version, based on c881a5a8, * splits protection from double inclusion and dpkg_datadir generation into separate commits * fixes an error in DEB_BUILD_OPTION_PARALLEL * removes a few dubious optimizations (like checking if dpkg_datadir is already computed in default.mk). * removes non-ASCII characters from comments >From e29be20064687eee52fa9b6c1ee1cb722867d590 Mon Sep 17 00:00:00 2001 From: Nicolas Boulenguez Date: Mon, 29 Jul 2019 14:38:32 +0200 Subject: [PATCH 01/10] scripts/mk: protect scripts from double inclusion Two such double inclusions already happen when default.mk is parsed. --- scripts/mk/architecture.mk | 5 + scripts/mk/buildapi.mk | 5 + scripts/mk/buildflags.mk | 6 ++ scripts/mk/buildopts.mk| 5 + scripts/mk/buildtools.mk | 5 + scripts/mk/default.mk | 5 + scripts/mk/pkg-info.mk | 5 + scripts/mk/vendor.mk | 5 + 8 files changed, 41 insertions(+) diff --git a/scripts/mk/architecture.mk b/scripts/mk/architecture.mk index c11cada16..2ffcee287 100644 --- a/scripts/mk/architecture.mk +++ b/scripts/mk/architecture.mk @@ -2,6 +2,9 @@ # DEB_BUILD_* variables that dpkg-architecture can return. Existing values # of those variables are preserved as per policy. +ifndef dpkg_architecture.mk_included +dpkg_architecture.mk_included := + dpkg_lazy_eval ?= $$(or $$(value DPKG_CACHE_$(1)),$$(eval DPKG_CACHE_$(1) := $$(shell $(2)))$$(value DPKG_CACHE_$(1))) dpkg_architecture_setvar = export $(1) ?= $(call dpkg_lazy_eval,$(1),dpkg-architecture -q$(1)) @@ -9,3 +12,5 @@ dpkg_architecture_setvar = export $(1) ?= $(call dpkg_lazy_eval,$(1),dpkg-archit $(foreach machine,BUILD HOST TARGET,\ $(foreach var,ARCH ARCH_ABI ARCH_LIBC ARCH_OS ARCH_CPU ARCH_BITS ARCH_ENDIAN GNU_CPU GNU_SYSTEM GNU_TYPE MULTIARCH,\ $(eval $(call dpkg_architecture_setvar,DEB_$(machine)_$(var) + +endif diff --git a/scripts/mk/buildapi.mk b/scripts/mk/buildapi.mk index 668e325c8..ba6b43543 100644 --- a/scripts/mk/buildapi.mk +++ b/scripts/mk/buildapi.mk @@ -1,5 +1,8 @@ # This Makefile fragment (since dpkg 1.22.0) handles the build API. +ifndef dpkg_buildapi.mk_included +dpkg_buildapi.mk_included := + # Default API level when not set. DPKG_BUILD_API ?= $(shell dpkg-buildapi) @@ -7,3 +10,5 @@ DPKG_BUILD_API ?= $(shell dpkg-buildapi) # complexity given no integer operators, given that we currently have to # fetch the build API level anyway. dpkg_build_api_ge = $(shell test "$(DPKG_BUILD_API)" -ge "$(1)" && echo yes) + +endif diff --git a/scripts/mk/buildflags.mk b/scripts/mk/buildflags.mk index 4b8a3d8c4..02baa53f2 100644 --- a/scripts/mk/buildflags.mk +++ b/scripts/mk/buildflags.mk @@ -28,6 +28,10 @@ # You can also export them in the environment by setting # DPKG_EXPORT_BUILDFLAGS to a non-empty value. # + +ifndef dpkg_buildflags.mk_included +dpkg_buildflags.mk_included := + # This list is kept in sync with the default set of flags returned # by dpkg-buildflags. @@ -77,3 +81,5 @@ $(foreach flag,$(DPKG_BUILDFLAGS_LIST),\ ifdef DPKG_EXPORT_BUILDFLAGS export $(DPKG_BUILDFLAGS_LIST) endif + +endif diff --git a/scripts/mk/buildopts.mk b/scripts/mk/buildopts.mk index c9519..6787da76f 100644 --- a/scripts/mk/buildopts.mk +++ b/scripts/mk/buildopts.mk @@ -5,6 +5,11 @@ # # DEB_BUILD_OPTION_PARALLEL: the argument for the parallel=N option. +ifndef dpkg_buildopts.mk_included +dpkg_buildopts.mk_included := + ifneq (,$(filter parallel=%,$(DEB_BUILD_OPTIONS))) DEB_BUILD_OPTION_PARALLEL = $(patsubst parallel=%,%,$(filter parallel=%,$(DEB_BUILD_OPTIONS))) endif + +endif diff --git a/scripts/mk/buildtools.mk b/scripts/mk/buildtools.mk index 933fdcfaa..08914c463 100644 --- a/scripts/mk/buildtools.mk +++ b/scripts/mk/buildtools.mk @@ -25,6 +25,9 @@ # The variables are not exported by default. This can be changed by # defining DPKG_EXPORT_BUILDTOOLS. +ifndef dpkg_buildtools.mk_included +dpkg_buildtools.mk_included := + dpkg_datadir = $(srcdir)/mk include $(dpkg_datadir)/architecture.mk @@ -74,3 +77,5 @@ $(eval $(call dpkg_buildtool_setvar,AR,ar)) $(eval $(call dpkg_buildtool_setvar,RANLIB,ranlib)) $(eval $(call dpkg_buildtool_setvar,PKG_CONFIG,pkgconf)) $(eval $(call dpkg_buildtool_setvar,QMAKE,qmake)) + +endif diff --git a/scripts/mk/default.mk b/scripts/mk/default.mk index 0b2fd4aca..b791f98a5 100644 --- a/scripts/mk/default.mk +++ b/scripts/mk/default.mk @@ -1,6 +1,9 @@ # This Makefile fragment (since dpkg 1.16.1) includes all the Makefile # fragments that define variables that can be useful within debian/rules. +ifndef dpkg_default.mk_included +dpkg_default.mk_included := + dpkg_datadir = $(srcdir)/mk include $(dpkg_datadir)/architecture.mk include $(dpkg_datadir)/buildapi.mk @@ -11,3 +14,5 @@ include $(dpkg_datadir)/buildflags.mk include $(dpkg_datadir)/buildopts.mk include $(dpkg_datadir)/pkg-info.mk include $(dpkg_datadir)/vendor.mk + +endif diff --git
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev Followup-For: Bug #872381 Hello. The attached commits rebase the suggestions, take your answers into account and slightly improved some style issues. There may remain typos, nothing is tested this time. >From d56d5af7fa1a01a581d0cc1901572ca9c407f538 Mon Sep 17 00:00:00 2001 From: Nicolas Boulenguez Date: Mon, 29 Jul 2019 14:38:32 +0200 Subject: [PATCH 1/8] scripts/mk: stop hard-coding dpkg_datadir, protect from double inclusion The Makefile snippets include each other from their common directory, but the path differ during tests and after installation. Instead of rewriting the file with a hardcoded path, compute it within Make. Use the same variables to avoid 'include' when possible, as it involves system calls. When setting dpkg_datadir, prefer 'ifndef' and ':=' to '?=', so that the value is computed at most once. --- build-aux/subst.am | 6 -- scripts/mk/Makefile.am | 21 - scripts/mk/architecture.mk | 5 + scripts/mk/buildapi.mk | 5 + scripts/mk/buildflags.mk | 6 ++ scripts/mk/buildopts.mk| 5 + scripts/mk/buildtools.mk | 11 ++- scripts/mk/default.mk | 10 +- scripts/mk/pkg-info.mk | 5 + scripts/mk/vendor.mk | 11 ++- 10 files changed, 55 insertions(+), 30 deletions(-) diff --git a/build-aux/subst.am b/build-aux/subst.am index 5515930d0..167a71257 100644 --- a/build-aux/subst.am +++ b/build-aux/subst.am @@ -39,9 +39,3 @@ SUFFIXES += .pl @test -d `dirname $@` || $(MKDIR_P) `dirname $@` $(do_perl_subst) <$< >$@ $(AM_V_at) chmod +x $@ - -# Makefile support. - -do_make_subst = $(AM_V_GEN) $(SED) \ - -e "s:dpkg_datadir[[:space:]]*=[[:space:]]*[^[:space:]]*:dpkg_datadir = $(pkgdatadir):" \ - # EOL diff --git a/scripts/mk/Makefile.am b/scripts/mk/Makefile.am index 257ba5252..6e85e17b9 100644 --- a/scripts/mk/Makefile.am +++ b/scripts/mk/Makefile.am @@ -10,24 +10,3 @@ dist_pkgdata_DATA = \ pkg-info.mk \ vendor.mk \ # EOL - -SUFFIXES = - -include $(top_srcdir)/build-aux/subst.am - -# Ideally we'd use '$(SED) -i', but unfortunately that's not portable. -install-data-hook: - $(do_make_subst) <$(DESTDIR)$(pkgdatadir)/default.mk \ - >$(DESTDIR)$(pkgdatadir)/default.mk.new - mv $(DESTDIR)$(pkgdatadir)/default.mk.new \ - $(DESTDIR)$(pkgdatadir)/default.mk - - $(do_make_subst) <$(DESTDIR)$(pkgdatadir)/buildtools.mk \ - >$(DESTDIR)$(pkgdatadir)/buildtools.mk.new - mv $(DESTDIR)$(pkgdatadir)/buildtools.mk.new \ - $(DESTDIR)$(pkgdatadir)/buildtools.mk - - $(do_make_subst) <$(DESTDIR)$(pkgdatadir)/vendor.mk \ - >$(DESTDIR)$(pkgdatadir)/vendor.mk.new - mv $(DESTDIR)$(pkgdatadir)/vendor.mk.new \ - $(DESTDIR)$(pkgdatadir)/vendor.mk diff --git a/scripts/mk/architecture.mk b/scripts/mk/architecture.mk index c11cada16..2ffcee287 100644 --- a/scripts/mk/architecture.mk +++ b/scripts/mk/architecture.mk @@ -2,6 +2,9 @@ # DEB_BUILD_* variables that dpkg-architecture can return. Existing values # of those variables are preserved as per policy. +ifndef dpkg_architecture.mk_included +dpkg_architecture.mk_included := + dpkg_lazy_eval ?= $$(or $$(value DPKG_CACHE_$(1)),$$(eval DPKG_CACHE_$(1) := $$(shell $(2)))$$(value DPKG_CACHE_$(1))) dpkg_architecture_setvar = export $(1) ?= $(call dpkg_lazy_eval,$(1),dpkg-architecture -q$(1)) @@ -9,3 +12,5 @@ dpkg_architecture_setvar = export $(1) ?= $(call dpkg_lazy_eval,$(1),dpkg-archit $(foreach machine,BUILD HOST TARGET,\ $(foreach var,ARCH ARCH_ABI ARCH_LIBC ARCH_OS ARCH_CPU ARCH_BITS ARCH_ENDIAN GNU_CPU GNU_SYSTEM GNU_TYPE MULTIARCH,\ $(eval $(call dpkg_architecture_setvar,DEB_$(machine)_$(var) + +endif diff --git a/scripts/mk/buildapi.mk b/scripts/mk/buildapi.mk index 668e325c8..ba6b43543 100644 --- a/scripts/mk/buildapi.mk +++ b/scripts/mk/buildapi.mk @@ -1,5 +1,8 @@ # This Makefile fragment (since dpkg 1.22.0) handles the build API. +ifndef dpkg_buildapi.mk_included +dpkg_buildapi.mk_included := + # Default API level when not set. DPKG_BUILD_API ?= $(shell dpkg-buildapi) @@ -7,3 +10,5 @@ DPKG_BUILD_API ?= $(shell dpkg-buildapi) # complexity given no integer operators, given that we currently have to # fetch the build API level anyway. dpkg_build_api_ge = $(shell test "$(DPKG_BUILD_API)" -ge "$(1)" && echo yes) + +endif diff --git a/scripts/mk/buildflags.mk b/scripts/mk/buildflags.mk index 4b8a3d8c4..02baa53f2 100644 --- a/scripts/mk/buildflags.mk +++ b/scripts/mk/buildflags.mk @@ -28,6 +28,10 @@ # You can also export them in the environment by setting # DPKG_EXPORT_BUILDFLAGS to a non-empty value. # + +ifndef dpkg_buildflags.mk_included +dpkg_buildflags.mk_included := + # This list is kept in sync with the default set of flags returned # by dpkg-buildflags. @@ -77,3 +81,5 @@ $(foreach flag,$(DPKG_BUILDFLAGS_LIST),\ ifdef DPKG_EXPORT_BUILDFLAGS export $(DPKG_BUILDFLAGS_LIST) endif + +endif diff --git a/scripts/mk/buildopts.mk b/scripts/mk/buildopts.mk
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
-- stop hard-coding dpkg_datadir > to avoid changing all pathname concatenation I changed dpkg_datadir to > «$(patsubst %/,%,$(dir $(lastword $(MAKEFILE_LIST». But given that > I think we might still need to substitute other things I've left this > one out for now. A rebased and slightly less intrusive version is attached. -- scripts/mk/buildopts.mk: small optimisations > I've left this one out as it is kind of an API change. I think it The tradition in Make APIs is to condider that "undefined" and "empty" are equivalent. I dislike this tradition, but it is widely accepted and there were precedents in scripts/mk/*.mk. > might perhaps make more sense to fallback to setting it to 1 if it's > missing, but I need to ponder about possible consequences/fallout, etc. In case you decide to keep an undefined (or empty :-) value by default, I suggest to add a commented example: # $(MAKE) $(addprefix -j,$(DEB_BUILD_OPTION_PARALLEL)) # SPHINXDOC += $(addprefix -j,$(DEB_BUILD_OPTION_PARALLEL)) $(addprefix) avoids an explicit test, which is exactly the purpose of DEB_BUILD_OPTION_PARALLEL as far as I understand. -- scripts/mk: reduce the number of subprocesses > I've left this one out for now. I'm not entirely satisfied with the > sed usage here. If we keep using sed, then I think it needs to be set > via a SED variable, substituted from the value found at configure > time. But then, I've been pondering whether we can have better export That is right only for packages using autoconf, but even then, how do you get $(SED) from debian/rules? Even autoconf-generated ./configure scripts assume that a minimal 'sed' is available, so I think we can do the same when building a Debian package. > formats, that might make the sed usage not necessary. I started with a > make-eval export mode for buildflags, but perhaps it would be better a > more generic formatting mode where the caller can specify how the > output should look like, akin «dpkg-query --showformat». Will ponder > about this. For me, the main problem is not whether COMMAND=dpkg-buildflags or COMMAND=dpkg-buildflags|sed, but Make itself. $(shell COMMAND) replaces line terminators with spaces, and then removes duplicate spaces. If we want to invoke COMMAND at most once for all variables, its ouput should contain multiple Make assignments. Make assignments are usually separated by line terminators, which are unavailable, so I have embedded each assignment in its own $(eval). In other words, I have used balanced parenthesis as separators. This seems a sensible compromise for build flags. The ideal solution would set Make variables with raw values, but I see no simple way. One could define ad-hoc variables like space := $(undefined) $(undefined) define new_line := endef that the output of COMMAND can $$(embed), but then you also need $(equal), $(colon), $(openparenthesis) and so on :-) A temporary file intended for inclusion by debian/rules solves most escaping problems, but adds complexity. -- scripts/mk: protect against repeated inclusion > dedicated variable instead of reusing the existing ones, in case the Fixed in the attached commit. -- scripts/mk: improve details in documentation > I've left this one out for now. The version information was added on > purpose to help people know when each interface or file was added, so Fixed in the attached commit (sorry, I thought that we were not supporting anything before oldoldstable). Thanks for the answers. >From 043f2cd6b8c445a2f0667260a2e4fabbe2e2488b Mon Sep 17 00:00:00 2001 From: Nicolas Boulenguez Date: Mon, 29 Jul 2019 14:38:32 +0200 Subject: [PATCH 1/6] scripts/mk: stop hard-coding dpkg_datadir The Makefile snippets include each other from their common directory, but the path differ during tests and after installation. Instead of rewriting the file with a hardcoded path, compute it within Make. --- build-aux/subst.am | 6 -- scripts/mk/Makefile.am | 16 scripts/mk/buildtools.mk | 2 +- scripts/mk/default.mk| 2 +- 4 files changed, 2 insertions(+), 24 deletions(-) diff --git a/build-aux/subst.am b/build-aux/subst.am index 74a40bf0b..4cd4bdca8 100644 --- a/build-aux/subst.am +++ b/build-aux/subst.am @@ -38,9 +38,3 @@ SUFFIXES += .pl @test -d `dirname $@` || $(MKDIR_P) `dirname $@` $(do_perl_subst) <$< >$@ $(AM_V_at) chmod +x $@ - -# Makefile support. - -do_make_subst = $(AM_V_GEN) $(SED) \ - -e "s:dpkg_datadir[[:space:]]*=[[:space:]]*[^[:space:]]*:dpkg_datadir = $(pkgdatadir):" \ - # EOL diff --git a/scripts/mk/Makefile.am b/scripts/mk/Makefile.am index a82e409d6..0c635bf00 100644 --- a/scripts/mk/Makefile.am +++ b/scripts/mk/Makefile.am @@ -9,19 +9,3 @@ dist_pkgdata_DATA = \ pkg-info.mk \ vendor.mk \ # EOL - -SUFFIXES = - -include $(top_srcdir)/build-aux/subst.am - -# Ideally we'd use '$(SED) -i', but unfortunately that's not portable. -install-data-hook: - $(do_make_subst) <$(DESTDIR)$(pkgdatadir)/default.mk \ -
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Hi! Thanks for the updated patches! I've gone over these and merged several of them, the others I've left out for now, as I wanted to get the current release out, which has been dragging for too long already while I was finalizing it. On Sun, 2022-02-13 at 18:38:19 +0100, Nicolas Boulenguez wrote: > >From 5852b310ea8cdd519a0f7d6e1099c3c54db026ed Mon Sep 17 00:00:00 2001 > From: Nicolas Boulenguez > Date: Mon, 29 Jul 2019 14:38:32 +0200 > Subject: [PATCH 01/10] scripts/mk: stop hard-coding dpkg_datadir > > The Makefile snippets include each other from their common directory, > but the path differ during tests and after installation. Instead of > rewriting the file with a hardcoded path, compute it within Make. Ah, nice, I think I like the dynamically computed path, yes. Although to avoid changing all pathname concatenation I changed dpkg_datadir to «$(patsubst %/,%,$(dir $(lastword $(MAKEFILE_LIST». But given that I think we might still need to substitute other things I've left this one out for now. > >From 94c84d34ff28d81f2fceef797fa8314d7b03fb23 Mon Sep 17 00:00:00 2001 > From: Nicolas Boulenguez > Date: Thu, 11 Feb 2021 15:36:15 +0100 > Subject: [PATCH 02/10] scripts/t: test SOURCE_DATE_EPOCH > > Set SOURCE_DATE_EPOCH either from the environment or the Debian changelog. > Check that the value is (re)exported. Merged. Changed from calling date(1) into hardcoding the timestamp though. > >From 32c2fad6ef96479afcffc38b40f8b2e82d3c46c4 Mon Sep 17 00:00:00 2001 > From: Nicolas Boulenguez > Date: Thu, 11 Feb 2021 15:45:03 +0100 > Subject: [PATCH 03/10] scripts/t: slightly optimize hash traversals > > Iterate on key/value pairs instead of iterating on keys then search > for each value. Merged. Changed the tools variable names as they were originally not a very good fit. > >From cb0d31dc92f61144150ad2b042a01987540e0ddf Mon Sep 17 00:00:00 2001 > From: Nicolas Boulenguez > Date: Thu, 11 Feb 2021 16:09:48 +0100 > Subject: [PATCH 04/10] scripts/t: use loops instead of repetitions, check > exports and overrides > > Replace copied lines with Make loops. > > Add tests: architecture variable override, buildflags set and export, > buildtool override and export. I've left this one out for now. > >From d27c9abc9d88a76c98597ee872adefd7c2dedd6a Mon Sep 17 00:00:00 2001 > From: Nicolas Boulenguez > Date: Thu, 11 Feb 2021 16:34:23 +0100 > Subject: [PATCH 05/10] scripts/buildtools.mk: remove unneeded conditionals > > The ?= had no effect when the previous test was succeeding. Make that > explicit with an 'else'. > > The 'ifdef' was always succeeding because previous stanza sets $1. Merged. > >From 26df5b04bb981bf9f1a23bf2341f5de1854e5daa Mon Sep 17 00:00:00 2001 > From: Nicolas Boulenguez > Date: Sat, 13 Feb 2021 09:58:27 +0100 > Subject: [PATCH 06/10] scripts/buildtools.mk: indent for readability Merged. > >From cb1a48beaa613b7f55dee0842afbd5ba51495b74 Mon Sep 17 00:00:00 2001 > From: Nicolas Boulenguez > Date: Mon, 1 Nov 2021 10:08:08 +0100 > Subject: [PATCH 07/10] scripts/mk/buildopts.mk: small optimisations > > Assign DEB_BUILD_OPTION_PARALLEL with := so that the value is computed > only once instead of every time the variable is used. > The maintainer is not supposed to modify DEB_BUILD_OPTIONS. > > Always define DEB_BUILD_OPTION_PARALLEL, even if empty when > DEB_BUILD_OPTIONS does not contain parallel=%. > The distinction between DEB_BUILD_OPTIONS= and > DEB_BUILD_OPTIONS=parallel= does probably not deserve a test. I've left this one out as it is kind of an API change. I think it might perhaps make more sense to fallback to setting it to 1 if it's missing, but I need to ponder about possible consequences/fallout, etc. > >From 4d63491c8dc9c6df85f0472d00f34c82e91ec05e Mon Sep 17 00:00:00 2001 > From: Nicolas Boulenguez > Date: Thu, 11 Feb 2021 16:26:50 +0100 > Subject: [PATCH 08/10] scripts/mk: reduce the number of subprocesses > > architecture.mk and buildflags.mk spawn less subshells, improving the > overall speed (the tests run twice faster according to bash time > builtin). > > pkg-info.mk uses the same trick than buildflags.mk in order to spawn > at most one subshell. The performance gain is visible, but minor > because there are way less variables. I've left this one out for now. I'm not entirely satisfied with the sed usage here. If we keep using sed, then I think it needs to be set via a SED variable, substituted from the value found at configure time. But then, I've been pondering whether we can have better export formats, that might make the sed usage not necessary. I started with a make-eval export mode for buildflags, but perhaps it would be better a more generic formatting mode where the caller can specify how the output should look like, akin «dpkg-query --showformat». Will ponder about this. > >From c92fd3aac8703475913db041c0bea53221757b5f Mon Sep 17 00:00:00 2001 > From: Nicolas Boulenguez > Date: Sun, 13 Feb 2022 14:17:20 +0100 > Subject: [PATCH 09/10]
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev
Followup-For: Bug #872381
Of course,
-dpkg_datadir := $(shell $(dir $(lastword $(MAKEFILE_LIST
+dpkg_datadir := $(dir $(lastword $(MAKEFILE_LIST)))
Hello.
I have rebased the changes and taken your explanations into
account. This new patch queue is not tested again, but should be
easyer to review.
>From 5852b310ea8cdd519a0f7d6e1099c3c54db026ed Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Mon, 29 Jul 2019 14:38:32 +0200
Subject: [PATCH 01/10] scripts/mk: stop hard-coding dpkg_datadir
The Makefile snippets include each other from their common directory,
but the path differ during tests and after installation. Instead of
rewriting the file with a hardcoded path, compute it within Make.
---
build-aux/subst.am | 6 --
scripts/mk/Makefile.am | 17 -
scripts/mk/buildtools.mk | 3 +--
scripts/mk/default.mk| 12 ++--
4 files changed, 7 insertions(+), 31 deletions(-)
diff --git a/build-aux/subst.am b/build-aux/subst.am
index 74a40bf0b..4cd4bdca8 100644
--- a/build-aux/subst.am
+++ b/build-aux/subst.am
@@ -38,9 +38,3 @@ SUFFIXES += .pl
@test -d `dirname $@` || $(MKDIR_P) `dirname $@`
$(do_perl_subst) <$< >$@
$(AM_V_at) chmod +x $@
-
-# Makefile support.
-
-do_make_subst = $(AM_V_GEN) $(SED) \
- -e "s:dpkg_datadir[[:space:]]*=[[:space:]]*[^[:space:]]*:dpkg_datadir = $(pkgdatadir):" \
- # EOL
diff --git a/scripts/mk/Makefile.am b/scripts/mk/Makefile.am
index 62d8592d2..0c635bf00 100644
--- a/scripts/mk/Makefile.am
+++ b/scripts/mk/Makefile.am
@@ -9,20 +9,3 @@ dist_pkgdata_DATA = \
pkg-info.mk \
vendor.mk \
# EOL
-
-SUFFIXES =
-
-include $(top_srcdir)/build-aux/subst.am
-
-install-data-hook:
- mv $(DESTDIR)$(pkgdatadir)/default.mk \
- $(DESTDIR)$(pkgdatadir)/default.mk.tmp
- $(do_make_subst) <$(DESTDIR)$(pkgdatadir)/default.mk.tmp \
- >$(DESTDIR)$(pkgdatadir)/default.mk
- rm -f $(DESTDIR)$(pkgdatadir)/default.mk.tmp
-
- mv $(DESTDIR)$(pkgdatadir)/buildtools.mk \
- $(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp
- $(do_make_subst) <$(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp \
- >$(DESTDIR)$(pkgdatadir)/buildtools.mk
- rm -f $(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp
diff --git a/scripts/mk/buildtools.mk b/scripts/mk/buildtools.mk
index b2ab2a2ac..5cd9151f1 100644
--- a/scripts/mk/buildtools.mk
+++ b/scripts/mk/buildtools.mk
@@ -26,8 +26,7 @@
# The variables are not exported by default. This can be changed by
# defining DPKG_EXPORT_BUILDTOOLS.
-dpkg_datadir = $(srcdir)/mk
-include $(dpkg_datadir)/architecture.mk
+include $(dir $(lastword $(MAKEFILE_LIST)))architecture.mk
# We set the TOOL_FOR_BUILD variables to the specified value, and the TOOL
# variables (for the host) to the their triplet-prefixed form iff they are
diff --git a/scripts/mk/default.mk b/scripts/mk/default.mk
index 3916a0c24..afe5d6876 100644
--- a/scripts/mk/default.mk
+++ b/scripts/mk/default.mk
@@ -1,9 +1,9 @@
# This Makefile fragment (since dpkg 1.16.1) includes all the Makefile
# fragments that define variables that can be useful within debian/rules.
-dpkg_datadir = $(srcdir)/mk
-include $(dpkg_datadir)/architecture.mk
-include $(dpkg_datadir)/buildflags.mk
-include $(dpkg_datadir)/buildopts.mk
-include $(dpkg_datadir)/pkg-info.mk
-include $(dpkg_datadir)/vendor.mk
+dpkg_datadir := $(dir $(lastword $(MAKEFILE_LIST)))
+include $(dpkg_datadir)architecture.mk
+include $(dpkg_datadir)buildflags.mk
+include $(dpkg_datadir)buildopts.mk
+include $(dpkg_datadir)pkg-info.mk
+include $(dpkg_datadir)vendor.mk
--
2.30.2
>From 94c84d34ff28d81f2fceef797fa8314d7b03fb23 Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Thu, 11 Feb 2021 15:36:15 +0100
Subject: [PATCH 02/10] scripts/t: test SOURCE_DATE_EPOCH
Set SOURCE_DATE_EPOCH either from the environment or the Debian changelog.
Check that the value is (re)exported.
---
scripts/t/mk.t | 9 -
scripts/t/mk/pkg-info.mk | 2 ++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/scripts/t/mk.t b/scripts/t/mk.t
index 10e030a16..25e25d56c 100644
--- a/scripts/t/mk.t
+++ b/scripts/t/mk.t
@@ -16,7 +16,7 @@
use strict;
use warnings;
-use Test::More tests => 10;
+use Test::More tests => 11;
use Test::Dpkg qw(:paths);
use File::Spec::Functions qw(rel2abs);
@@ -131,6 +131,13 @@ foreach my $tool (keys %buildtools) {
delete $ENV{"${tool}_FOR_BUILD"};
}
+delete $ENV{SOURCE_DATE_EPOCH};
+$ENV{TEST_SOURCE_DATE_EPOCH} = `date +%s -d "Tue, 04 Aug 2015 16:13:50 +0200"`;
+chomp $ENV{TEST_SOURCE_DATE_EPOCH};
+test_makefile('pkg-info.mk');
+
+$ENV{SOURCE_DATE_EPOCH} = 100;
+$ENV{TEST_SOURCE_DATE_EPOCH} = 100;
test_makefile('pkg-info.mk');
test_makefile('vendor.mk');
diff --git a/scripts/t/mk/pkg-info.mk b/scripts/t/mk/pkg-info.mk
index 22a2bf44f..c0e3287b5 100644
--- a/scripts/t/mk/pkg-info.mk
+++ b/scripts/t/mk/pkg-info.mk
@@ -7,3 +7,5 @@ test:
test "$(DEB_VERSION_UPSTREAM_REVISION)" = "2:3.4-5-6"
test "$(DEB_VERSION_UPSTREAM)" =
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
> > [in scripts/mk/Makefile.am], I suggest to rename
> > scripts/mk/{default,buildtools}.mk to scripts/mk/*.mk.sed or similar
> > (for example, .sed.mk in order to keep syntax highlighting). Distinct
> > source and object files would also simplify scripts/mk/Makefile a lot.
> Yeah, that would be more convenient, the problem is that these files
> need to have specific pathnames during tests, which are different from
> the ones at run-time. Those could be set from the environment, but that
> would miss the case where the variable is completely missing from the
> file. :)
Would this be OK?
dpkg_datadir := $(shell $(dir $(lastword $(MAKEFILE_LIST
If I remember well, only GNU Make supports this, but else something
like
dpkg_datadir != dirname `echo $(MAKEFILE_LIST) | sed 's/.* //'`
could probably be improved to compute the result only once.
> The main issue I see is that this trades performance depending on the
> mode of operation. Some of these variables are (currently) not
> expected to be set by the driver calling debian/rules (in Debian we
> still have to support that being any thing :/), but then most of our
> tools do go through dpkg-buildpackage. So the various «.mk» cannot
> expect these variables to be previously set, but should assume that
> the common case is them being executed through dpkg-buildpackage, and
> that one setting some of those, which means unconditionally setting
> some of them will make the package builds somewhat slower. The other
> variables should then not be set if not used, as that would slow down
> packages that do not need them.
As far as I have tested, my suggestion behaves exactly like the
current one. The only difference is that when several variables are
unset, the related dpkg-{architecture,buildflags,..} tool is called
only once.
> Ideally, most of this would not be needed at all, because we could
> rely on dpkg-buildpackage having exported all of these, which it
> already needs to compute in most cases anyway, but this is something
> we cannot have in Debian for now. I'm pondering adding a mode, not to
> be used by Debian, which could be used by local packages or other
> distros though.
A variable like DEB_BUILD_FLAGS_ALREADY_SET could shortcut all tests,
but I wonder if the speed gain would be worth the added complexity,
assuming that each dpkg-{architecture,buildflags,...} is called at
most once and only if at least a variable is both unset and actually
read.
> I'll go over these in the coming days.
I will do my best to rebase before that :-)
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Hi!
On Thu, 2022-02-10 at 21:53:59 +0100, Nicolas Boulenguez wrote:
> Package: dpkg-dev
> Followup-For: Bug #872381
> The --in-place sed option ([1]) may simplify scripts/mk/Makefile.am
> much more than the do_make_subst variable provided by
> build-aux/subst.am does. It is for now the only Makefile using
> do_make_subst.
I'm aware of -i, the problem is that this is unfortunately not portable,
see the comment f.ex. in scripts/Makefile.am, should perhaps add a
similar one in the new subst.am. Will also probably fix the few
instances in the test suites.
> If you select this option, I suggest to rename
> scripts/mk/{default,buildtools}.mk to scripts/mk/*.mk.sed or similar
> (for example, .sed.mk in order to keep syntax highlighting). Distinct
> source and object files would also simplify scripts/mk/Makefile a lot.
Yeah, that would be more convenient, the problem is that these files
need to have specific pathnames during tests, which are different from
the ones at run-time. Those could be set from the environment, but that
would miss the case where the variable is completely missing from the
file. :)
> More generally, I would really appreciate the opinion of a dpkg
> maintainer about the changes suggested in
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=872381. Rebasing
> and testing them again and again is an ungrateful work. Even "these
> changes seem interesting but require a careful review that we cannot
> afford now" would be more motivating than no answer at all.
Right, sorry, I keep seeing the mails, and concoct the replies in my
head, and either then think I've already replied or miss doing that
entirely. :/
The main issue I see is that this trades performance depending on the
mode of operation. Some of these variables are (currently) not
expected to be set by the driver calling debian/rules (in Debian we
still have to support that being any thing :/), but then most of our
tools do go through dpkg-buildpackage. So the various «.mk» cannot
expect these variables to be previously set, but should assume that
the common case is them being executed through dpkg-buildpackage, and
that one setting some of those, which means unconditionally setting
some of them will make the package builds somewhat slower. The other
variables should then not be set if not used, as that would slow down
packages that do not need them.
Ideally, most of this would not be needed at all, because we could
rely on dpkg-buildpackage having exported all of these, which it
already needs to compute in most cases anyway, but this is something
we cannot have in Debian for now. I'm pondering adding a mode, not to
be used by Debian, which could be used by local packages or other
distros though.
In any case, thanks for the work here, and sorry for the repeated
rebasing. I'll go over these in the coming days.
Thanks,
Guillem
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev
Followup-For: Bug #872381
(CCing Guillem as the author of the commits referenced below)
Hello.
The changes introduced by 1947fef8 and 981d5acb are basically
incompatible with the first patch in bug report #872381 [1].
For the record, commits a49f0a9c and f28cbcc4 also require a rebase of
the patches from #872381. The merge is trivial, if not automatic for
git.
The --in-place sed option ([1]) may simplify scripts/mk/Makefile.am
much more than the do_make_subst variable provided by
build-aux/subst.am does. It is for now the only Makefile using
do_make_subst.
On the other hand, do_make_subst (from 1947fef8 and 981d5acb) is more
flexible and allows the source and generated paths to differ if ever
necessary.
If you select this option, I suggest to rename
scripts/mk/{default,buildtools}.mk to scripts/mk/*.mk.sed or similar
(for example, .sed.mk in order to keep syntax highlighting). Distinct
source and object files would also simplify scripts/mk/Makefile a lot.
More generally, I would really appreciate the opinion of a dpkg
maintainer about the changes suggested in
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=872381. Rebasing
and testing them again and again is an ungrateful work. Even "these
changes seem interesting but require a careful review that we cannot
afford now" would be more motivating than no answer at all.
Thank you for your work on dpkg.
[1]
https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=872381;filename=0001-scripts-mk-simplify-Makefile.am-with-sed-in-place-op.patch;msg=30
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev Followup-For: Bug #872381 One more suggestion is attached. >From 95a908b270a533fc5d4a58a45deb929ce0d6159e Mon Sep 17 00:00:00 2001 From: Nicolas Boulenguez Date: Mon, 1 Nov 2021 10:08:08 +0100 Subject: [PATCH 8/8] scripts/mk/buildopts.mk: small optimisations Assign DEB_BUILD_OPTION_PARALLEL with := so that the value is computed only once instead of every time the variable is used. The maintainer is not supposed to modify DEB_BUILD_OPTIONS. Always define DEB_BUILD_OPTION_PARALLEL, even if empty when DEB_BUILD_OPTIONS does not contain parallel=%. The distinction between DEB_BUILD_OPTIONS= and DEB_BUILD_OPTIONS=parallel= does probably not deserve a test. --- scripts/mk/buildopts.mk | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/scripts/mk/buildopts.mk b/scripts/mk/buildopts.mk index c9519..420b6359c 100644 --- a/scripts/mk/buildopts.mk +++ b/scripts/mk/buildopts.mk @@ -5,6 +5,5 @@ # # DEB_BUILD_OPTION_PARALLEL: the argument for the parallel=N option. -ifneq (,$(filter parallel=%,$(DEB_BUILD_OPTIONS))) - DEB_BUILD_OPTION_PARALLEL = $(patsubst parallel=%,%,$(filter parallel=%,$(DEB_BUILD_OPTIONS))) -endif +DEB_BUILD_OPTION_PARALLEL := $(patsubst parallel=%,%,\ + $(filter parallel=%,$(DEB_BUILD_OPTIONS))) -- 2.30.2
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev
Version: 1.20.9
Followup-For: Bug #872381
The attached commits are, once more, manually rebased on the current
HEAD.
>From ad7a566e7966f7c8062a044d0c2df0b07b0011d7 Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Mon, 29 Jul 2019 14:38:32 +0200
Subject: [PATCH 1/8] scripts/mk: simplify Makefile.am with sed --in-place
option
---
scripts/mk/Makefile.am | 18 --
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/scripts/mk/Makefile.am b/scripts/mk/Makefile.am
index 9f0462eca..92e95c429 100644
--- a/scripts/mk/Makefile.am
+++ b/scripts/mk/Makefile.am
@@ -9,18 +9,8 @@ dist_pkgdata_DATA = \
pkg-info.mk \
vendor.mk
-do_path_subst = $(AM_V_GEN) sed \
- -e "s:dpkg_datadir[[:space:]]*=[[:space:]]*[^[:space:]]*:dpkg_datadir = $(pkgdatadir):"
-
install-data-hook:
- mv $(DESTDIR)$(pkgdatadir)/default.mk \
- $(DESTDIR)$(pkgdatadir)/default.mk.tmp
- $(do_path_subst) <$(DESTDIR)$(pkgdatadir)/default.mk.tmp \
- >$(DESTDIR)$(pkgdatadir)/default.mk
- rm -f $(DESTDIR)$(pkgdatadir)/default.mk.tmp
-
- mv $(DESTDIR)$(pkgdatadir)/buildtools.mk \
- $(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp
- $(do_path_subst) <$(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp \
- >$(DESTDIR)$(pkgdatadir)/buildtools.mk
- rm -f $(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp
+ $(AM_V_GEN) sed --in-place \
+ 's:dpkg_datadir[[:space:]]*=[[:space:]]*[^[:space:]]*:dpkg_datadir = $(pkgdatadir):' \
+ $(DESTDIR)$(pkgdatadir)/buildtools.mk \
+ $(DESTDIR)$(pkgdatadir)/default.mk
--
2.30.2
>From 0eeea0412d1d4320a48c410aa789e09bcc7c05b3 Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Thu, 11 Feb 2021 15:36:15 +0100
Subject: [PATCH 2/8] scripts: test SOURCE_DATE_EPOCH
Set SOURCE_DATE_EPOCH either from the environment or the Debian changelog.
Check that the value is (re)exported.
---
scripts/t/mk.t | 9 -
scripts/t/mk/pkg-info.mk | 2 ++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/scripts/t/mk.t b/scripts/t/mk.t
index 10e030a16..25e25d56c 100644
--- a/scripts/t/mk.t
+++ b/scripts/t/mk.t
@@ -16,7 +16,7 @@
use strict;
use warnings;
-use Test::More tests => 10;
+use Test::More tests => 11;
use Test::Dpkg qw(:paths);
use File::Spec::Functions qw(rel2abs);
@@ -131,6 +131,13 @@ foreach my $tool (keys %buildtools) {
delete $ENV{"${tool}_FOR_BUILD"};
}
+delete $ENV{SOURCE_DATE_EPOCH};
+$ENV{TEST_SOURCE_DATE_EPOCH} = `date +%s -d "Tue, 04 Aug 2015 16:13:50 +0200"`;
+chomp $ENV{TEST_SOURCE_DATE_EPOCH};
+test_makefile('pkg-info.mk');
+
+$ENV{SOURCE_DATE_EPOCH} = 100;
+$ENV{TEST_SOURCE_DATE_EPOCH} = 100;
test_makefile('pkg-info.mk');
test_makefile('vendor.mk');
diff --git a/scripts/t/mk/pkg-info.mk b/scripts/t/mk/pkg-info.mk
index 22a2bf44f..c0e3287b5 100644
--- a/scripts/t/mk/pkg-info.mk
+++ b/scripts/t/mk/pkg-info.mk
@@ -7,3 +7,5 @@ test:
test "$(DEB_VERSION_UPSTREAM_REVISION)" = "2:3.4-5-6"
test "$(DEB_VERSION_UPSTREAM)" = "2:3.4-5"
test "$(DEB_DISTRIBUTION)" = "suite"
+ test '$(SOURCE_DATE_EPOCH)' = '$(TEST_SOURCE_DATE_EPOCH)'
+ test "$${SOURCE_DATE_EPOCH}" = '$(TEST_SOURCE_DATE_EPOCH)'
--
2.30.2
>From 15661098522a9805472fa0c283e00a8f2d7ffc70 Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Thu, 11 Feb 2021 15:45:03 +0100
Subject: [PATCH 3/8] scripts/t: slightly optimize hash traversals
Iterate on key/value pairs instead of iterating on keys then search
for each value.
---
scripts/t/mk.t | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/scripts/t/mk.t b/scripts/t/mk.t
index 25e25d56c..139b19455 100644
--- a/scripts/t/mk.t
+++ b/scripts/t/mk.t
@@ -74,10 +74,15 @@ sub cmd_get_vars {
my %arch = cmd_get_vars($ENV{PERL}, "$srcdir/dpkg-architecture.pl", '-f');
-delete $ENV{$_} foreach keys %arch;
-$ENV{"TEST_$_"} = $arch{$_} foreach keys %arch;
+while (my ($k, $v) = each %arch) {
+delete $ENV{$k};
+$ENV{"TEST_$k"} = $v;
+}
test_makefile('architecture.mk', 'without envvars');
-$ENV{$_} = $arch{$_} foreach keys %arch;
+
+while (my ($k, $v) = each %arch) {
+$ENV{$k} = $v;
+}
test_makefile('architecture.mk', 'with envvars');
$ENV{DEB_BUILD_OPTIONS} = 'parallel=16';
@@ -88,8 +93,10 @@ delete $ENV{TEST_DEB_BUILD_OPTION_PARALLEL};
my %buildflag = cmd_get_vars($ENV{PERL}, "$srcdir/dpkg-buildflags.pl");
-delete $ENV{$_} foreach keys %buildflag;
-$ENV{"TEST_$_"} = $buildflag{$_} foreach keys %buildflag;
+while (my ($k, $v) = each %buildflag) {
+delete $ENV{$k};
+$ENV{"TEST_$k"} = $v;
+}
test_makefile('buildflags.mk');
my %buildtools = (
@@ -112,11 +119,11 @@ my %buildtools = (
PKG_CONFIG => 'pkg-config',
);
-foreach my $tool (keys %buildtools) {
+while (my ($tool, $default) = each %buildtools) {
delete $ENV{$tool};
-$ENV{"TEST_$tool"} = "$ENV{DEB_HOST_GNU_TYPE}-$buildtools{$tool}";
+$ENV{"TEST_$tool"} = "$ENV{DEB_HOST_GNU_TYPE}-$default";
delete $ENV{"${tool}_FOR_BUILD"};
-
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev
Followup-For: Bug #872381
The attached changes are split into smaller chunks and rebased on
55cdc52e. A merge conflict with 52166568 could have easily been
avoided, please review pending patches before reformatting comments.
>From f021e4d5ada4df39948afa339bd3a0859d28ed5a Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Mon, 29 Jul 2019 14:38:32 +0200
Subject: [PATCH 1/7] scripts/mk: simplify Makefile.am with sed --in-place
option
---
scripts/mk/Makefile.am | 18 --
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/scripts/mk/Makefile.am b/scripts/mk/Makefile.am
index 9f0462eca..92e95c429 100644
--- a/scripts/mk/Makefile.am
+++ b/scripts/mk/Makefile.am
@@ -9,18 +9,8 @@ dist_pkgdata_DATA = \
pkg-info.mk \
vendor.mk
-do_path_subst = $(AM_V_GEN) sed \
- -e "s:dpkg_datadir[[:space:]]*=[[:space:]]*[^[:space:]]*:dpkg_datadir = $(pkgdatadir):"
-
install-data-hook:
- mv $(DESTDIR)$(pkgdatadir)/default.mk \
- $(DESTDIR)$(pkgdatadir)/default.mk.tmp
- $(do_path_subst) <$(DESTDIR)$(pkgdatadir)/default.mk.tmp \
- >$(DESTDIR)$(pkgdatadir)/default.mk
- rm -f $(DESTDIR)$(pkgdatadir)/default.mk.tmp
-
- mv $(DESTDIR)$(pkgdatadir)/buildtools.mk \
- $(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp
- $(do_path_subst) <$(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp \
- >$(DESTDIR)$(pkgdatadir)/buildtools.mk
- rm -f $(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp
+ $(AM_V_GEN) sed --in-place \
+ 's:dpkg_datadir[[:space:]]*=[[:space:]]*[^[:space:]]*:dpkg_datadir = $(pkgdatadir):' \
+ $(DESTDIR)$(pkgdatadir)/buildtools.mk \
+ $(DESTDIR)$(pkgdatadir)/default.mk
--
2.30.0
>From 447b6dd8ff7bd1ed21c63a6285193fdd1980eebc Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Thu, 11 Feb 2021 15:36:15 +0100
Subject: [PATCH 2/7] scripts: test SOURCE_DATE_EPOCH
Set SOURCE_DATE_EPOCH either from the environment or the Debian changelog.
Check that the value is (re)exported.
---
scripts/t/mk.t | 9 -
scripts/t/mk/pkg-info.mk | 2 ++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/scripts/t/mk.t b/scripts/t/mk.t
index 766aa82e0..c9ca0bc9f 100644
--- a/scripts/t/mk.t
+++ b/scripts/t/mk.t
@@ -16,7 +16,7 @@
use strict;
use warnings;
-use Test::More tests => 10;
+use Test::More tests => 11;
use Test::Dpkg qw(:paths);
use File::Spec::Functions qw(rel2abs);
@@ -126,6 +126,13 @@ foreach my $tool (keys %buildtools) {
delete $ENV{"${tool}_FOR_BUILD"};
}
+delete $ENV{SOURCE_DATE_EPOCH};
+$ENV{TEST_SOURCE_DATE_EPOCH} = `date +%s -d "Tue, 04 Aug 2015 16:13:50 +0200"`;
+chomp $ENV{TEST_SOURCE_DATE_EPOCH};
+test_makefile('pkg-info.mk');
+
+$ENV{SOURCE_DATE_EPOCH} = 100;
+$ENV{TEST_SOURCE_DATE_EPOCH} = 100;
test_makefile('pkg-info.mk');
test_makefile('vendor.mk');
diff --git a/scripts/t/mk/pkg-info.mk b/scripts/t/mk/pkg-info.mk
index 22a2bf44f..c0e3287b5 100644
--- a/scripts/t/mk/pkg-info.mk
+++ b/scripts/t/mk/pkg-info.mk
@@ -7,3 +7,5 @@ test:
test "$(DEB_VERSION_UPSTREAM_REVISION)" = "2:3.4-5-6"
test "$(DEB_VERSION_UPSTREAM)" = "2:3.4-5"
test "$(DEB_DISTRIBUTION)" = "suite"
+ test '$(SOURCE_DATE_EPOCH)' = '$(TEST_SOURCE_DATE_EPOCH)'
+ test "$${SOURCE_DATE_EPOCH}" = '$(TEST_SOURCE_DATE_EPOCH)'
--
2.30.0
>From c7e5b518b5f356cba972c50f2f85187f8a17872a Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Thu, 11 Feb 2021 15:45:03 +0100
Subject: [PATCH 3/7] scripts/t: slightly optimize hash traversals
Iterate on key/value pairs instead of iterating on keys then search
for each value.
---
scripts/t/mk.t | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/scripts/t/mk.t b/scripts/t/mk.t
index c9ca0bc9f..33c933f06 100644
--- a/scripts/t/mk.t
+++ b/scripts/t/mk.t
@@ -69,10 +69,15 @@ sub cmd_get_vars {
my %arch = cmd_get_vars($ENV{PERL}, "$srcdir/dpkg-architecture.pl", '-f');
-delete $ENV{$_} foreach keys %arch;
-$ENV{"TEST_$_"} = $arch{$_} foreach keys %arch;
+while (my ($k, $v) = each %arch) {
+delete $ENV{$k};
+$ENV{"TEST_$k"} = $v;
+}
test_makefile('architecture.mk');
-$ENV{$_} = $arch{$_} foreach keys %arch;
+
+while (my ($k, $v) = each %arch) {
+$ENV{$k} = $v;
+}
test_makefile('architecture.mk');
$ENV{DEB_BUILD_OPTIONS} = 'parallel=16';
@@ -83,8 +88,10 @@ delete $ENV{TEST_DEB_BUILD_OPTION_PARALLEL};
my %buildflag = cmd_get_vars($ENV{PERL}, "$srcdir/dpkg-buildflags.pl");
-delete $ENV{$_} foreach keys %buildflag;
-$ENV{"TEST_$_"} = $buildflag{$_} foreach keys %buildflag;
+while (my ($k, $v) = each %buildflag) {
+delete $ENV{$k};
+$ENV{"TEST_$k"} = $v;
+}
test_makefile('buildflags.mk');
my %buildtools = (
@@ -107,11 +114,11 @@ my %buildtools = (
PKG_CONFIG => 'pkg-config',
);
-foreach my $tool (keys %buildtools) {
+while (my ($tool, $default) = each %buildtools) {
delete $ENV{$tool};
-$ENV{"TEST_$tool"} = "$ENV{DEB_HOST_GNU_TYPE}-$buildtools{$tool}";
+$ENV{"TEST_$tool"} =
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Please replace 0003.patch with the attached version (changes are cosmetic but should use review and merge). >From a93a87581dbe90bd15822f11f374ae6d76263ca4 Mon Sep 17 00:00:00 2001 From: Nicolas Boulenguez Date: Mon, 29 Jul 2019 19:03:16 +0200 Subject: [PATCH 3 (v2)/3] scripts/mk: improve subprocesses architecture.mk and buildflags.mk spawn less subshells, improving the overall speed (the tests run twice faster according to bash time builtin). pkg-info.mk uses the same trick than buildflags.mk in order to spawn at most one subshell. The performance gain is visible, but minor because there are way less variables. In buildtools.mk, remove the test 'ifdef $(1)', since $(1) is affected by previous stanza. diff --git a/scripts/mk/architecture.mk b/scripts/mk/architecture.mk index 0af96019d..86ef28a41 100644 --- a/scripts/mk/architecture.mk +++ b/scripts/mk/architecture.mk @@ -2,10 +2,9 @@ # that dpkg-architecture can return. Existing values of those variables # are preserved as per policy. -dpkg_lazy_eval ?= $$(or $$(value DPKG_CACHE_$(1)),$$(eval DPKG_CACHE_$(1) := $$(shell $(2)))$$(value DPKG_CACHE_$(1))) - -dpkg_architecture_setvar = export $(1) ?= $(call dpkg_lazy_eval,$(1),dpkg-architecture -q$(1)) - -$(foreach machine,BUILD HOST TARGET,\ - $(foreach var,ARCH ARCH_ABI ARCH_LIBC ARCH_OS ARCH_CPU ARCH_BITS ARCH_ENDIAN GNU_CPU GNU_SYSTEM GNU_TYPE MULTIARCH,\ -$(eval $(call dpkg_architecture_setvar,DEB_$(machine)_$(var) +# According to dpkg-architecture(1), the variables must always be +# exported, so we need at least an external subprocess. +# Attempt to set all variables with this subprocesses. +# This also avoids maintaining a copy of the variable list here. +# Each = is replaced with ?= in order to export existing overridden values. +$(eval $(shell dpkg-architecture | sed 's/\([^=]*\)\(.*\)/$$(eval export \1?\2)/')) diff --git a/scripts/mk/buildflags.mk b/scripts/mk/buildflags.mk index bb496e108..e7276a1d8 100644 --- a/scripts/mk/buildflags.mk +++ b/scripts/mk/buildflags.mk @@ -16,11 +16,13 @@ # This list is kept in sync with the default set of flags returned # by dpkg-buildflags. -dpkg_lazy_eval ?= $$(or $$(value DPKG_CACHE_$(1)),$$(eval DPKG_CACHE_$(1) := $$(shell $(2)))$$(value DPKG_CACHE_$(1))) - DPKG_BUILDFLAGS_LIST = CFLAGS CPPFLAGS CXXFLAGS OBJCFLAGS OBJCXXFLAGS \ GCJFLAGS FFLAGS FCFLAGS LDFLAGS +# Accumulate the parameters for dpkg-buildflags, in case it is ever +# called. + +DPKG_BUILDFLAGS_EXPORT_ENVVAR := define dpkg_buildflags_export_envvar ifdef $(1) DPKG_BUILDFLAGS_EXPORT_ENVVAR += $(1)="$$(value $(1))" @@ -33,11 +35,20 @@ $(foreach flag,$(DPKG_BUILDFLAGS_LIST),\ $(foreach operation,SET STRIP APPEND PREPEND,\ $(eval $(call dpkg_buildflags_export_envvar,DEB_$(flag)_MAINT_$(operation) -dpkg_buildflags_setvar = $(1) = $(call dpkg_lazy_eval,$(1),$(DPKG_BUILDFLAGS_EXPORT_ENVVAR) dpkg-buildflags --get $(1)) - -$(foreach flag,$(DPKG_BUILDFLAGS_LIST),\ - $(eval $(call dpkg_buildflags_setvar,$(flag +# This variable is only expanded on demand, and we ensure that it +# happens at most once.. +dpkg-buildflags_run = $(eval $(shell \ + $(DPKG_BUILDFLAGS_EXPORT_ENVVAR) dpkg-buildflags \ + | sed 's/^\([^=]*\)\(.*\)/$$(eval \1:\2)/')) ifdef DPKG_EXPORT_BUILDFLAGS + # We must compute all values right now. + $(dpkg-buildflags_run) export $(DPKG_BUILDFLAGS_LIST) +else + # Only run a subprocess when a variable is actually used, + # but then replace each recursive definition with a non-recursive one + # (and of course return the asked value). + $(foreach var,$(DPKG_BUILDFLAGS_LIST),\ +$(eval $(var)=$$(dpkg-buildflags_run)$$($(var endif diff --git a/scripts/mk/buildtools.mk b/scripts/mk/buildtools.mk index c3b44bb8a..a95b0a69c 100644 --- a/scripts/mk/buildtools.mk +++ b/scripts/mk/buildtools.mk @@ -23,27 +23,30 @@ # # The variables are not exported by default. This can be changed by # defining DPKG_EXPORT_BUILDTOOLS. +# +# If TOOL is undefined or contains the Make built-in value, +# it is assigned the Debian default prefixed with the host triplet. +# +# If TOOL_FOR_BUILD is undefined, +# it is assigned the value of TOOL for native builds, +# or the Debian default prefixed with the build triplet for cross builds. + dpkg_datadir = $(srcdir)/mk include $(dpkg_datadir)/architecture.mk -# We set the TOOL_FOR_BUILD variables to the specified value, and the TOOL -# variables (for the host) to the their triplet-prefixed form iff they are -# not defined or contain the make built-in defaults. On native builds if -# TOOL is defined and TOOL_FOR_BUILD is not, we fallback to use TOOL. define dpkg_buildtool_setvar ifeq ($(origin $(1)),default) $(1) = $(DEB_HOST_GNU_TYPE)-$(2) -endif +else $(1) ?= $(DEB_HOST_GNU_TYPE)-$(2) +endif -# On native build fallback to use TOOL if that's defined. ifeq ($(DEB_BUILD_GNU_TYPE),$(DEB_HOST_GNU_TYPE)) -ifdef $(1) $(1)_FOR_BUILD ?= $$($(1)) -endif -endif +else $(1)_FOR_BUILD ?=
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev
Version: 1.19.7
Followup-For: Bug #872381
Hello.
The attachments are commits based on the current git head (f0c3cccf).
2/ adds tests for the Make snippets.
The current implementation passes these new tests.
3/ implements the suggestion as a git commit.
It also passes all tests, slightly faster.
How can I help?
Thanks.
>From 55c1f85f2f371b94aa36db098cf5408213a8295c Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Mon, 29 Jul 2019 14:38:32 +0200
Subject: [PATCH 1/3] scripts/mk: simplify Makefile.am with sed --in-place
option
---
scripts/mk/Makefile.am | 18 --
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/scripts/mk/Makefile.am b/scripts/mk/Makefile.am
index 54a41e712..e2d1f7a43 100644
--- a/scripts/mk/Makefile.am
+++ b/scripts/mk/Makefile.am
@@ -8,18 +8,8 @@ dist_pkgdata_DATA = \
pkg-info.mk \
vendor.mk
-do_path_subst = $(AM_V_GEN) sed \
- -e "s:dpkg_datadir[[:space:]]*=[[:space:]]*[^[:space:]]*:dpkg_datadir = $(pkgdatadir):"
-
install-data-hook:
- mv $(DESTDIR)$(pkgdatadir)/default.mk \
- $(DESTDIR)$(pkgdatadir)/default.mk.tmp
- $(do_path_subst) <$(DESTDIR)$(pkgdatadir)/default.mk.tmp \
- >$(DESTDIR)$(pkgdatadir)/default.mk
- rm -f $(DESTDIR)$(pkgdatadir)/default.mk.tmp
-
- mv $(DESTDIR)$(pkgdatadir)/buildtools.mk \
- $(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp
- $(do_path_subst) <$(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp \
- >$(DESTDIR)$(pkgdatadir)/buildtools.mk
- rm -f $(DESTDIR)$(pkgdatadir)/buildtools.mk.tmp
+ $(AM_V_GEN) sed --in-place \
+ 's:dpkg_datadir[[:space:]]*=[[:space:]]*[^[:space:]]*:dpkg_datadir = $(pkgdatadir):' \
+ $(DESTDIR)$(pkgdatadir)/buildtools.mk \
+ $(DESTDIR)$(pkgdatadir)/default.mk
--
2.20.1
>From c9363d090298601494e3a551a98e7fdaad73abbf Mon Sep 17 00:00:00 2001
From: Nicolas Boulenguez
Date: Mon, 29 Jul 2019 18:34:13 +0200
Subject: [PATCH 2/3] scripts/mk: extend and simplify tests
Use less hash traversals, deal with key/values pairs instead of
iterating on keys and search each value.
There is no need to unexport build tools, tests happen in a separate process.
Test SOURCE_DATE_EPOCH, from environemnt or d/changelog.
Use Make loops instead of similar lines.
Check that values may be overridden.
Check that values are (or can be) exported, even if overridden.
---
scripts/t/mk.t | 35
scripts/t/mk/architecture.mk | 57 +---
scripts/t/mk/buildflags.mk | 33 +--
scripts/t/mk/buildtools.mk | 63 +---
scripts/t/mk/pkg-info.mk | 2 ++
5 files changed, 100 insertions(+), 90 deletions(-)
diff --git a/scripts/t/mk.t b/scripts/t/mk.t
index 98c7e5083..a46512b88 100644
--- a/scripts/t/mk.t
+++ b/scripts/t/mk.t
@@ -16,7 +16,7 @@
use strict;
use warnings;
-use Test::More tests => 8;
+use Test::More tests => 9;
use Test::Dpkg qw(:paths);
use File::Spec::Functions qw(rel2abs);
@@ -69,16 +69,23 @@ sub cmd_get_vars {
my %arch = cmd_get_vars($ENV{PERL}, "$srcdir/dpkg-architecture.pl", '-f');
-delete $ENV{$_} foreach keys %arch;
-$ENV{"TEST_$_"} = $arch{$_} foreach keys %arch;
+while (my ($k, $v) = each %arch) {
+delete $ENV{$k};
+$ENV{"TEST_$k"} = $v;
+}
test_makefile('architecture.mk');
-$ENV{$_} = $arch{$_} foreach keys %arch;
+
+while (my ($k, $v) = each %arch) {
+$ENV{$k} = $v;
+}
test_makefile('architecture.mk');
my %buildflag = cmd_get_vars($ENV{PERL}, "$srcdir/dpkg-buildflags.pl");
-delete $ENV{$_} foreach keys %buildflag;
-$ENV{"TEST_$_"} = $buildflag{$_} foreach keys %buildflag;
+while (my ($k, $v) = each %buildflag) {
+delete $ENV{$k};
+$ENV{"TEST_$k"} = $v;
+}
test_makefile('buildflags.mk');
my %buildtools = (
@@ -101,19 +108,21 @@ my %buildtools = (
PKG_CONFIG => 'pkg-config',
);
-foreach my $tool (keys %buildtools) {
+while (my ($tool, $default) = each %buildtools) {
delete $ENV{$tool};
-$ENV{"TEST_$tool"} = "$ENV{DEB_HOST_GNU_TYPE}-$buildtools{$tool}";
+$ENV{"TEST_$tool"} = "$ENV{DEB_HOST_GNU_TYPE}-$default";
delete $ENV{"${tool}_FOR_BUILD"};
-$ENV{"TEST_${tool}_FOR_BUILD"} = "$ENV{DEB_BUILD_GNU_TYPE}-$buildtools{$tool}";
+$ENV{"TEST_${tool}_FOR_BUILD"} = "$ENV{DEB_BUILD_GNU_TYPE}-$default";
}
test_makefile('buildtools.mk');
-foreach my $tool (keys %buildtools) {
-delete $ENV{${tool}};
-delete $ENV{"${tool}_FOR_BUILD"};
-}
+delete $ENV{SOURCE_DATE_EPOCH};
+$ENV{TEST_SOURCE_DATE_EPOCH} = `date +%s -d "Tue, 04 Aug 2015 16:13:50 +0200"`;
+chomp $ENV{TEST_SOURCE_DATE_EPOCH};
+test_makefile('pkg-info.mk');
+$ENV{SOURCE_DATE_EPOCH} = 100;
+$ENV{TEST_SOURCE_DATE_EPOCH} = 100;
test_makefile('pkg-info.mk');
test_makefile('vendor.mk');
diff --git a/scripts/t/mk/architecture.mk b/scripts/t/mk/architecture.mk
index 2ac0222ca..939835be3 100644
--- a/scripts/t/mk/architecture.mk
+++ b/scripts/t/mk/architecture.mk
@@ -1,36 +1,25 @@
+DEB_BUILD_ARCH := overridden
+
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev Version: 1.19.2 Followup-For: Bug #872381 Hi. This new version improves the readability in various aspects, and should be easyer to review. It fixes a difference with the current version in dpkg-dev by exporting all dpkg-architecture variables. Is there anything that I can do to help merging these changes? dpkg-patches2.tar.gz Description: application/gzip
Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules
Package: dpkg-dev Version: 1.18.24 Severity: wishlist Tags: patch Hello. A new implementation of /usr/share/dpkg/architecture.mk /usr/share/dpkg/buildflags.mk /usr/share/dpkg/pkg-info.mk is attached. You may use run_tests.sh to check that the behaviour remains the same in most circumstances, but roughly twice faster. The current implementation executes a dpkg-* subprocess when a new Make variable is read, then sets this variable. The new one executes a subprocess when the first variable is read, but then affects all variables so that no later execution is ever performed. This optimization cannot be applied to vendors.mk, but would not help much for 2 variables anyway. If you accept the files, please remove $(if $(dbg),$(warning subshell)) at the end of the dpkg-*-run line of each file. Thanks. dpkg-patches.tar.gz Description: application/gzip
