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

Use pkg-config on Windows #16752

Open
cmb69 opened this issue Nov 11, 2024 · 4 comments · May be fixed by #16830
Open

Use pkg-config on Windows #16752

cmb69 opened this issue Nov 11, 2024 · 4 comments · May be fixed by #16830

Comments

@cmb69
Copy link
Member

cmb69 commented Nov 11, 2024

Description

On POSIX systems, we use pkg-config to configure most (all?) dependency libraries. On Windows, we still look for the headers and libraries "manually" (CHECK_LIB and CHECK_HEADER_ADD_INCLUDE, what is clumsy and likely more constraining than necessary. And maybe worse, it makes it harder to port m4 configurations to w32, or to keep them in sync. It gets especially annoying when we want to check for a certain package version (usually a minimum requirement). Instead I suggest to use pkg-config on Windows, too.

POC

I've put a pkg-config.exe in the PATH, put a suitable zlib.pc in %DEPS%\lib\pkgconfig, and added that to PKG_CONFIG_PATH.

Then I've applied the following patch to php-src:

 ext/zlib/config.w32      |  6 +++---
 win32/build/confutils.js | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/ext/zlib/config.w32 b/ext/zlib/config.w32
index 3bc24d88e1..623d752e57 100644
--- a/ext/zlib/config.w32
+++ b/ext/zlib/config.w32
@@ -3,9 +3,9 @@
 ARG_ENABLE("zlib", "ZLIB support", "yes");
 
 if (PHP_ZLIB == "yes") {
-	if (CHECK_LIB("zlib_a.lib;zlib.lib", "zlib", PHP_ZLIB) &&
-		CHECK_HEADER_ADD_INCLUDE("zlib.h", "CFLAGS", "..\\zlib;" + php_usual_include_suspects)) {
-
+	if (PKG_CHECK_MODULES("ZLIB", "zlib >= 1.2.11")) {
+		PHP_EVAL_INCLINE(ZLIB_CFLAGS);
+		PHP_EVAL_LIBLINE(ZLIB_LIBS, "zlib");
 		EXTENSION("zlib", "zlib.c zlib_fopen_wrapper.c zlib_filter.c", PHP_ZLIB_SHARED, "/D ZLIB_EXPORTS /DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
 		AC_DEFINE("HAVE_ZLIB", 1, "Define to 1 if the PHP extension 'zlib' is available.");
 
diff --git a/win32/build/confutils.js b/win32/build/confutils.js
index 3623dcf7e2..63bd1b867a 100644
--- a/win32/build/confutils.js
+++ b/win32/build/confutils.js
@@ -1054,6 +1054,51 @@ function CHECK_HEADER_ADD_INCLUDE(header_name, flag_name, path_to_check, use_env
 	return p;
 }
 
+function PKG_CHECK_MODULES(prefix, list_of_modules)
+{
+	var out;
+	STDOUT.Write("checking for " + list_of_modules + "... ");
+	if (!(out = execute("pkg-config --cflags " + list_of_modules))) {
+		STDOUT.WriteLine("no");
+		return false;
+	}
+	eval(prefix + "_CFLAGS = out");
+	if (!(out = execute("pkg-config --libs " + list_of_modules))) {
+		STDOUT.WriteLine("no");
+		return false;
+	}
+	eval(prefix + "_LIBS = out");
+	STDOUT.WriteLine("yes");
+	return true;
+}
+
+function PHP_EVAL_LIBLINE(libline, libs_variable, not_extension)
+{
+	libs_variable = libs_variable.toUpperCase();
+	var args = libline.split(/\s/);
+	for (var i = 0; i < args.length; i++) {
+		var arg = args[i];
+		if (arg.match(/^-l(.*)/)) {
+			ADD_FLAG("LIBS_" + libs_variable, RegExp.$1 + ".lib");
+		} else if (arg.match(/^-L(.*)/)) {
+			var path = condense_path(RegExp.$1);
+			ADD_FLAG("LDFLAGS_" + libs_variable, '/libpath:"' + path + '" ');
+			ADD_FLAG("ARFLAGS_" + libs_variable, '/libpath:"' + path + '" ');
+		}
+	}
+}
+
+function PHP_EVAL_INCLINE(headerline)
+{
+	var args = headerline.split(/\s/);
+	for (var i = 0; i < args.length; i++) {
+		var arg = args[i];
+		if (arg.match(/^-I(.*)/)) {
+			ADD_FLAG("CFLAGS", "/I " + condense_path(RegExp.$1));
+		}
+	}
+}
+
 /* XXX check whether some manifest was originally supplied, otherwise keep using the default. */
 function generate_version_info_manifest(makefiletarget)
 {

If we want to take this route, we should ship pkg-config with https://github.com/php/php-sdk-binary-tools (we can probably use https://github.com/skeeto/u-config). And the winlibs builds would need to actually contain .pc files. I don't think it's viable to rebuilt all php-src dependency winlibs right away, but when updating to new versions, the *.pc files could be added, and then php-src could be adapted to use them.

Thoughts?

@cmb69
Copy link
Member Author

cmb69 commented Nov 12, 2024

Hmm, not quite sure how we should approach the .pc files shipped with winlib builds.

  • libraries already building .pc files might cause issues with pkg-config variants/versions which always verify Requires.private (Prepend brew's pkgconfig to PKG_CONFIG_PATH #16741 (comment) ff)
  • it might not be worth doing proper .pc files for libraries not already building .pc files for Windows (spend quite a while on OpenSSL and Net-SNMP the other day)
  • PHP completely ignores static builds (i.e. the *.private lines)
  • PHP (i.e. PHP_EVAL_INCLINE and PHP_EVAL_LIBLINE) completely ignores anything but -I, -L and -l, and -pthread (might be correct wrt the item above)

So we could just roll out minimalist .pc files for our builds, not even catering to the "canonical" .pc names, and just have a single .pc file per package (i.e. not openssl.pc, libssl.pc and libcrypto.pc for OpenSSL, but only openssl.pc). That would save a lot of work (including rebuilding the packages), but might not be best for interoperability with custom dependency builds (although these may not be interoperable at all, see https://gitlab.freedesktop.org/pkg-config/pkg-config/-/merge_requests/13).


SETUP_OPENSSL("snmp", PHP_SNMP) >= 2) {

is a nice example why not using something like pkg-config is not the best idea. We're unconditionally requiring OpenSSL, even though Net-SNMP might have been built without OpenSSL support.

Perhaps better examples are ext/ssh2 and ext/curl, which also require OpenSSL, although either libssh2 and libcurl could be built with Schannel support instead.

Or ext/zip, which requires to have liblzma, and sets a couple of defines in the CFLAGS, so makes some strong assumptions about how libzip had been built. Wanting to use these flags from .pc files would be a point pro proper .pc files.

@cmb69
Copy link
Member Author

cmb69 commented Nov 13, 2024

Or ext/zip, which requires to have liblzma, and sets a couple of defines in the CFLAGS, so makes some strong assumptions about how libzip had been built.

Except for /D LZMA_API_STATIC (which might not even be needed), these defines are not something pkg-config (usually) would report. These flags are set whether some functions in the library exists, and these check are hard to replicate with our Windows AC compat layer, so hard-coding makes some sense (albeit these are still strong assumptions). But this is unrelated to this ticket.

Anyhow, I've build libzip as usual, and got the following libzip.pc:

prefix=C:/Program Files/libzip
exec_prefix=${prefix}
bindir=${prefix}/bin
libdir=${prefix}/lib
includedir=${prefix}/include

zipcmp=${bindir}/zipcmp

Name: libzip
Description: library for handling zip archives
Version: 1.11.2
Libs:  -L${libdir} -lzip
Libs.private:  -ladvapi32 -lbz2 -llzma -lbcrypt -lzlib_a
Cflags: -I${includedir}

That is pretty accurate (besides that -lzip won't work since we're calling it libzip_a.lib; likely an insufficient patch of winlibs), but how would PHP (or any other client) know whether to build against a static or shared library (in this case there is no shared library at all)? Note again that PHP ever only calls PKG_CHECK_MODULES (what assumes shared libraries); that might be appropriate for most systems, but on Windows we have quite a couple of static library dependencies. Why? Maybe for BC reasons, maybe for convenience (to avoid shipping many more DLLs which would not easily be found by Webserver modules), and sometimes likely to avoid problems with libraries wich are not fully thread-safe (e.g. Net-SNMP has init and shutdown routines; cf. libxml2; not sure that the commit message is accurate, but multi-threading can indeed be a problem).

So if we want to use pkg-config on Windows (and I still think this is a good idea for the reasons given in the OP), we should just roll our own .pc files. After all, it's not hard to manually craft (and some automation might still be possible) something like:

prefix=C:/usr
libdir=${prefix}/lib
includedir=${prefix}/include

Name: libzip
Description: winlibs' libzip package
Version: 1.11.2
Requires: libbzip2 zlib liblzma
Libs: -L${libdir} -llibzip_a
Cflags: -I${includedir}

Custom dependency builds could copy these and adapt if needed. And downstream consumers other than PHP (if there are any) would likely not be affected, since we've shipped no .pc files so far, so they probably don't use pkg-config at all.

@cmb69 cmb69 linked a pull request Nov 16, 2024 that will close this issue
@NattyNarwhal
Copy link
Member

FWIW, my nitpick here is suggesting pkgconf instead of u-config. It's portable and maintained (unlike fdo p-c), and has been around for years so it should be well tested with libraries (unlike u-c).

@cmb69
Copy link
Member Author

cmb69 commented Nov 18, 2024

I don't mind which pkg-config variant we use here. u-config was just trivial to build (CMake), while pkgconf would require Meson (not a big deal, though).

and has been around for years so it should be well tested with libraries

This is likely irrelevant here, because we're probably not using *.pc files built by packages (of the 37 core dependencies, only about 6 already produce *.pc files), and we cannot necessarily use these, because:

Note again that PHP ever only calls PKG_CHECK_MODULES (what assumes shared libraries); that might be appropriate for most systems, but on Windows we have quite a couple of static library dependencies.

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.

2 participants