On Mon, 21 Apr 2025 17:28:12 GMT, Jay Bhaskar <jbhas...@openjdk.org> wrote:
>> The upgrade is required to address several known issues from the previous >> version to improve stability and performance. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > remove unused files I left some inline feedback on the libxml2 update. I provided one more example of a libxml2 file that was not new in the 2.13.x release, but which was added as part of this PR (there are 30 such files. I will list them in a separate comment). modules/javafx.web/src/main/native/Source/ThirdParty/libxml/linux/config.h line 29: > 27: > 28: /* getentropy */ > 29: #define HAVE_GETENTROPY 0 The pattern in `config.h` is to comment out the define for an unused feature, not to define it to 0. This is important because most of the conditional defines are checked using `#ifdef` or `#if defined`, and those macros don't care what the value is. This needs to be fixed. If this feature is now needed, the define it as follows: /* Define to 1 if you have the 'getentropy' function. */ #define HAVE_GETENTROPY 1 else comment it out as follows: /* Define to 1 if you have the 'getentropy' function. */ /* #undef HAVE_GETENTROPY */ modules/javafx.web/src/main/native/Source/ThirdParty/libxml/linux/config.h line 67: > 65: > 66: /* Define to 1 if you have the <poll.h> header file. */ > 67: /* #undef HAVE_POLL_H */ Is this no longer needed? modules/javafx.web/src/main/native/Source/ThirdParty/libxml/linux/config.h line 97: > 95: > 96: /* Define to 1 if you have the <sys/random.h> header file. */ > 97: #define HAVE_SYS_RANDOM_H 0 This also needs to be fixed (either define it to 1 or else comment it out). modules/javafx.web/src/main/native/Source/ThirdParty/libxml/linux/include/libxml/xmlversion.h line 322: > 320: * the string suffix used by dynamic modules (usually shared libraries) > 321: */ > 322: #define LIBXML_MODULE_EXTENSION ".so This looks wrong. It would almost certainly fail to compile if the code path were active. You might want to fix it, although it is unused. modules/javafx.web/src/main/native/Source/ThirdParty/libxml/mac/config.h line 29: > 27: > 28: /* getentropy */ > 29: #define HAVE_GETENTROPY 1 I think the comment should be: /* Define to 1 if you have the 'getentropy' function. */ Can you check this? modules/javafx.web/src/main/native/Source/ThirdParty/libxml/mac/include/xmlversion.h line 1: > 1: /* This file is not used and should be deleted. It looks like a spurious copy of `mac/include/libxml/xmlversion.h`. modules/javafx.web/src/main/native/Source/ThirdParty/libxml/src/CMakeLists.txt line 1: > 1: cmake_minimum_required(VERSION 3.18) I don't think we use this file in our build. We should remove it if not. modules/javafx.web/src/main/native/Source/ThirdParty/libxml/src/SAX.c line 2: > 1: /* > 2: * SAX.c : Old SAX v1 handlers to build a tree. Here is an example of what I mentioned earlier: This is a file that we didn't used to take from the upstream. I don't think we use it or need it. modules/javafx.web/src/main/native/Source/ThirdParty/libxml/src/dict.c line 933: > 931: #else > 932: // This block will compile on macOS (and any non-Linux system) if > HAVE_GETENTROPY is defined > 933: #if defined(HAVE_GETENTROPY) && !defined(__linux__) Can you check whether this is needed if `HAVE_GETENTROPY` is left undefined? I suspect it will no longer be necessary, and it would be better not to have local mods to upstream files. If a modification _is_ needed, then we will need a clear comment with the changes, noting that this is a JavaFX-specific addition. modules/javafx.web/src/main/native/Source/ThirdParty/libxml/src/tree.c line 5862: > 5860: * > 5861: * Merge the second text node into the first. The second node is > 5862: * unlinked and freed. It looks like you missed applying one of the changes from 2.13.7. Replace the above two lines with: * Merge the second text node into the first. If @first is NULL, * @second is returned. Otherwise, the second node is unlinked and * freed. ------------- PR Review: https://git.openjdk.org/jfx/pull/1785#pullrequestreview-2781990520 PR Review Comment: https://git.openjdk.org/jfx/pull/1785#discussion_r2052845497 PR Review Comment: https://git.openjdk.org/jfx/pull/1785#discussion_r2052864462 PR Review Comment: https://git.openjdk.org/jfx/pull/1785#discussion_r2052861880 PR Review Comment: https://git.openjdk.org/jfx/pull/1785#discussion_r2052866676 PR Review Comment: https://git.openjdk.org/jfx/pull/1785#discussion_r2052869329 PR Review Comment: https://git.openjdk.org/jfx/pull/1785#discussion_r2052877696 PR Review Comment: https://git.openjdk.org/jfx/pull/1785#discussion_r2052878445 PR Review Comment: https://git.openjdk.org/jfx/pull/1785#discussion_r2052926586 PR Review Comment: https://git.openjdk.org/jfx/pull/1785#discussion_r2052935612 PR Review Comment: https://git.openjdk.org/jfx/pull/1785#discussion_r2052940458
