On Fri, 8 Mar 2024 16:52:33 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Only show runtime image suffix for JDK modules > > 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` > make/Images.gmk line 104: > >> 102: endif >> 103: >> 104: > > Is this extra line intentional? No. I can remove it. Thanks. > 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1518027506 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1518028421 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1518030091
