On Thu, 19 Dec 2024 11:10:16 GMT, Severin Gehwolf <[email protected]> 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 incrementally with one
> additional commit since the last revision:
>
> Don't allow --limit-modules with ALL-MODULE-PATH
Looks good. Please update the copyright end year.
test/jdk/tools/jlink/basic/AllModulePath.java line 133:
> 131: */
> 132: @Test
> 133: public void testSubsetModules() throws Throwable {
Can simply move `testLimitModules` (line 209-229) to replace this method.
This test case is not needed as this test focuses on `ALL-MODULE-PATH` and
covered by other tests. Modifying `testLimitModules` to test the error will
help as clearly shown from the commit.
test/jdk/tools/jlink/basic/AllModulePath.java line 162:
> 160: List<String> opts = List.of("--module-path", MODS.toString(),
> 161: "--output", image.toString(),
> 162: "--add-modules", "m1,test",
Suggestion:
"--add-modules", "m1",
This can show clearly that `ALL-MODULE-PATH` includes all modules on the module
path.
-------------
PR Review: https://git.openjdk.org/jdk/pull/22494#pullrequestreview-2515591856
PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1892746545
PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1892774475
