Hi,
while working on https://github.com/php/php-src/pull/10663 I saw CI
failures because after that PR, the sanitizer flags were missing in
the linker call; they were only present in CFLAGS and LDFLAGS but not
in CXXFLAGS.
That is because ".cirrus.yml" sets LDFLAGS, but that value never gets
used because of the line "unset LIBS LDFLAGS" in configure.ac.
13 years ago, there was commit
https://github.com/php/php-src/commit/477649cd3f09 which attempted to
fix this, but was reverted on the same day by commit
https://github.com/php/php-src/commit/16450418b188 with just commit
message "Revert bad patch" but nothing else, no explanation what was
bad about the patch.
Yet the CI uses this variable, even though it's ignored. That went
unnoticed for years, because the sanitizer flags are passed through
CFLAGS to the linker.
Who can explain this? Shall we revert the revert to repair this?
(I despise bad commit messages like that. A revert always needs a
proper explanation.)
Max
13 years ago, there was commit
https://github.com/php/php-src/commit/477649cd3f09 which attempted to
fix this, but was reverted on the same day by commit
https://github.com/php/php-src/commit/16450418b188 with just commit
message "Revert bad patch" but nothing else, no explanation what was
bad about the patch.
Oh look, there was an "edit war" with a bunch of commits repeatedly
adding and commenting this "unset LDFLAGS" line:
https://github.com/php/php-src/commit/c4d84aa33390
https://github.com/php/php-src/commit/21a6e1f96229
https://github.com/php/php-src/commit/937358ebc7a6
https://github.com/php/php-src/commit/477649cd3f09
https://github.com/php/php-src/commit/16450418b188
13 years ago, there was commit
https://github.com/php/php-src/commit/477649cd3f09 which attempted to
fix this, but was reverted on the same day by commit
https://github.com/php/php-src/commit/16450418b188 with just commit
message "Revert bad patch" but nothing else, no explanation what was
bad about the patch.Oh look, there was an "edit war" with a bunch of commits repeatedly
adding and commenting this "unset LDFLAGS" line:https://github.com/php/php-src/commit/c4d84aa33390
https://github.com/php/php-src/commit/21a6e1f96229
https://github.com/php/php-src/commit/937358ebc7a6
https://github.com/php/php-src/commit/477649cd3f09
https://github.com/php/php-src/commit/16450418b188--
To unsubscribe, visit: https://www.php.net/unsub.php
From my quick check, the unset is initially done to move the currently
set flags to extra flags variable and clean the variable for further
additions so there are no duplicate ones or something like that.
https://github.com/php/php-src/commit/9417570dc1f31a0e3ddb9f6736a547c87ce7cfba
There seems to be another unset done to this change here:
https://github.com/php/php-src/commit/c4d84aa33390045cd4ff542719a0f79cff52fb7c
which fixed some bugs related to linker errors and duplicate symbols.
https://bugs.php.net/bug.php?id=14616
https://bugs.php.net/bug.php?id=14824
Most likely, some of those three unset lines could be removed today,
yes, but I can't be sure. If removal would fix some bug, then probably
it is time to remove them and see where things break afterwards.
From my quick check, the unset is initially done to move the currently
set flags to extra flags variable and clean the variable for further
additions so there are no duplicate ones or something like that.
https://github.com/php/php-src/commit/9417570dc1f31a0e3ddb9f6736a547c87ce7cfbaThere seems to be another unset done to this change here:
https://github.com/php/php-src/commit/c4d84aa33390045cd4ff542719a0f79cff52fb7c
which fixed some bugs related to linker errors and duplicate symbols.
Smells like people doing random things until some bug disappears...
and no commit messages here either, no explanation for the change.
Most likely, some of those three unset lines could be removed today,
yes, but I can't be sure. If removal would fix some bug, then probably
it is time to remove them and see where things break afterwards.
Yes, it fixes bugs. Once my other build system improvement PRs are
merged (the CFLAGS mess), I'll submit a patch to fix the LDFLAGS mess.
Max
Hi!
There seems to be another unset done to this change here:
https://github.com/php/php-src/commit/c4d84aa33390045cd4ff542719a0f79cff52fb7c
which fixed some bugs related to linker errors and duplicate symbols.Smells like people doing random things until some bug disappears...
and no commit messages here either, no explanation for the change.
Are you familiar with the principle called "Chesterton's fence"?
It could be these changes do not make sense. It also could be they are
necessary for the build to work on one of dozens of systems, distros and
tools combinations PHP is being built on. Unless you have tested on all
of them, I'd advise caution there.
--
Stas Malyshev
smalyshev@gmail.com
It could be these changes do not make sense. It also could be they are
necessary for the build to work on one of dozens of systems, distros and
tools combinations PHP is being built on. Unless you have tested on all of
them, I'd advise caution there.
I'm well aware that this is an area where lots of trouble can be
caused, and obviously I can't test all of them (I don't have anything
other than Linux).
This email thread is part me being cautious: I wanted an explanation
for these strange changes (and the "edit war"), but so far, nobody was
able to explain. I now know which bug reports provoked these changes,
but that doesn't explain the "fix", which rather looks like a random
kludge to me.
Workarounds that nobody can explain are a bad thing. One thing about
such workarounds is that they keep piling up, and after a few years,
nobody is brave enough to ever touch this knot, fearing it could break
something, and from here, everything detoriates. That fear is fair, I
don't mean to play down your cautious warning. But I think the pile
needs to be cut down eventually.
It's looong ago, but back in the day, I admit I was one of the few who
actually liked autoconf (not anymore), in spite of its weirdnesses.
One day, when I finally "got" how autoconf works, I figured that all
the layers of weird workarounds in my own scripts were just bad
kludges written by somebody who didn't know what he's doing (= me). I
learned that many bugs can be fixed by removing code.
An autoconf script must be simple and elegant (yes, I'm talking about
autoconf), and if it starts messing around with variables too much
(like PHP's LDFLAGS "fix"), it's a sign it's just a fragile mess.
Sure, PHP's configure script is battle-proven, because all the
workarounds are there, and the LDFLAGS mess appears to be part of
that. But that doesn't mean the script is really well-designed or
robust.
Max
If removal would fix some bug, then probably it is time to remove
them and see where things break afterwards.
Observe the output of PHP's "./configure --help":
"Some influential environment variables:
[...]
LDFLAGS linker flags, e.g. -L<lib dir> if you have libraries in a
nonstandard directory <lib dir>"
LDFLAGS is not just a convention, but is explicitly documented for
PHP, but it doesn't work, so this is clearly a bug.
Most likely, some of those three unset lines could be removed today,
yes, but I can't be sure. If removal would fix some bug, then probably
it is time to remove them and see where things break afterwards.