On Wed, 11 Dec 2024 20:50:15 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 three > additional commits since the last revision: > > - Remove too strong assertion > - Test fixes > - Address comments from Mandy Looks good. Minor renaming suggestion. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 500: > 498: * {@code roots} set. > 499: */ > 500: public static ModuleFinder limitFinder(ModuleFinder finder, Inlining `limitFinder` sounds good while I suggest keeping the method name as `newModuleFinder` which is clear. Suggestion: public static ModuleFinder newModuleFinder(ModuleFinder finder, src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 551: > 549: * version. > 550: */ > 551: private static void checkVersion(ModuleFinder finder) { Suggestion: private static void checkJavaBaseVersion(ModuleFinder finder) { test/jdk/tools/jlink/IntegrationTest.java line 160: > 158: JlinkConfiguration config = new Jlink.JlinkConfiguration(output, > 159: mods, > 160: > JlinkTask.limitFinder(JlinkTask.newModuleFinder(modulePaths), limits, mods), > linkFromRuntime, false, false); `linkFromRuntime` parameter is no longer needed and this should fail compilation. ------------- Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22609#pullrequestreview-2496995286 PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1881007188 PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1880998805 PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1881005827
