On Fri, 25 Oct 2024 19:12:55 GMT, Mandy Chung <mch...@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
