Fix GH-15534: Build failure for libxml2 2.9.0 - 2.9.3 #15536

petk · 2024-08-22T00:31:40Z

The xmlDictPtr was moved before the includes in libxml2 2.9.4 so the <libxml/dict.h> can be included directly but for earlier versions the <libxml/tree.h> needs to be included before. Since PHP requires libxml2 2.9.0 or later and this also fixes builds on Solaris 10.

nielsdos · 2024-08-22T06:18:22Z

Honestly, I think support for these old libxml versions should just be dropped. The early 2.9.x versions have quite some memory corruption bugs and they're also 8 years old.
It's also a version that no longer compiles our of the box on my system, so I can't test it easily.
However, the patch seems right.

petk · 2024-08-22T14:31:05Z

Yes, installing all these various versions is very cumbersome. I have major issues installing somehow "fresher" libxml2 version on Solaris 10 :D

petk · 2024-08-22T15:16:36Z

Then, let's try bumping the minimum version to 2.9.4 as noted in the issue by @cmb69

I've appended a new commit then.

cmb69 · 2024-08-22T15:39:40Z

If we bump the requirement, we should check that on Windows, too (just in case somebody actually tries to build with libxml2 < 2.9.4). If all else fails, we could use GREP_HEADER(), although I need to check out the details. But that can be done after this PR is merged.

I would suggest to wait a bit with merging, though (PHP 8.4.0beta4 is only next week).

petk · 2024-08-22T15:51:39Z

In <libxml/xmlversion.h> there is LIBXML_DOTTED_VERSION that can be perhaps used for the check on Windows with GREP_HEADER. For example, CMake's FindLibXml2 module also checks that as the main source of the correct version.

cmb69 · 2024-08-22T17:03:48Z

The following should do

 ext/libxml/config.w32 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/ext/libxml/config.w32 b/ext/libxml/config.w32
