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.mk 20% 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 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})'
I'll try to revisit this, but I guess GNU sed might be ok for now.
> * scripts/buildtools.mk: style suggestions
> This arguably improves the readability, and fixes a minor issue
> ($(findstring nostrip,...) unwantedly matches arduinostrip).
I've taken the filter change, and the explicit origin one. I've left
out the nostrip one, because if we need more of those, that would
require repeating stuff. (Of course YAGNI could be invoked here, but
the code seems fine to me as it is now. :)
> * 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.
See my other comment about folding this one.
I've pushed the merged changes to git main.
Thanks,
Guillem
