Factor out SETUP_ZLIB_LIB() #16798
Open
Factor out SETUP_ZLIB_LIB() #16798
+10
−9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a mere refactoring for core readability and maintainability reasons. Note that this function is similar to `SETUP_OPENSSL`, but since the zlib headers are not necessarily required, we append `_LIB`.
Extensions requiring zlib are looking only for zlib.lib if ext/zlib has been built as shared extension. This is overly restrictive at best, and actually makes no sense, since (a) we're not shipping shared zlib builds for years, and (b) users could have a zlib.lib which is a static build (they could even just rename zlib_a.lib to zlib.lib).
The bundled libgd relies on zlib; we're setting up the library, but don't check for the header (which is included by gd_gd2.c), which may pass configure, although the build fails. We better fail early if the required header is not available.
cmb69
commented
Nov 14, 2024
win32/build/confutils.js
Outdated
| function SETUP_ZLIB_LIB(target, path_to_check) | ||
| { | ||
| return ((PHP_ZLIB == "no" || PHP_ZLIB_SHARED) && CHECK_LIB("zlib_a.lib;zlib.lib", target, path_to_check)) || | ||
| (PHP_ZLIB == "yes" && !PHP_ZLIB_SHARED); |
There was a problem hiding this comment.
This might need some explanation. zlib (like a couple of other extensions) is a bit special, because if PHP with zlib as static extension, php8.dll provides the functions as if they had been built as shared extension. Thus, there is no need to involve zlib.lib.
There was a problem hiding this comment.
This is not correct; see #16801 (comment).
|
looks good but @petk might be more appropriate I think. |
|
I've just noticed that there is (and was) a glitch. If a user enables zlib and curl, but zlib can't be enabled because zlib.h is not found, configure would still pass (since curl is checked before zlib; there is no extension dependency), although the build would fail. I think we can stick with this, though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The code is repetitive, and had some minor bugs regarding edge-cases. Thus we rework a bit. The messages of the individual commits contain some more information.