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

[Severin Gehwolf]

On Thu, 21 Mar 2024 14:54:15 GMT, Magnus Ihse Bursie <ihse at openjdk.org> wrote:

>> Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Move CreateLinkableRuntimePlugin to build folder
>>   
>>   Keep runtime link supporting classes in package
>>   jdk.tools.jlink.internal.runtimelink
>
> make/Images.gmk line 124:
> 
>> 122: 
>> 123: ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true)
>> 124: 
> 
> Please avoid newlines after if statements.
> 
> Suggestion:

OK.

> make/Images.gmk line 131:
> 
>> 129:   # in FixPath call in order to avoid needing to use strip.
>> 130:   RL_JIMAGE_PATH_ARG := $(call FixPath,$(JDK_LINK_OUTPUT_DIR)/lib/modules)
>> 131:   RL_MOD_PATH_ARG := $(call FixPath,$(IMAGES_OUTPUTDIR)/jmods)
> 
> I'd much rather prefer to use strip and have proper space after commas. This is the approach taken elsewhere in the build system.
> 
> Suggestion:
> 
>   RL_JIMAGE_PATH_ARG := $(strip $(call FixPath, $(JDK_LINK_OUTPUT_DIR)/lib/modules))
>   RL_MOD_PATH_ARG := $(strip $(call FixPath, $(IMAGES_OUTPUTDIR)/jmods))

Thanks! This will also likely change. I'm thinking of just generating the diff file and putting it into `lib/` of the JDK image. That avoids needing to call this build-time only jlink plugin and this `FixPath` magic.

> make/Images.gmk line 145:
> 
>> 143:   $(eval $(call SetupJavaCompilation, BUILD_JDK_RUNLINK_CLASSES, \
>> 144:       COMPILER := buildjdk, \
>> 145:       DISABLED_WARNINGS := try, \
> 
> Why do we get warnings in the java code?

That's not needed anymore. There are some `try` warnings in the `JmodsReader` and `JimageDiffGenerator` classes which used to get compiled with this. It'll probably change again...

> make/Images.gmk line 171:
> 
>> 169: 
>> 170:   JLINK_JDK_TARGETS += $(jlink_runtime_jdk)
>> 171: 
> 
> Please avoid newlines before endif statements.
> 
> Suggestion:

OK. Will fix.

> First question, do this class really need to be in a separate module? (I'm afraid the answer is "yes" but I need to ask it anyway).

Yes, because it uses the `Plugin` ServiceLoader extension using the boot ModuleLayer. But it's going to be a moot point because this'll get reworked due to concerns raised by @mlchung (having the plugin pipeline running when producing a linkable runtime jimage).

> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/JimageDiffGenerator.java line 36:
> 
>> 34: 
>> 35: /**
>> 36:  * Used by the build-only jlink plugin CreateLinkableRuntimePlugin.
> 
> Why does this file not live next to the jlink plugin then? Does it have to be part of the jdk.jlink module?

The idea was to have the "supporting" classes inside jdk.jlink to keep code-bases in sync. But the actual invocation of them should be in the build code. This will likely change again.

> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/JimageDiffGenerator.java line 40:
> 
>> 38: public class JimageDiffGenerator {
>> 39: 
>> 40:     private static final boolean DEBUG = false;
> 
> This seems like left-over debug code. If this should go into product code I suggest you either remove it, or alternatively make it possible to change at runtime, if the debug functionality will be needed.

OK.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534137541
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534141577
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534148956
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534137294
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534136264
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534154082
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534151066