RFR: 8345259: Disallow ALL-MODULE-PATH without explicit --module-path [v5]

[Mandy Chung]

On Tue, 10 Dec 2024 12:06:19 GMT, Severin Gehwolf <sgehwolf at 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