Bug#872381: dpkg-dev: optimize Makefile snippets for debian/rules

[Nicolas Boulenguez]

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.

[Nicolas Boulenguez]

Please replace 0003.patch with the attached version (changes are
cosmetic but should use review and merge).

[Nicolas Boulenguez]

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.

[Nicolas Boulenguez]

Package: dpkg-dev
Version: 1.20.9
Followup-For: Bug #872381

The attached commits are, once more, manually rebased on the current
HEAD.

[Nicolas Boulenguez]

Package: dpkg-dev
Followup-For: Bug #872381

One more suggestion is attached.

[Nicolas Boulenguez]

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

[Guillem Jover]

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

[Nicolas Boulenguez]

> > [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 :-)

[Nicolas Boulenguez]

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.

[Guillem Jover]

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 <nic...@debian.org>
> 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 <nic...@debian.org>
> 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 <nic...@debian.org>
> 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 <nic...@debian.org>
> 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 <nic...@debian.org>
> 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 <nicolas.b...@free.fr>
> 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 <nic...@debian.org>
> 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 <nic...@debian.org>
> 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 <nic...@debian.org>
> Date: Sun, 13 Feb 2022 14:17:20 +0100
> Subject: [PATCH 09/10] scripts/mk: protect against repeated inclusion
>
> For example, buildtools.mk implicitly includes architecture.mk.

I've left this one out of now. I think it would be better to use a
dedicated variable instead of reusing the existing ones, in case the
callers set them for some reason, f.ex.

> >From 7a873a2e15dbb003aa9974c9019a7ca1821062e7 Mon Sep 17 00:00:00 2001
> From: Nicolas Boulenguez <nic...@debian.org>
> Date: Sun, 13 Feb 2022 13:41:26 +0100
> Subject: [PATCH 10/10] scripts/mk: improve details in documentation
>
> architecture.mk: give more details
> Mention default.mk as an alternative in included scripts.
> Improve consistency accross Makefile snippets.
> Stop documenting version restrictions older than oldoldstable.

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
aid with dependency management or deciding when API usage might be
satisfactory.

Thanks,
Guillem

[Nicolas Boulenguez]

-- 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.

[Nicolas Boulenguez]

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.