On Tue, 19 Mar 2024 16:55:14 GMT, Severin Gehwolf <sgehw...@openjdk.org> wrote:

>> Please review this patch which adds a jlink mode to the JDK which doesn't 
>> need the packaged modules being present. A.k.a run-time image based jlink. 
>> Fundamentally this patch adds an option to use `jlink` even though your JDK 
>> install might not come with the packaged modules (directory `jmods`). This 
>> is particularly useful to further reduce the size of a jlinked runtime. 
>> After the removal of the concept of a JRE, a common distribution mechanism 
>> is still the full JDK with all modules and packaged modules. However, 
>> packaged modules can incur an additional size tax. For example in a 
>> container scenario it could be useful to have a base JDK container including 
>> all modules, but without also delivering the packaged modules. This comes at 
>> a size advantage of `~25%`. Such a base JDK container could then be used to 
>> `jlink` application specific runtimes, further reducing the size of the 
>> application runtime image (App + JDK runtime; as a single image *or* 
>> separate bundles, depending on the app 
 being modularized).
>> 
>> The basic design of this approach is to add a jlink plugin for tracking 
>> non-class and non-resource files of a JDK install. I.e. files which aren't 
>> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` 
>> class which has all the info of what constitutes the final jlinked runtime.
>> 
>> Basic usage example:
>> 
>> $ diff -u <(./bin/java --list-modules --limit-modules java.se) 
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules 
>> --limit-modules java.se)
>> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) 
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules 
>> --limit-modules jdk.jlink)
>> $ ls ../linux-x86_64-server-release/images/jdk/jmods
>> java.base.jmod            java.net.http.jmod       java.sql.rowset.jmod      
>> jdk.crypto.ec.jmod         jdk.internal.opt.jmod                     
>> jdk.jdi.jmod         jdk.management.agent.jmod  jdk.security.auth.jmod
>> java.compiler.jmod        java.prefs.jmod          java.transaction.xa.jmod  
>> jdk.dynalink.jmod          jdk.internal.vm.ci.jmod                   
>> jdk.jdwp.agent.jmod  jdk.management.jfr.jmod    jdk.security.jgss.jmod
>> java.datatransfer.jmod    java.rmi.jmod            java.xml.crypto.jmod      
>> jdk.editpad.jmod           jdk.internal.vm.compiler.jmod             
>> jdk.jfr.jmod         jdk.management.jmod        jdk.unsupported.desktop.jmod
>> java.desktop.jmod         java.scripting.jmod      java.xml.jmod             
>> jdk.hotspot.agent.jmod     jdk.i...
>
> 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:

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))

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?

make/Images.gmk line 171:

> 169: 
> 170:   JLINK_JDK_TARGETS += $(jlink_runtime_jdk)
> 171: 

Please avoid newlines before endif statements.

Suggestion:

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534080047
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534079021
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534082359
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534080744

Reply via email to