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

Wed Dec 11 18:33:16 UTC 2024

On Wed, 11 Dec 2024 16:02:29 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 one additional commit since the last revision:
> 
>   Mandy's feedback

Thanks for the update.   A couple more comments...

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

> 378:     private static final String ALL_MODULE_PATH = "ALL-MODULE-PATH";
> 379:     private JlinkConfiguration initJlinkConfig() throws BadArgs {
> 380:         ModuleFinder appModulePathFinder = createFinderFromPath(options.modulePath);

Suggestion:

        ModuleFinder appModuleFinder = createFinderFromPath(options.modulePath);

Nit: Rereading my suggested code, better to rename the finder to "appModuleFinder" and also the comment in line 384

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

> 394:             Path defModPath = getDefaultModulePath();
> 395:             if (defModPath != null) {
> 396:                 options.modulePath.add(defModPath);

Default module path has already been added if `--module-path is not set` [1].   It's okay to leave it as is as line 281-287 will be removed by https://github.com/openjdk/jdk/pull/22494.

https://github.com/openjdk/jdk/blob/master/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java#L281

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

> 403:                 isLinkFromRuntime = true;
> 404:                 // JDK modules come from the system module path
> 405:                 finder = ModuleFinder.compose(ModuleFinder.ofSystem(), appModulePathFinder);

I think this should check `LinkableRuntimeImage.isLinkableRuntime()` and add the system module path only if it's a linkable runtime.

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

> 441:      * Creates a ModuleFinder for the given module paths.
> 442:      */
> 443:     public static ModuleFinder createFinderFromPath(List<Path> paths) {

Suggestion:

    public static ModuleFinder newModuleFinder(List<Path> paths) {

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

> 493:      * same as the current runtime.
> 494:      */
> 495:     public static ModuleFinder newModuleFinder(ModuleFinder original,

Suggestion:

    public static ModuleFinder newModuleFinder(ModuleFinder finder,

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

> 496:                                                Set<String> limitMods,
> 497:                                                Set<String> roots,
> 498:                                                boolean isRuntimeLink)

`isRuntimeLink` is strictly for performance but this method has no relationship with linking from packaged modules or run-time image.   Suggest to drop this new parameter and refactor the version check as a separate method and call it before creating `JlinkConfiguration`

test/jdk/tools/jlink/bindservices/BindServices.java line 77:

> 75:                 System.err.println("Test skipped. Not a linkable runtime and no JMODs");
> 76:                 return false;
> 77:             }

Suggestion:

    private static boolean isExplodedJDKImage() {
        if (!JMODS_EXIST && !LINKABLE_RUNTIME) {
            System.err.println("Test skipped. Not a linkable runtime and no JMODs");
            return true;
        }
        return false;

This method name tells the conditions when to skip the test.

test/jdk/tools/jlink/bindservices/SuggestProviders.java line 77:

> 75:                 System.err.println("Test skipped. Not a linkable runtime and no JMODs");
> 76:                 return false;
> 77:             }

Suggestion:

    private static boolean isExplodedJDKImage() {
        if (!JMODS_EXIST && !LINKABLE_RUNTIME) {
            System.err.println("Test skipped. Not a linkable runtime and no JMODs");
            return true;
        }
        return false;

This method name tells the conditions when to skip the test.

test/jdk/tools/jlink/runtimeImage/AbstractLinkableRuntimeTest.java line 196:

> 194:             boolean successExit = analyzer.getExitValue() == 0;
> 195:             String msg = String.format("Expected jlink to %s given a run-time image " +
> 196:                                        "link capable image. Exit code was: %d",

Suggestion:

            String msg = String.format("Expected jlink to %s given a linkable run-time image. " +
                                       "Exit code was: %d",

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

PR Review: https://git.openjdk.org/jdk/pull/22609#pullrequestreview-2496369242
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880620866
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880643361
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880657008
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880706992
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880685468
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880692700
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880681898
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880678056
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880680081