Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v2]
On Wed, 10 Apr 2024 13:41:53 GMT, Magnus Ihse Bursie wrote: >> make/common/JdkNativeCompilation.gmk line 206: >> >>> 204: $$(eval $$(call ResolveLibPath,$1,$2)) >>> 205: >>> 206: $1_EXTRA_HEADER_DIRS += $$($1_$2_MODULE):lib$$($1_$2_NAME) >> >> I think the top level comment need to be clearer about JDK_LIB implicitly >> setting EXTRA_HEADER_DIRS. > > It is documented on line 175, for AddJdkLibrary. I added it to > SetupJdkNativeCompilation as well, not sure if that was what you meant. That is what I meant, as that is the public API. - PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1559473879
Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v2]
On Fri, 5 Apr 2024 18:32:54 GMT, Erik Joelsson wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix libfallbackLinker > > make/common/JdkNativeCompilation.gmk line 206: > >> 204: $$(eval $$(call ResolveLibPath,$1,$2)) >> 205: >> 206: $1_EXTRA_HEADER_DIRS += $$($1_$2_MODULE):lib$$($1_$2_NAME) > > I think the top level comment need to be clearer about JDK_LIB implicitly > setting EXTRA_HEADER_DIRS. It is documented on line 175, for AddJdkLibrary. I added it to SetupJdkNativeCompilation as well, not sure if that was what you meant. - PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1559462557
Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v2]
On Fri, 5 Apr 2024 14:14:35 GMT, Magnus Ihse Bursie wrote: >> This is the pinnacle of the recent stream of refactorings in the build >> system. This patch introduces a more abstract concept of "JDK_LIBS", where >> only the library name (e.g. "java" or "java.desktop:jawt") is specified, and >> the build system turns this into suitable linker flags: `-ljawt -L> path>` or `jawt.lib -libpath:`, depending on linker. It will >> also automatically create proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix libfallbackLinker This is a nice improvement! make/common/JdkNativeCompilation.gmk line 107: > 105: ) > 106: > 107: # Create a proper LIBPATH for the given library. Suggestion: # Create a proper LIBPATH for the given library. Sets result in $1_$2_LIBPATH. make/common/JdkNativeCompilation.gmk line 139: > 137: else > 138: $1_$2_JVM_VARIANT_PATH := $(JVM_VARIANT_MAIN) > 139: endif I think this needs to be moved before lin 127. make/common/JdkNativeCompilation.gmk line 206: > 204: $$(eval $$(call ResolveLibPath,$1,$2)) > 205: > 206: $1_EXTRA_HEADER_DIRS += $$($1_$2_MODULE):lib$$($1_$2_NAME) I think the top level comment need to be clearer about JDK_LIB implicitly setting EXTRA_HEADER_DIRS. make/common/JdkNativeCompilation.gmk line 262: > 260: # be specified either as an absolute path, or relative directory > names which > 261: # are preprocessed like SRC, or in the format :, > which > 262: # will be processed like SRC but for the given module. The name > "*:libjvm" When I first read this, it wasn't obvious that the `*` was actually a wildcard. Looking through all uses of :libjvm below, you are always doing `java.base:libjvm`. Is there a need to allow any prefix for the special libjvm case, or should we just enforce `java.base:libjvm` to maybe make things clearer? make/common/JdkNativeCompilation.gmk line 269: > 267: # These take the form :. If the module is > java.base, > 268: # the entire java.base: prefix can be omitted. If the module is @, > this will > 269: # be replaced with the current module. The gtest module is a virtual > module I understand wanting to optimize for the common case, which is linking against libjava and libjvm in java.base, but I'm wondering if it wouldn't be better to be more coherent with the EXTRA_HEADER_DIRS argument. No prefix there means current module, while no prefix here means java.base. I think we will be tripping over this inconsistency in the future. I also note that headers are referred to as `libfoo` while libraries are only `foo`. This is harder to do anything about as on references a directory name, while the other is the build library. Still something to consider. It would be neat if these options could take the same syntax when appropriate. make/common/JdkNativeCompilation.gmk line 275: > 273: # EXTRA_RCFLAGS -- additional RCFLAGS to append. > 274: # RC_FILEDESC -- override the default FILEDESC for Windows version.rc > 275: # such as "java.base:headers". I don't think this line belongs here. It doesn't make sense for RC_FILEDESC. Probably got misplaced in an earlier patch. make/modules/jdk.internal.le/Lib.gmk line 39: > 37: EXTRA_HEADER_DIRS := \ > 38: java.base:libjava \ > 39: java.base:libjvm, \ Indentation. - PR Review: https://git.openjdk.org/jdk/pull/18649#pullrequestreview-1983816521 PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554098311 PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554108796 PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554115371 PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554066430 PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554070533 PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554073121 PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554085354
Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v2]
On Fri, 5 Apr 2024 14:14:35 GMT, Magnus Ihse Bursie wrote: >> This is the pinnacle of the recent stream of refactorings in the build >> system. This patch introduces a more abstract concept of "JDK_LIBS", where >> only the library name (e.g. "java" or "java.desktop:jawt") is specified, and >> the build system turns this into suitable linker flags: `-ljawt -L> path>` or `jawt.lib -libpath:`, depending on linker. It will >> also automatically create proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix libfallbackLinker I apologize for bouncing this one back and forth between open and draft. I thought I had verified everything but then I discovered two missing includes on Windows. And then GHA failed on linux-x86 for libfallbackLinker. Now these are fixed; I have confirmed that Windows builds properly on Oracle's CI system, and that libfallbackLinker works locally on a linux-x86 build, so I'm just waiting for GHA to confirm this. I have also run tier1 + tier2 on Oracle's CI system for windows x64, linux x64/aarch64 and macos x64/aarch64, without failures. (The Windows run is still waiting for a few tests to finish.) Furthermore I have checked with COMPARE_BUILD, and the only differences I can see is related to library linking: On macOS and Linux, most libraries linked with "-ljvm -ljava" as default before, regardless of if they were needed or not. Now we only link with these libraries if needed, so this changes the bit-by-bit comparison for almost all libraries. On Windows, there was just a few libraries that had dependency changes. As far as I can tell, all changes discovered were related to the change in dynamic library dependencies. - PR Comment: https://git.openjdk.org/jdk/pull/18649#issuecomment-2039920478
Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v2]
> This is the pinnacle of the recent stream of refactorings in the build > system. This patch introduces a more abstract concept of "JDK_LIBS", where > only the library name (e.g. "java" or "java.desktop:jawt") is specified, and > the build system turns this into suitable linker flags: `-ljawt -L path>` or `jawt.lib -libpath:`, depending on linker. It will > also automatically create proper dependencies. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Fix libfallbackLinker - Changes: - all: https://git.openjdk.org/jdk/pull/18649/files - new: https://git.openjdk.org/jdk/pull/18649/files/cdee250d..6e737795 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18649=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18649=00-01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18649.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18649/head:pull/18649 PR: https://git.openjdk.org/jdk/pull/18649
