XPost: linux.debian.bugs.dist
From: [email protected]

Package: dpkg-dev
Version: 1.22.8
Severity: important

Hi!

It seems like constructs like

include /usr/share/dpkg/buildflags.mk
CFLAGS += -DFOO
LDFLAGS += -lbar

etc somehow stopped working on buildds since dpkg 1.22.8,
although I can't reproduce it locally no matter how I try.

An example is samba package which uses this construct in
a few places, and later uses the variables like this:

CFLAGS="${CFLAGS}" LDFLAGS="${LDFLAGS}" ./configure ...

No matter what I append to these variables, the resulting
value does not change.

I can only guess this is due to dpkg_lazy_eval which resets
values of each variable (it has `eval VAR=VAL` inside), but
the thing is that I can't reproduce it locally. Something
might be different on a buildd.

Yes, I can switch to using DEB_LDFLAGS_MAINT_APPEND etc,
but it requires quite some rearrangements/reordering of
d/rules, and it doesn't tell me why current way, which
used to work before, broke now.

This issue potentially can affect multiple packages in an
unexpected way, because CFLAGS+=foo definitely used to
work before.

And no, 1.22.9 upload does not fix the issue.

--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)

XPost: linux.debian.bugs.dist
From: [email protected]

Hello.

Hmm, at this point I'm starting to ponder whether to revert the
optimization commit for the Makefile fragment files, because this
is starting to feel like too much breakage, and then the fragment
code is becoming too hard to debug, or even test.

This sentence seems a bit unfair. The new implementation has come
with new regression tests, and does not increase the source
complexity.

I've not yet looked into it, Nicolas if you can have a look please,
otherwise I might do the revert and another upload later today or so.

I will investigate, but without much hope. The difference is probably
caused by things like conflicting CFLAGS on the command line or in the environment, from debian/rules or dpkg-buildpackage, for ./configure
or make, possibly kept by ./configure for make despite a now
conflicting environment… This mess is probably one of the reasons why DEB_CFLAGS_MAINT_APPEND was introduced and CFLAGS+= deprecated in both
the environment and debian/rules.
Moreover, I cannot connect to buildds either.

The fact that the previous lazy evaluation mechanism, in which the
$(evals VAR=$(VAR)) trick is already present, did what you expect in
some contexts does not make CFLAGS+= a supported interface, and your
code could break in other contexts.

Replacing
CFLAGS+= foo
with
DEB_CFLAGS_MAINT_APPEND := foo
in the broken packages seems more fruitful to me, and I would prefer
to help with that.

That would most probably fix this bug, does not hurt if the
optimization is reverted, and will actively help if dpkg-buildpackage
ever becomes the main entry point for package builds.

--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)

XPost: linux.debian.bugs.dist
From: [email protected]

Hi!

On Thu, 2024-07-25 at 10:19:28 +0300, Michael Tokarev wrote:

Package: dpkg-dev
Version: 1.22.8
Severity: important

It seems like constructs like

include /usr/share/dpkg/buildflags.mk
CFLAGS += -DFOO
LDFLAGS += -lbar

etc somehow stopped working on buildds since dpkg 1.22.8,
although I can't reproduce it locally no matter how I try.

An example is samba package which uses this construct in
a few places, and later uses the variables like this:

CFLAGS="${CFLAGS}" LDFLAGS="${LDFLAGS}" ./configure ...

No matter what I append to these variables, the resulting
value does not change.

I can only guess this is due to dpkg_lazy_eval which resets
values of each variable (it has `eval VAR=VAL` inside), but
the thing is that I can't reproduce it locally. Something
might be different on a buildd.

Yes, I can switch to using DEB_LDFLAGS_MAINT_APPEND etc,
but it requires quite some rearrangements/reordering of
d/rules, and it doesn't tell me why current way, which
used to work before, broke now.

This issue potentially can affect multiple packages in an
unexpected way, because CFLAGS+=foo definitely used to
work before.

And no, 1.22.9 upload does not fix the issue.

Hmm, at this point I'm starting to ponder whether to revert the
optimization commit for the Makefile fragment files, because this
is starting to feel like too much breakage, and then the fragment
code is becoming too hard to debug, or even test.

Although, my ideal path forward has been for a long time to try to
make these files completely unnecessary, and instead make
dpkg-buildpackage be able to set any of these flags, which would be
way cheaper as it has already computed most if not all of those, and
avoid reparsing files, etc. But that partially depends on making dpkg-buildpackage the only supported entry point. I'll try to bring
that up on the list in a bit.

I've not yet looked into it, Nicolas if you can have a look please,
otherwise I might do the revert and another upload later today or so.

Thanks,
Guillem

--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)

XPost: linux.debian.bugs.dist
From: [email protected]

