RFR: 8345573: Module dependencies not resolved from run-time image when --limit-module is being used [v4]

[Mandy Chung]

On Wed, 11 Dec 2024 20:50:15 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Please review this fix for JEP 493 enabled JDKs related to the `--limit-modules` option. The existing jlink `bindservices` tests cover this issue. Previously they didn't run on a JEP 493 enabled JDK, since `jmods` folder is missing for them.
>> 
>> The gist of the issue is that multiple `ModuleFinder`s were at play. In particular, when linking from the run-time image a finder is being set up based solely on the module-path. Which in that case only contains the application modules (not the JDK dependencies). So if the `--limit-modules` clause included any JDK modules that the app needs as a dependency the finder would fail because the module specified in the `--limit-modules` clause didn't exist in the finder's look-up space.
>> 
>> [A similar case happens](https://bugs.openjdk.org/browse/JDK-8345573?focusedId=14729247&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14729247) for a regular JDK build where `ALL-MODULE-PATH` is being used together with `--limit-modules`. In that case, the `FindException` is being thrown when the `root` set of modules are attempted to get assembled.
>> 
>> The fix that I'm proposing here is two fold:
>> 
>> 1. Distinguish whether or not we ought to resolve the JDK modules from the run-time image or not.
>> 2. Based on that information set up the finders as appropriate and use the same finders when assembling the module root set.
>> 
>> I think this makes for a cleaner implementation and also fixes the `--limit-modules` issue when linking from the run-time image as any finder that is being limited has already been properly set up to include dependencies (if need be).
>> 
>> Testing:
>> 
>> - [x]  GHA
>> - [x] jlink tests in `test/jdk/tools/jlink` on a regular JDK build and on a build with `--enable-linkable-runtime`. Both test runs show all tests passing.
>> 
>> Thoughts?
>
> Severin Gehwolf has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Remove too strong assertion
>  - Test fixes
>  - Address comments from Mandy

Looks good.  Minor renaming suggestion.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 500:

> 498:      * {@code roots} set.
> 499:      */
> 500:     public static ModuleFinder limitFinder(ModuleFinder finder,

Inlining `limitFinder` sounds good while I suggest keeping the method name as `newModuleFinder` which is clear.

Suggestion:

    public static ModuleFinder newModuleFinder(ModuleFinder finder,

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 551:

> 549:      * version.
> 550:      */
> 551:     private static void checkVersion(ModuleFinder finder) {

Suggestion:

    private static void checkJavaBaseVersion(ModuleFinder finder) {

test/jdk/tools/jlink/IntegrationTest.java line 160:

> 158:         JlinkConfiguration config = new Jlink.JlinkConfiguration(output,
> 159:                 mods,
> 160:                 JlinkTask.limitFinder(JlinkTask.newModuleFinder(modulePaths), limits, mods), linkFromRuntime, false, false);

`linkFromRuntime` parameter is no longer needed and this should fail compilation.

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

Marked as reviewed by mchung (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22609#pullrequestreview-2496995286
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1881007188
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880998805
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1881005827