On Wed, 11 Dec 2024 16:02:29 GMT, Severin Gehwolf <sgehw...@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