index 416ae1f136..d836f0efcb 100644
--- a/ext/libxml/config.w32
+++ b/ext/libxml/config.w32
@@ -9,13 +9,19 @@ if (PHP_LIBXML == "yes") {
 			CHECK_HEADER_ADD_INCLUDE("libxml/tree.h", "CFLAGS_LIBXML", PHP_PHP_BUILD + "\\include\\libxml2") &&
 			ADD_EXTENSION_DEP('libxml', 'iconv')) {
 
-		EXTENSION("libxml", "libxml.c mime_sniff.c", false /* never shared */, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
-		AC_DEFINE("HAVE_LIBXML", 1, "Define to 1 if the PHP extension 'libxml' is available.");
-		ADD_FLAG("CFLAGS_LIBXML", "/D LIBXML_STATIC /D LIBXML_STATIC_FOR_DLL /D HAVE_WIN32_THREADS ");
-		if (!PHP_LIBXML_SHARED) {
-			ADD_DEF_FILE("ext\\libxml\\php_libxml2.def");
+		if (GREP_HEADER("libxml/xmlversion.h", "#define\\s+LIBXML_VERSION\\s+(\\d+)", PHP_PHP_BUILD + "\\include\\libxml2") &&
+				+RegExp.$1 >= 20904) {
+
+			EXTENSION("libxml", "libxml.c mime_sniff.c", false /* never shared */, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
+			AC_DEFINE("HAVE_LIBXML", 1, "Define to 1 if the PHP extension 'libxml' is available.");
+			ADD_FLAG("CFLAGS_LIBXML", "/D LIBXML_STATIC /D LIBXML_STATIC_FOR_DLL /D HAVE_WIN32_THREADS ");
+			if (!PHP_LIBXML_SHARED) {
+				ADD_DEF_FILE("ext\\libxml\\php_libxml2.def");
+			}
+			PHP_INSTALL_HEADERS("ext/libxml", "php_libxml.h");
+		} else {
+			WARNING("libxml support can't be enabled, libxml version >= 2.9.4 required");
 		}
-		PHP_INSTALL_HEADERS("ext/libxml", "php_libxml.h");
 	} else {
 		WARNING("libxml support can't be enabled, iconv or libxml are missing")
 		PHP_LIBXML = "no"

Feel free to commit this @petk.

(Isn't that a wonderful programming language? No need for ugly return values; you just can get the matches from a nice global variable. ;)

nielsdos · 2024-08-22T17:28:17Z

I don't know if our process "allows" bumping the version this late into the 8.4 development cycle, but if we can then fine by me.
This means less work for me as I have to watch out for fewer libxml bugs (PHP already implements some workarounds for libxml bugs, and I hate that we have to do it)

petk · 2024-08-22T17:37:06Z

(Isn't that a wonderful programming language? No need for ugly return values; you just can get the matches from a nice global variable. ;)

Hehehe. :D

Ok, then Windows changes added in the appended commit. Thanks @cmb69

Edit: Patch also seems to be working fine on Windows.

nielsdos

Thanks @cmb69 and @petk !

petk · 2024-08-23T21:02:01Z

Just wondering, here we're still waiting or can this be merged now already? :D

nielsdos · 2024-08-23T21:07:28Z

IMO, this can be merged. Feel free to do so!

Jan-E · 2024-08-28T13:43:37Z

ext/libxml/config.w32

-		ADD_FLAG("CFLAGS_LIBXML", "/D LIBXML_STATIC /D LIBXML_STATIC_FOR_DLL /D HAVE_WIN32_THREADS ");
-		if (!PHP_LIBXML_SHARED) {
-			ADD_DEF_FILE("ext\\libxml\\php_libxml2.def");
+		if (GREP_HEADER("libxml/xmlversion.h", "#define\\s+LIBXML_VERSION\\s+(\\d+)", PHP_PHP_BUILD + "\\include\\libxml2") &&


@petk @cmb69 This change assumes that xmlversion.h is in include/libxml2/libxml/xmlversion.h which is most likely not the case. The most likely place for xmlversion.h is include/libxml. With LIBXML_VERSION 20914 I got the warning that I did not have the required libxml version.
Dropping the \\libxml2 in the last argument of GREP_HEADER did the trick. Or dropping the libxml/ in the first argument and putting the headers in include/libxml2.

I see. The GREP_HEADER function should be adjusted a bit, yes. Because the CHECK_HEADER_ADD_INCLUDE checks the include flags by appending them to the other flags, and this one works by a single file path only. It already checks the flags but only the empty PHP_EXTRA_INCLUDES variable (probably)...

CHECK_HEADER_ADD_INCLUDE is smart enough to first check in the main include directory. With the headers in include/libxml it will find the headers. In my case:

Checking for libxml/parser.h ... ../win64build\include
Checking for libxml/tree.h ... ../win64build\include

for the x64 build

PHP_PHP_BUILD + "\\include\\libxml2" in CHECK_HEADER_ADD_INCLUDE has been there for ages. I can see it there in PHP 7.0. In PHP 5.6 it simply was

CHECK_HEADER_ADD_INCLUDE("libxml/parser.h", "CFLAGS_LIBXML")

I'm testing something like this now:

diff --git a/win32/build/confutils.js b/win32/build/confutils.js index 95a4e5ce3c..b652a497d1 100644 --- a/win32/build/confutils.js +++ b/win32/build/confutils.js @@ -965,6 +965,11 @@ function GREP_HEADER(header_name, regex, path_to_check) if (!c) { /* look in the include path */ + if (path_to_check == null) { + path_to_check = php_usual_include_suspects; + } else { + path_to_check += ";" + php_usual_include_suspects; + } var p = search_paths(header_name, path_to_check, "INCLUDE"); if (typeof(p) == "string") {

This change assumes that xmlversion.h is in include/libxml2/libxml/xmlversion.h which is most likely not the case. The most likely place for xmlversion.h is include/libxml. With LIBXML_VERSION 20914 I got the warning that I did not have the required libxml version.

From where did you get these libraries? I've checked a couple from windows.php.net, and even one from http://xmlsoft.org/sources/win32/64bit/, and there is always include/libxml2/libxml/*.h.

I do not have my build machine at hand, but I probably just used https://github.com/winlibs/libxml2

https://windows.php.net/downloads/php-sdk/deps/archives/vc11/x64/libxml2-2.9.4-1-vc11-x64.zip
And updates after that.

Thanks for digging this out. So we need to fix this for best compatibility with other builds. I'll have a look at @petk's PR (thanks for that!) soon (probably the weekend).

Thanks for digging this out. So we need to fix this for best compatibility with other builds. I'll have a look at @petk's PR (thanks for that!) soon (probably the weekend).

@cmb69 Could you review the PR so that it can be merged?

petk requested a review from nielsdos as a code owner August 22, 2024 00:31

github-actions bot added the Extension: dom label Aug 22, 2024

nielsdos approved these changes Aug 22, 2024

View reviewed changes

petk linked an issue Aug 22, 2024 that may be closed by this pull request

Build with libxml2 versions 2.9.0 to 2.9.3 fails #15534

Closed

Autotools: Bump minimum libxml2 version to 2.9.4

56237da

github-actions bot added the Category: Build System label Aug 22, 2024

Remove redundant include file

55ca034

[skip ci] Simplify UPGRADING note

ca8bd23

Check libxml2 version on Windows

ffd8ab5

github-actions bot added the Extension: libxml label Aug 22, 2024

nielsdos approved these changes Aug 22, 2024

View reviewed changes

petk merged commit dc8f18a into php:master Aug 23, 2024
10 checks passed

petk deleted the patch-libxml-includes-15534 branch August 23, 2024 21:14

Jan-E reviewed Aug 28, 2024

View reviewed changes

petk mentioned this pull request Aug 28, 2024

Fix GREP_HEADER on Windows #15619

Closed

cmb69 mentioned this pull request Nov 11, 2024

Use pkg-config on Windows #16752

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GH-15534: Build failure for libxml2 2.9.0 - 2.9.3 #15536

Fix GH-15534: Build failure for libxml2 2.9.0 - 2.9.3 #15536

petk commented Aug 22, 2024

nielsdos commented Aug 22, 2024

petk commented Aug 22, 2024

petk commented Aug 22, 2024 •

edited

Loading

cmb69 commented Aug 22, 2024

petk commented Aug 22, 2024

cmb69 commented Aug 22, 2024

nielsdos commented Aug 22, 2024

petk commented Aug 22, 2024 •

edited

Loading

nielsdos left a comment

petk commented Aug 23, 2024

nielsdos commented Aug 23, 2024 •

edited

Loading

Jan-E Aug 28, 2024 •

edited

Loading

petk Aug 28, 2024

Jan-E Aug 28, 2024

Jan-E Aug 28, 2024

petk Aug 28, 2024

cmb69 Aug 28, 2024

Jan-E Aug 28, 2024

Jan-E Aug 28, 2024

cmb69 Aug 28, 2024

Jan-E Sep 20, 2024

Fix GH-15534: Build failure for libxml2 2.9.0 - 2.9.3 #15536

Fix GH-15534: Build failure for libxml2 2.9.0 - 2.9.3 #15536

Conversation

petk commented Aug 22, 2024

nielsdos commented Aug 22, 2024

petk commented Aug 22, 2024

petk commented Aug 22, 2024 • edited Loading

cmb69 commented Aug 22, 2024

petk commented Aug 22, 2024

cmb69 commented Aug 22, 2024

nielsdos commented Aug 22, 2024

petk commented Aug 22, 2024 • edited Loading

nielsdos left a comment

Choose a reason for hiding this comment

petk commented Aug 23, 2024

nielsdos commented Aug 23, 2024 • edited Loading

Jan-E Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petk commented Aug 22, 2024 •

edited

Loading

petk commented Aug 22, 2024 •

edited

Loading

nielsdos commented Aug 23, 2024 •

edited

Loading

Jan-E Aug 28, 2024 •

edited

Loading