On Fri, 5 Apr 2024 14:14:35 GMT, Magnus Ihse Bursie <i...@openjdk.org> 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<correct >> path>` or `jawt.lib -libpath:<correct path>`, 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 <module>:<directory>, > 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 <module>:<basename>. 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