Michael has already replaced CFLAGS+=foo with DEB_CFLAGS_MAINT_APPEND:=foo https://salsa.debian.org/samba-team/samba/-/commit/505e4ff2084280b09eb1ecea277de056ff62684a

Ironically, the debian/rules for samba contains the exact same
optimization we are talking about:
# Fast version of dpkg/architecture.mk defining all vars in one go
ifeq (${DEB_HOST_MULTIARCH},)
$(foreach d, $(shell dpkg-architecture | sed 's/=/?=/'), $(eval export $d))
endif

--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)

XPost: linux.debian.bugs.dist
From: [email protected]

Control: reassign -1 1076932 dpkg-dev
Control: forcemerge -1 1076932
Control: affects -1 src:webdis

On Fri, Jul 26, 2024 at 01:20:28PM GMT, Nicolas Boulenguez wrote:

The fact that the previous lazy evaluation mechanism, in which the
$(evals VAR=$(VAR)) trick is already present, did what you expect in
some contexts does not make CFLAGS+= a supported interface, and your
code could break in other contexts.

Replacing
CFLAGS+= foo
with
DEB_CFLAGS_MAINT_APPEND := foo
in the broken packages seems more fruitful to me, and I would prefer
to help with that.

I'm running into a similar problem with webdis (#1076932), however in
that case we're doing

CFLAGS += $(CPPFLAGS)

I'm not sure how to do that with the DEB_CFLAGS_MAINT_APPEND. My naïve
attempt at

DEB_CFLAGS_MAINT_APPEND = $(CPPFLAGS)

results in

/usr/share/dpkg/buildflags.mk:51: *** Recursive variable 'dpkg_buildflags_run' references itself (eventually). Stop.

Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB

--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)

XPost: linux.debian.bugs.dist
From: [email protected]

Hi!

On Fri, 2024-07-26 at 13:20:28 +0200, Nicolas Boulenguez wrote:

Hmm, at this point I'm starting to ponder whether to revert the optimization commit for the Makefile fragment files, because this
is starting to feel like too much breakage, and then the fragment
code is becoming too hard to debug, or even test.

This sentence seems a bit unfair. The new implementation has come
with new regression tests, and does not increase the source
complexity.

Oh, it was not my intention to disparage your work, I'm also glad
you have been helping out with these regressions, and to be very clear
I have no problem in general with implementation bugs or regressions
which happen to all of us, as long as I get to get these fixed quickly (timelines depending on the severity of course, etc).

My comment was more about make(1) being a very brittle language to use
for general programming, and the fact that to get good performance it
needs such a maze of nested eval processing, the multiple variable
origins, etc, which IMO does add complexity, cognitive if nothing else
(and of course the previous code was already not trivial).

I've not yet looked into it, Nicolas if you can have a look please, otherwise I might do the revert and another upload later today or so.

I will investigate, but without much hope. The difference is probably
caused by things like conflicting CFLAGS on the command line or in the environment, from debian/rules or dpkg-buildpackage, for ./configure
or make, possibly kept by ./configure for make despite a now
conflicting environment…

Thanks for looking into it, in any case! If I end up reverting, I'd not necessarily consider this final, and I'd be fine reintroducing the
changes, once the problem is understood, etc, and ideally once we
can test for this, or if it's deemed a problem then after an orderly
transition has been planned, or perhaps as an additional implementation
to choose from based on the dpkg-build-api, so that maintainers can
opt-in to the new behavior and watch for issues, etc.

This mess is probably one of the reasons why
DEB_CFLAGS_MAINT_APPEND was introduced and CFLAGS+= deprecated in both
the environment and debian/rules.

I don't recall the exact context for these, and I don't think it's
worth digging that up in email/irc-logs, but I'd say these were added
just to provide a declarative (ish) way to get output flags already
as desired w/o needing to post-process them, and to match the
dpkg-buildflags config options, and globally settable environment
variables to override all build flags that people were probably starting
to misuse from within packaging.

The fact that the previous lazy evaluation mechanism, in which the
$(evals VAR=$(VAR)) trick is already present, did what you expect in
some contexts does not make CFLAGS+= a supported interface, and your
code could break in other contexts.

Replacing
CFLAGS+= foo
with
DEB_CFLAGS_MAINT_APPEND := foo
in the broken packages seems more fruitful to me, and I would prefer
to help with that.

That would most probably fix this bug, does not hurt if the
optimization is reverted, and will actively help if dpkg-buildpackage
ever becomes the main entry point for package builds.

The problem though is, that this is something that used to work, and
it is currently a silent breakage (the worst kind), which _could_ (but
might not) affect hundreds of packages. A not very exhaustive query on codesearch.d.n shows that just for CFLAGS this could indeed be a
potential problem for hundreds of packages. And I'm not very comfortable
with this kind of silent and subtle breakage at this scale, when that
can end up with either miscompilation, reduced security or features or
similar.

Thanks,
Guillem

--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)