On Thu, 21 Mar 2024 14:54:15 GMT, Magnus Ihse Bursie <i...@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
