RFR: 8345259: Disallow ALL-MODULE-PATH without explicit --module-path [v6]

[Mandy Chung]

On Thu, 12 Dec 2024 17:06:58 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Please review this extension to #22609 which now disallows `ALL-MODULE-PATH` without explicit `--module-path` option or a non-existent module path. In addition, this fixes a bug mentioned in #22609 when `ALL-MODULE-PATH` and `--limit-modules` are used in combination. It failed earlier and passes now due to alignment of `ModuleFinder`s. With this patch JEP 493 enabled builds and regular JDK builds behave the same in terms of `ALL-MODULE-PATH`.
>> 
>> When an explicit module path is being added, there is no difference. All modules on that path will be added as roots. Tests have been added for the various cases and existing tests updated to allow for them to run on JEP 493 enabled builds. Thoughts?
>> 
>> Testing:
>> - [x] GHA, `test/jdk/tools/jlink` (all pass)
>> - [x] Added jlink test.
>
> Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 20 commits:
> 
>  - More test clean-ups
>  - Merge two AllModulePath tests
>  - Better error message with no modules on mod-path and ALL-MODULE-PATH
>  - Merge branch 'jdk-8345573-runtime-link-limit-mods' into jdk-8345259-all-module-path-fix
>  - Mandy's feedback
>  - Merge branch 'jdk-8345573-runtime-link-limit-mods' into jdk-8345259-all-module-path-fix
>  - Handle non-existent module-path with ALL-MODULE-PATH
>  - Move test, more test fixes for JEP 493 enabled builds
>  - Fix JLinkTest.java
>  - 8345259: Disallow ALL-MODULE-PATH without explicit --module-path
>  - ... and 10 more: https://git.openjdk.org/jdk/compare/c9643d31...9ba004cc

Need to bump the copyright end year in the tests.

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

> 374:             throw taskHelper.newBadArgs("err.no.module.path");
> 375:         }
> 376:         List<Path> originalModulePath = new ArrayList<>(options.modulePath);

This is to keep the module path set in the command line for error message.  An alternative is to keep `options.modulePath` be the original value and do not add the default module path to it.  Instead:


            Path defModPath = getDefaultModulePath();
            if (defModPath != null) {
                finder = newModuleFinder(Stream.concat(options.modulePath.stream(), Stream.of(defModPath)).toList());
            }


Then the error message can just print `options.modulePath`.

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

> 439:                 // run-time image. Only do this if no --limit-modules has been
> 440:                 // specified to begin with.
> 441:                 ModuleFinder mf = newLimitedFinder(finder,

FWIW, rereading this, it's still a bit awk to call a module finder a limited finder.

test/jdk/tools/jlink/basic/AllModulePath.java line 66:

> 64:  *        jdk.test.lib.process.OutputAnalyzer
> 65:  *        jdk.test.lib.compiler.CompilerUtils
> 66:  * @run testng/othervm -Duser.language=en -Duser.country=US AllModulePath

Is `-Duser.language=en -Duser.country=US` needed? 

Please add `@bug` to this test.

test/jdk/tools/jlink/basic/AllModulePath.java line 129:

> 127:             // java.base is a dependency of external modules
> 128:             modules.add("java.base");
> 129:         }

The difference in the expected list is because `$JDK/jmods` is added to the module path explicitly.

I suggest to change `createImage` not to include `jmods` in `--module-path` option to simplify it so this test case checks for the same expected result in different linking modes.

test/jdk/tools/jlink/basic/AllModulePath.java line 184:

> 182:         assertTrue(rc != 0);
> 183:         String actual = new String(baos.toByteArray()).trim();
> 184:         assertEquals(actual, "Error: --module-path option must be specified with --add-modules ALL-MODULE-PATH");

The new test cases can be cleaned up a little bit to match the existing test style.

Suggest to refactor this that other test cases can use something like `String jlink(boolean succeed, String... options)` that returns the output of stdout and stderr.   This constrains the output/error checking but it will catch test failures if any. 

`createImage` can simply call `jlink(true, ....)` that concatenates `options` with `"--output", image.toString()`

test/jdk/tools/jlink/basic/AllModulePath.java line 306:

> 304:                                     .map(s -> { return s.split("@")[0]; })
> 305:                                     .sorted()
> 306:                                     .toList();

Suggestion:

        OutputAnalyzer out = ProcessTools.executeCommand(java.toString(), "--list-modules");
                                         .shouldHaveExitValue(0);
        List<String> actual = out.asLines().stream()
                                 .map(s -> { return s.split("@")[0]; })
                                 .sorted()
                                 .toList();

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

PR Review: https://git.openjdk.org/jdk/pull/22494#pullrequestreview-2500386715
PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1882631255
PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1882735540
PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1882622347
PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1882704393
PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1882715241
PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1882731917