configure.ac: don't unset LDFLAGS
#10678
Conversation
The `LDFLAGS` variable was initially cleared by commit c4d84aa in 2009, and reverted in commit 21a6e1f later that year, but the revert was reverted one day later in commit 937358e; the edit war continued another day later with another revert by commit 477649c, but was reverted again on the same day by commit 1645041 The reverts (don't clear) were documented: "unsetting LIBS and LDFLAGS just makes it impossible to specify LDFLAGS from the environment" ... but none of the "unset LDFLAGS" commits had any explanation in the commit message. Clearing `LDFLAGS` obviously breaks a feature that is documented by `./configure --help`: "Some influential environment variables: [...] LDFLAGS linker flags, e.g. -L<lib dir> if you have libraries in a nonstandard directory <lib dir>" This feature is used by several CI scripts, e.g. `.cirrus.yml`, `.github/actions/configure-x32/action.yml` and `.github/nightly_matrix.php` - but there, it didn't ever work. Apparently, nobody ever noticed this. Unlike the other reverts, this patch keeps the `unset LIBS`, because the `LIBS` variable had already been copied to `EXTRA_LIBS`. Same for the last `unset LIBS LDFLAGS` command in line 1374: prior to that, `LDFLAGS` had already been copied to `EXTRA_LDFLAGS`. Maybe that's enough to keep people from revert it again and again? With this patch, I can use the `mold` linker for the first time by simply doing `./configure .... LDFLAGS="-fuse-ld=mold"`.
There was a problem hiding this comment.
This is LGTM from my point of view. The first two LDFLAGS unsets are redundant in today's configure.ac, while the 3rd one is ok and clears the LDFLAGS for further processing in other macros. Thanks @MaxKellermann
|
Yeah, PHP is quite something, I know! 😆 But we should remain hopeful, nonetheless, and wait a bit longer with PRs. I haven't merged this because I leave merging on this repository to current maintainers. cc @iluuu1994 |
|
I don't object to this change, but I'm not familiar enough with the build system to review this. There was word of caution on the ML but IMO a build potentially breaking on @petk Looking at your commit history, you seem much more capable of reviewing this so I trust your judgement 🙂 There are also internal talks about regulating changes like this, so it might be best to wait until we have something concrete on that front. |
The
LDFLAGSvariable was initially cleared by commit c4d84aa in 2009, and reverted in commit 21a6e1f later that year, but the revert was reverted one day later in commit 937358e; the edit war continued another day later with another revert by commit 477649c, but was reverted again on the same day by commit 1645041The reverts (don't clear) were documented:
"unsetting LIBS and LDFLAGS just makes it impossible to specify
LDFLAGS from the environment"
... but none of the "unset LDFLAGS" commits had any explanation in the commit message.
Clearing
LDFLAGSobviously breaks a feature that is documented by./configure --help:"Some influential environment variables:
[...]
LDFLAGS linker flags, e.g. -L if you have libraries in a
nonstandard directory "
This feature is used by several CI scripts, e.g.
.cirrus.yml,.github/actions/configure-x32/action.ymland.github/nightly_matrix.php- but there, it didn't ever work. Apparently, nobody ever noticed this.Unlike the other reverts, this patch keeps the
unset LIBS, because theLIBSvariable had already been copied toEXTRA_LIBS. Same for the lastunset LIBS LDFLAGScommand in line 1374: prior to that,LDFLAGShad already been copied toEXTRA_LDFLAGS. Maybe that's enough to keep people from revert it again and again?With this patch, I can use the
moldlinker for the first time by simply doing./configure .... LDFLAGS="-fuse-ld=mold".(php-internals thread: https://news-web.php.net/php.internals/119593)