On Fri, 6 Dec 2024 18:33:06 GMT, Mandy Chung <mch...@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. > > Such behavioral change is a good change as jlink from the default and > --generate-linkable-runtime build would have the consistent behavior. If a > module path is given with no root module (empty path), it throws the > following error. I think it can throw a similar message as if `--add-modules > ALL-MODULE-PATH` is given but no `--module-path`. > > > $ jlink --add-modules ALL-MODULE-PATH --output myimage --module-path emptyPath > Error: Cannot invoke "java.nio.file.Path.getFileName()" because > "javaBasePath" is null > java.lang.NullPointerException: Cannot invoke > "java.nio.file.Path.getFileName()" because "javaBasePath" is null > at > jdk.jlink/jdk.tools.jlink.internal.JlinkTask.isJavaBaseFromDefaultModulePath(JlinkTask.java:660) > at > jdk.jlink/jdk.tools.jlink.internal.JlinkTask.targetPlatform(JlinkTask.java:632) > at > jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImageProvider(JlinkTask.java:569) > at > jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImage(JlinkTask.java:410) > at jdk.jlink/jdk.tools.jlink.internal.JlinkTask.run(JlinkTask.java:285) > at jdk.jlink/jdk.tools.jlink.internal.Main.run(Main.java:56) > at jdk.jlink/jdk.tools.jlink.internal.Main.main(Main.java:34) > @mlchung It's not clear if you think there is an action expected from myself. > If there is, please clarify what. Thanks! Sorry for not being clear. This requires discussion. As I look into the behavior of `--add-modules ALL-MODULE-PATH` with `--limit-modules` more link time and run-time, I find some uncertainty and question if `--add-modules ALL-MODULE-PATH` with `--limit-modules` is needed/interesting. Thanks @AlanBateman for clarifying that `--add-modules ALL-MODULE-PATH` with `--limit-modules` combination isn't implemented at run-time. While I agree that this is a separate issue, this PR depends on how this should be implemented because the test cases (testLimitedModules and testAddModules in AllModulePath.java) do not disambiguate the expected behavior. Unsupporting this combination could be one option but different from run-time which I think it's not ideal. I propose to implement as described in https://openjdk.org/jeps/261#Limiting-the-observable-modules -- to limit the observable modules to those in the transitive closure of the named modules plus the modules specified via the `--add-modules` option. In other words, `--add-modules ALL-MODULE-PATH` adds all modules on module path to the observable set. It's easy to explain and understand and no difference when `--limit-modules` is combined with `--add-modules m1,m2,...` with named modules. ------------- PR Comment: https://git.openjdk.org/jdk/pull/22494#issuecomment-2549311088
