On Thu, 12 Dec 2024 17:06:58 GMT, Severin Gehwolf <sgehw...@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
