On Tue, 10 Dec 2024 12:06:19 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 incrementally with two > additional commits since the last revision: > > - Handle non-existent module-path with ALL-MODULE-PATH > - Move test, more test fixes for JEP 493 enabled builds This comment is also part of https://github.com/openjdk/jdk/pull/22609. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 417: > 415: allModsLimits = modsLimits; > 416: } > 417: ModuleFinder mf = newModuleFinder(rootsFinder, > allModsLimits, Set.of(), isLinkFromRuntime); It took me some time to understand this change as it depends on https://github.com/openjdk/jdk/pull/22609. I adjusted the code trying to help the readers easier to understand. See what you think. Instead of the `determineLinkFromRuntime` method, it does the check in this method that makes it explicit to the readers. private JlinkConfiguration initJlinkConfig() throws BadArgs { // Empty module path not allowed with ALL-MODULE-PATH in --add-modules if (options.addMods.contains(ALL_MODULE_PATH) && options.modulePath.isEmpty()) { throw taskHelper.newBadArgs("err.all.module.path.empty.mod.path"); } ModuleFinder appModulePathFinder = createFinderFromPath(options.modulePath); ModuleFinder finder = appModulePathFinder; boolean isLinkFromRuntime = false; // if packaged module for java.base is not found, either from // the default module path or the run-time image if (!appModulePathFinder.find("java.base").isPresent()) { // Add the default module path if that exists Path defModPath = getDefaultModulePath(); if (defModPath != null) { options.modulePath.add(defModPath); finder = createFinderFromPath(options.modulePath); } if (!finder.find("java.base").isPresent()) { isLinkFromRuntime = true; // java.base is from the system module path finder = ModuleFinder.compose(ModuleFinder.ofSystem(), appModulePathFinder); } } // Determine the roots set Set<String> roots = new HashSet<>(); for (String mod : options.addMods) { if (mod.equals(ALL_MODULE_PATH)) { // all observable modules are roots Set<String> initialRoots = appModulePathFinder.findAll() .stream() .map(ModuleReference::descriptor) .map(ModuleDescriptor::name) .collect(Collectors.toSet()); // Error if no module is found on module path if (initialRoots.isEmpty()) { throw taskHelper.newBadArgs("err.all.module.path.empty.mod.path"); } // Use a module finder with limited observability to find the observable modules // as not all JDK modules on the default module path or the run-time image ModuleFinder mf = newModuleFinder(finder, options.limitMods.isEmpty() ? initialRoots : options.limitMods, Set.of(), isLinkFromRuntime); mf.findAll() .stream() .map(ModuleReference::descriptor) .map(ModuleDescriptor::name) .forEach(mn -> roots.add(mn)); } else { roots.add(mod); } } finder = newModuleFinder(finder, options.limitMods, roots, isLinkFromRuntime); // --keep-packaged-modules doesn't make sense as we are not linking // from packaged modules to begin with. if (isLinkFromRuntime && options.packagedModulesPath != null) { throw taskHelper.newBadArgs("err.runtime.link.packaged.mods"); } return new JlinkConfiguration(options.output, roots, finder, isLinkFromRuntime, options.ignoreModifiedRuntime, options.generateLinkableRuntime); } src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 131: > 129: \ when running on a patched runtime with --patch-module > 130: err.all.module.path.empty.mod.path=ALL-MODULE-PATH requires > --module-path option (or --module-path does not exist) > 131: err.empty.module.path=empty module path `err.empty.module.path` is unused and can be modified for this use. Suggestion: err.no.module.path=--module-path option must be specified with --add-modules ALL-MODULE-PATH err.empty.module.path=No module found from the module path {0} test/jdk/tools/jlink/basic/AllModulePathTest.java line 54: > 52: * @run main/othervm -Xmx1g AllModulePathTest > 53: */ > 54: public class AllModulePathTest { Any reason why these test cases can't be added to `AllModulePath`? `AllModulePathTest` and `AllModulePath` test names are very alike. ------------- PR Review: https://git.openjdk.org/jdk/pull/22494#pullrequestreview-2493930943 PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1879074721 PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1879083050 PR Review Comment: https://git.openjdk.org/jdk/pull/22494#discussion_r1879092072
