On Fri, 8 Mar 2024 17:25:18 GMT, Severin Gehwolf <sgehw...@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
