On Thu, 12 Dec 2024 18:41:51 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> 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 > > 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. Makes sense. Done. > 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()` Yes, agreed. It should be much nicer now. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1883969543 PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1883970477
