Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CURL_STATICLIB flag set even if linked with shared lib #17855

Closed
nono303 opened this issue Feb 18, 2025 · 2 comments
Closed

CURL_STATICLIB flag set even if linked with shared lib #17855

nono303 opened this issue Feb 18, 2025 · 2 comments

Comments

@nono303
Copy link
Contributor

nono303 commented Feb 18, 2025

Description

Looking at https://github.com/php/php-src/blob/master/ext/curl/config.w32#L18 when compiling ext/curl on Windows, CFLAG CURL_STATICLIB is set event if linked with shared libcurl.
I don’t know the extent of the impact but it might be interesting to set it only when it’s built against static libcurl.
My working patch (which can definitely be improved)

diff --git a/ext/curl/config.w32 b/ext/curl/config.w32
index 82be5212e5..14ac3ffca1 100644
--- a/ext/curl/config.w32
+++ b/ext/curl/config.w32
@@ -27,7 +27,10 @@ if (PHP_CURL != "no") {
 		) {
 		EXTENSION("curl", "interface.c multi.c share.c curl_file.c");
 		AC_DEFINE('HAVE_CURL', 1, "Define to 1 if the PHP extension 'curl' is available.");
-		ADD_FLAG("CFLAGS_CURL", "/D CURL_STATICLIB /D PHP_CURL_EXPORTS=1");
+		ADD_FLAG("CFLAGS_CURL", "/D PHP_CURL_EXPORTS=1");
+		if (CHECK_LIB("libcurl_a.lib", "curl", PHP_CURL)){
+			ADD_FLAG("CFLAGS_CURL", "/D CURL_STATICLIB");
+		}
 		PHP_INSTALL_HEADERS("ext/curl", "php_curl.h");
 	} else {
 		WARNING("curl not enabled; libraries and headers not found");

PHP Version

PHP 8.4.4

Operating System

Windows 11 - MSVC x64 build

@devnexen
Copy link
Member

cc @cmb69

@cmb69
Copy link
Member

cmb69 commented Feb 18, 2025

Yeah, that is clearly a bug. Thanks for reporting!

I'm not quite sure about the solution, though. Checking the lib again does not really hurt, but yields duplicate output. An alternative might be:

 ext/curl/config.w32 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ext/curl/config.w32 b/ext/curl/config.w32
index db584c29e7f..eddaa907e9c 100644
--- a/ext/curl/config.w32
+++ b/ext/curl/config.w32
@@ -3,7 +3,8 @@
 ARG_WITH("curl", "cURL support", "no");
 
 if (PHP_CURL != "no") {
-	if (CHECK_LIB("libcurl_a.lib;libcurl.lib", "curl", PHP_CURL) &&
+	var curl_location;
+	if ((curl_location = CHECK_LIB("libcurl_a.lib;libcurl.lib", "curl", PHP_CURL)) &&
 		CHECK_HEADER_ADD_INCLUDE("curl/easy.h", "CFLAGS_CURL") &&
 		SETUP_OPENSSL("curl", PHP_CURL) >= 2 &&
 		CHECK_LIB("winmm.lib", "curl", PHP_CURL) &&
@@ -15,7 +16,10 @@ if (PHP_CURL != "no") {
 		) {
 		EXTENSION("curl", "interface.c multi.c share.c curl_file.c");
 		AC_DEFINE('HAVE_CURL', 1, "Define to 1 if the PHP extension 'curl' is available.");
-		ADD_FLAG("CFLAGS_CURL", "/D CURL_STATICLIB /D PHP_CURL_EXPORTS=1");
+		ADD_FLAG("CFLAGS_CURL", "/D PHP_CURL_EXPORTS=1");
+		if (curl_location.match(/libcurl_a.lib$/)) {
+			ADD_FLAG("CFLAGS_CURL", "/D CURL_STATICLIB");
+		}
 		PHP_INSTALL_HEADERS("ext/curl", "php_curl.h");
 	} else {
 		WARNING("curl not enabled; libraries and headers not found");

But that introduces a new global variable. We could also wrap the code in a IIFE, but that is harder to read.

@cmb69 cmb69 changed the title CURL_STATICLIB flag set event if linked with shared libcurl [Windows] CURL_STATICLIB flag set even if linked with shared lib Feb 18, 2025
@cmb69 cmb69 self-assigned this Feb 18, 2025
cmb69 added a commit to cmb69/php-src that referenced this issue Feb 18, 2025
We must define `CURL_STATICLIB` only when building against a static
libcurl.  The detection relies on our usual naming conventions, what
should be revised in the future (possibly using pkg-config, or
switching to CMake).
cmb69 added a commit to cmb69/php-src that referenced this issue Feb 18, 2025
We must define `CURL_STATICLIB` only when building against a static
libcurl.  The detection relies on our usual naming conventions, what
should be revised in the future (possibly using pkg-config, or
switching to CMake).
cmb69 added a commit to cmb69/php-src that referenced this issue Feb 18, 2025
We must define `CURL_STATICLIB` only when building against a static
libcurl.  The detection relies on our usual naming conventions, what
should be revised in the future (possibly using pkg-config, or
switching to CMake).
@cmb69 cmb69 closed this as completed in 29c39a3 Feb 21, 2025
cmb69 added a commit that referenced this issue Feb 21, 2025
* PHP-8.3:
  Fix GH-17855: CURL_STATICLIB flag set even if linked with shared lib
cmb69 added a commit that referenced this issue Feb 21, 2025
* PHP-8.4:
  Fix GH-17855: CURL_STATICLIB flag set even if linked with shared lib
cmb69 added a commit that referenced this issue Feb 21, 2025
We must define `CURL_STATICLIB` only when building against a static
libcurl.  The detection relies on our usual naming conventions, what
should be revised in the future (possibly using pkg-config, or
switching to CMake).

Closes GH-17857.
cmb69 added a commit that referenced this issue Feb 21, 2025
* PHP-8.3:
  Fix GH-17855: CURL_STATICLIB flag set even if linked with shared lib
cmb69 added a commit that referenced this issue Feb 21, 2025
* PHP-8.4:
  Fix GH-17855: CURL_STATICLIB flag set even if linked with shared lib
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants