RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v2]

[Erik Joelsson]

On Fri, 5 Apr 2024 14:14:35 GMT, Magnus Ihse Bursie <ihse at 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