RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]

[Severin Gehwolf]

On Fri, 8 Mar 2024 17:25:18 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> make/Images.gmk line 96:
>> 
>>> 94: 
>>> 95: ifeq ($(JLINK_KEEP_PACKAGED_MODULES), true)
>>> 96:   ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true)
>> 
>> I don't get it. Why don't you use the  JDK_LINK_OUTPUT_DIR from just above?
>
> Makes sense. I can fold both `JLINK_JDK_EXTRA_OPTS` into one. `JLINK_JDK_EXTRA_OPTS := --keep-packaged-modules $(JDK_LINK_OUTPUT_DIR)/jmods`

Fixed now.

>> make/Images.gmk line 104:
>> 
>>> 102: endif
>>> 103: 
>>> 104: 
>> 
>> Is this extra line intentional?
>
> No. I can remove it. Thanks.

Should be gone.

>> make/Images.gmk line 128:
>> 
>>> 126:   JDK_RUN_TIME_IMAGE_SUPPORT_DIR := $(SUPPORT_OUTPUTDIR)/images/jlink-run-time
>>> 127:   JDK_JMODS_DIFF := $(JDK_RUN_TIME_IMAGE_SUPPORT_DIR)/jimage_packaged_modules_diff.data
>>> 128:   JLINK_RUNTIME_CREATE_EXTRA += --create-linkable-runtime=$(JDK_JMODS_DIFF)
>> 
>> My understanding is that the `--create-linkable-runtime` is the primary argument for the jlink invocation in jlink_runtime_jdk. As such, I think it would be much better if this was inlined in place in the jlink_runtime_jdk COMMAND. The "extra" makes it sound like it is optional. 
>> 
>> In contrast, the value of JLINK_RUNTIME_CREATE_EXTRA that is set above for JLINK_KEEP_PACKAGED_MODULES is indeed "extra".
>
> Your understanding is correct. Will fix.

Should be fixed in the update.

>> make/ToolsJdk.gmk line 88:
>> 
>>> 86:     --add-modules=jdk.jlink --add-exports=java.base/jdk.internal.module=ALL-UNNAMED \
>>> 87:     --add-exports=java.base/jdk.internal.jimage=ALL-UNNAMED \
>>> 88:     build.tools.runtimelink.JimageDiffGenerator
>> 
>> While it might be a bit redundant, we try to keep the same name in the make name, the package and the main class, e.g. something like:
>> 
>> Suggestion:
>> 
>> TOOL_JIMAGE_DIFF_GENERATOR = $(BUILD_JAVA_SMALL) -cp $(BUILDTOOLS_OUTPUTDIR)/jdk_tools_classes \
>>     --add-modules=jdk.jlink --add-exports=java.base/jdk.internal.module=ALL-UNNAMED \
>>     --add-exports=java.base/jdk.internal.jimage=ALL-UNNAMED \
>>     build.tools.jimagediffgenerator.JimageDiffGenerator
>> 
>> 
>> This is of course not consistently followed, but for new tooling I think it would be a good idea to try and follow.
>
> Based on some private feedback I've got it seems this will change a bit (move to the `jdk.jlink` module instead). But if still relevant, I'll keep that in mind. Thanks!

The latest update does the compilation in `make/Images.gmk` and doesn't use the tool approach any more. This should be moot.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1524960817
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1524961177
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1524960431
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1524964174