RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v40]

[Severin Gehwolf]

On Fri, 25 Oct 2024 19:12:55 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> Severin Gehwolf has updated the pull request incrementally with five additional commits since the last revision:
>> 
>>  - Better handle patched modules
>>    
>>    Also add a test which ensures that module patching (if present), will
>>    get an appropriate error message.
>>  - Add /othervm to some langtools tier1 tests
>>    
>>    Those tests are using module patches from JTREG. Since the run-time
>>    image based link uses ModuleFinder.ofSystem(), which will see the extra
>>    classes comming from the module patch. Then the size look-up using the
>>    JRT FS provider fails, since that only looks in the module image
>>    (lib/modules) and NOT also in the patch. Thus, we get a
>>    NoSuchFileException and the link fails.
>>    
>>    Run the tests with othervm so that the JTREG patch'ed module isn't
>>    visible to the test.
>>  - Fix tests for builds with --enable-linable-runtime
>>    
>>    Those builds don't include the packaged modules, `jmods` directory.
>>    However, some tests assume that they're there. Add appropriate requires
>>    tag.
>>  - Fix provider verification when some JMODs are present
>>    
>>    In some configurations, e.g. when java.base is missing from the packaged
>>    modules, but another JDK module is present as JMOD that is linked into
>>    an image, then provider verification can fail. Thus, the run-time image
>>    link fails. Verify that this doesn't happen.
>>    
>>    The fix is to return Platform.runtime() for run-time image based links
>>    as well. Otherwise this code would return the wrong result.
>>  - Show run-time image based capability in help
>>    
>>    Also add a test for it when it's turned on and off.
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 414:
> 
>> 412:         }
>> 413:         // Setup and init actions for JDK linkable runtimes
>> 414:         LinkableRuntimesResult result = linkableJDKRuntimesInit(finder, roots);
> 
> I think it's clearer to the readers if inlining the code of `linkablejDKRuntimesInit` here.  Also the `--keep-package-modules` check can be moved after `isLinkFromRuntime` is determined so checking `isLinkFromRuntime` is more explicit.
> 
> Suggestion:
> 
>         boolean isLinkFromRuntime = options.modulePath.isEmpty();
>         // In case of custom modules outside the JDK we may
>         // have a non-empty module path, which must not include
>         // java.base. If it did, we link using packaged modules from that
>         // module path. If the module path does not include java.base, we must
>         // have a linkable JDK runtime. In that case we take the JDK modules
>         // from the run-time image.
>         if (finder.find("java.base").isEmpty()) {
>             isLinkFromRuntime = true;
>             ModuleFinder runtimeImageFinder = ModuleFinder.ofSystem();
>             finder = combinedFinders(runtimeImageFinder, finder, options.limitMods, roots);
>         }
> 
>         // --keep-packaged-modules doesn't make sense as we are not linking
>         // from packaged modules to begin with.
>         if (isLinkFromRuntime && options.packagedModulesPath != null) {
>             throw taskHelper.newBadArgs("err.runtime.link.packaged.mods");
>         }

OK, thanks!

> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 559:
> 
>> 557:         Runtime.Version version = Runtime.version();
>> 558:         Path[] entries = paths.toArray(new Path[0]);
>> 559:         ModuleFinder finder = paths.isEmpty() ? ModuleFinder.ofSystem()
> 
> The javadoc needs update to something like:
> 
> 
>     * Returns a module finder of the given module path or system modules
>     * if the module path is empty that limits the observable modules to ...

Done.

> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 741:
> 
>> 739:         // we don't have the per module resource diffs in the modules image
>> 740:         try (InputStream inStream = getDiffInputStream()) {
>> 741:             if (inStream == null) {
> 
> Suggest to define a method `isLinkableRuntime` in the new `LinkableRuntimeImage` class to test if it's a linkable runtime image.

OK.

> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ImageReader.java line 48:
> 
>> 46:                 .filter(ImageReader::isNotTreeInfoResource)
>> 47:                 .sorted()
>> 48:                 .collect(Collectors.toList());
> 
> Suggestion:
> 
>         return Arrays.stream(getEntryNames())
>                 .filter(Predicate.not(ImageResourcesTree::isTreeInfoResource))
>                 .sorted()
>                 .toList();

ImageReader is no longer in the patch. Like JmodsReader.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823389406
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823395485
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823396005
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1823388023