8345259: Disallow ALL-MODULE-PATH without explicit --module-path #22494

jerboaa · 2024-12-02T18:25:22Z

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 ModuleFinders. 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:

GHA, test/jdk/tools/jlink (all pass)
Added jlink test.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8345894 to be approved

Issues

JDK-8345259: Disallow ALL-MODULE-PATH without explicit --module-path (Bug - P3)
JDK-8345894: Disallow ALL-MODULE-PATH without explicit --module-path (CSR)

Reviewers

Mandy Chung (@mlchung - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22494/head:pull/22494
$ git checkout pull/22494

Update a local copy of the PR:
$ git checkout pull/22494
$ git pull https://git.openjdk.org/jdk.git pull/22494/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22494

View PR using the GUI difftool:
$ git pr show -t 22494

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22494.diff

Using Webrev

Link to Webrev Comment

…accepted

bridgekeeper · 2024-12-02T18:26:33Z

👋 Welcome back sgehwolf! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

test/jdk/tools/jlink/runtimeImage/BasicJlinkTest.java

openjdk · 2024-12-02T18:27:24Z

@jerboaa This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8345259: Disallow ALL-MODULE-PATH without explicit --module-path

Reviewed-by: mchung

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

b2811a0: 8340493: Fix some Asserts failure messages

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-12-02T18:28:04Z

@jerboaa The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-12-02T18:31:11Z

Webrevs

AlanBateman · 2024-12-03T15:26:32Z

I'm in two minds as to whether we should do anything here.

ALL-MODULE-PATH means all observable modules on the module path but here, it's all observable modules minus jdk.jlink or any module that directly requires jdk.jlink. This is hard to reason about, and not exactly right either because any filtering should be any module that transitively requires jdk.jlink.

As background, the original motivation for ALL-MODULE-PATH was Maven build. It has always been supported by jlink too but I can't recall, or find, why we did that. Maybe Mandy can remember. In JEP 261, it is listed as being valid for both compile-time and run-time, but also mentions link-time.

jerboaa · 2024-12-03T15:48:39Z

I'm in two minds as to whether we should do anything here.

So am I. The main motivation is that this no longer works with JEP 493 is enabled (no jmods dir):

$ bin/jlink --add-modules ALL-MODULE-PATH --output all-mods

Without looking at the implementation, the user doesn't add anything explicit via --module-path (the impl in fact adds jmods folder if it exists). So it can be seen as a regression.

ALL-MODULE-PATH means all observable modules on the module path but here, it's all observable modules minus jdk.jlink or any module that directly requires jdk.jlink. This is hard to reason about.

I'll add here that the reason we need to do this in this patch is because jdk.jlink isn't being allowed to be part of the link in JEP 493. So in a way, when that restriction goes away this restriction can go away too.

I'd be fine with adding a Warning that for run-time image links and ALL-MODULE-PATH doesn't add jdk.jlink or modules depending on jdk.jlink to the root set. This would be the right thing to do to call out the difference.

mlchung · 2024-12-03T18:01:18Z

ALL-MODULE-PATH means all observable modules on the module path but here, it's all observable modules minus jdk.jlink or any module that directly requires jdk.jlink. This is hard to reason about, and not exactly right either because any filtering should be any module that transitively requires jdk.jlink.

I have the same comment. ALL-MODULE-PATH means all observable modules on the module path that includes jdk.jlink. It would be confusing if the resulting image linked from the run-time image is different than the image linked from packaged modules. I think it may just be another restriction that --add-modules ALL-MODULE-PATH can't be used when linking from the run-time image for now and removed together when the other restriction is removed.

As background, the original motivation for ALL-MODULE-PATH was Maven build. It has always been supported by jlink too but I can't recall, or find, why we did that. Maybe Mandy can remember. In JEP 261, it is listed as being valid for both compile-time and run-time, but also mentions link-time.

I don't recall neither. It might just be implemented as consistent with java and javac.

jerboaa · 2024-12-03T18:51:04Z

ALL-MODULE-PATH means all observable modules on the module path but here, it's all observable modules minus jdk.jlink or any module that directly requires jdk.jlink. This is hard to reason about, and not exactly right either because any filtering should be any module that transitively requires jdk.jlink.

I have the same comment. ALL-MODULE-PATH means all observable modules on the module path that includes jdk.jlink. It would be confusing if the resulting image linked from the run-time image is different than the image linked from packaged modules. I think it may just be another restriction that --add-modules ALL-MODULE-PATH can't be used when linking from the run-time image for now and removed together when the other restriction is removed.

That makes sense. As long as we handle it, I'm fine. So my thinking then is:

Abort the link with an appropriate error when ALL-MODULE-PATH is being used and a link from the run-time image is used but no module path given (--module-path arg missing).
Continue as is now when linking from the run-time image, --add-modules ALL-MODULE-PATH is given, and --module-path arg is given as well (there is no difference to a run with packaged modules).

Does that sound OK to you?

mlchung · 2024-12-03T19:15:59Z

Sounds good to me. If --add-modules ALL-MODULE-PATH is given, jlink will find all observable modules from --module-path and use them as the root modules. If the root set is empty, error; otherwise, it will link with the application modules on the module path and its transitive dependences.

AlanBateman · 2024-12-05T14:39:58Z

Abort the link with an appropriate error when ALL-MODULE-PATH is being used and a link from the run-time image is used but no module path given (--module-path arg missing).

Just to double check, with the default build, then jlink --add-modules ALL-MODULE-PATH --output myimage would fail because the module path hasn't been specified. No difference between default and --generate-linkable-runtime build. It would be a change in behaviour but I think the right thing to do.

jerboaa · 2024-12-06T09:31:21Z

Abort the link with an appropriate error when ALL-MODULE-PATH is being used and a link from the run-time image is used but no module path given (--module-path arg missing).

Just to double check, with the default build, then jlink --add-modules ALL-MODULE-PATH --output myimage would fail because the module path hasn't been specified. No difference between default and --generate-linkable-runtime build. It would be a change in behaviour but I think the right thing to do.

Originally I hadn't intended to do that, but I can add this. Should I do it in this bug or a separate one? I would think that this change of behaviour would need a CSR?

…limit-module is being used

AlanBateman · 2024-12-06T11:18:29Z

Just to double check, with the default build, then jlink --add-modules ALL-MODULE-PATH --output myimage would fail because the module path hasn't been specified. No difference between default and --generate-linkable-runtime build. It would be a change in behaviour but I think the right thing to do.

Originally I hadn't intended to do that, but I can add this. Should I do it in this bug or a separate one? I would think that this change of behaviour would need a CSR?

I think this case was missed when jlink was modified to default the module path to $JDK_HOME/jmods. I think my preference would be for this case to fail when --module-path is not specified. Let's see if Mandy agrees. If we do this change then it would be a behavior change so would need a CSR.

mlchung · 2024-12-06T18:33:06Z

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)

jerboaa · 2024-12-09T10:31:16Z

OK. I'll update the PR accordingly.

… is not accepted" This reverts commit 93c2392.

…limit-module is being used

mlchung · 2024-12-18T18:01:24Z

$ jlink --add-modules ALL-MODULE-PATH --module-path build/jmods --limit-modules jdk.net --output ./build/test-img

This creates an image with jdk.net and its transitive dependences which is the existing behavior, right? We are trying to revisit --add-modules ALL-MODULE-PATH with --limit-modules in the future.

"transitive closure of named modules plus the modules added with --add-modules" as {java.base, a, b, c}

Yes. That's how it is implemented as https://openjdk.org/jeps/261#Limiting-the-observable-modules describes:

(The transitive closure computed for the interpretation of the --limit-modules option is a temporary result, used only to compute the limited set of observable modules. The resolver will be invoked again in order to compute the actual module graph.)

So jlink sets up the finder that limits the observability and compute the module graph from the roots using this finder.

jerboaa · 2024-12-18T18:25:36Z

$ jlink --add-modules ALL-MODULE-PATH --module-path build/jmods --limit-modules jdk.net --output ./build/test-img
This creates an image with jdk.net and its transitive dependences which is the existing behavior, right? We are trying to revisit --add-modules ALL-MODULE-PATH with --limit-modules in the future.

Only if the $JAVA_HOME/jmods folder is also added to the module path. Existing behaviour is like this:

bin/jlink --add-modules ALL-MODULE-PATH --module-path mods --limit-modules jdk.net --verbose --output test-img
Error: Module jdk.net not found
java.lang.module.FindException: Module jdk.net not found
	at java.base/java.lang.module.Resolver.findFail(Resolver.java:892)
	at java.base/java.lang.module.Resolver.resolve(Resolver.java:129)
	at java.base/java.lang.module.Configuration.resolve(Configuration.java:413)
	at java.base/java.lang.module.Configuration.resolve(Configuration.java:252)
	at jdk.jlink/jdk.tools.jlink.internal.JlinkTask.limitFinder(JlinkTask.java:780)
	at jdk.jlink/jdk.tools.jlink.internal.JlinkTask.newModuleFinder(JlinkTask.java:552)
	at jdk.jlink/jdk.tools.jlink.internal.JlinkTask.initJlinkConfig(JlinkTask.java:384)
	at jdk.jlink/jdk.tools.jlink.internal.JlinkTask.run(JlinkTask.java:289)
	at jdk.jlink/jdk.tools.jlink.internal.Main.run(Main.java:50)
	at jdk.jlink/jdk.tools.jlink.internal.Main.main(Main.java:34)

vs.

$ ./bin/jlink --add-modules ALL-MODULE-PATH --module-path mods:$(pwd)/jmods --limit-modules jdk.net --verbose --output test-img
java.base file:///disk/openjdk/builds/oracle-ea/jdk-24+26/jmods/java.base.jmod
jdk.net file:///disk/openjdk/builds/oracle-ea/jdk-24+26/jmods/jdk.net.jmod

Providers:
  java.base provides java.nio.file.spi.FileSystemProvider used by java.base

I can revert those changes if you prefer.

AlanBateman · 2024-12-18T19:08:25Z

I think it helps to think of --module-path $JAVA_HOME/jmods --add-modules ALL-MODULE-PATH as --module-path $JAVA_HOME/jmods --add-modules java.base,java.compiler,java.datatransfer, .. jdk.xml.dom,jdk.zipfs.

This leads jlink --add-modules ALL-MODULE-PATH --limit-modules jdk.net to create an image with "all modules". This may be surprising on first glance but it's the recursive enumeration so jdk.net (which is just jdk.net and java.base), plus the modules specified to --add-modules.

Note that we have issues at run-time with this combination, and at run-time there are other possible tokens that can be used too. Not for here, this issue has always existed.

jerboaa · 2024-12-18T19:10:54Z

Only if the $JAVA_HOME/jmods folder is also added to the module path. Existing behaviour is like this:

Never mind. That's what JDK-8345573 changed.

I've reverted the eager configuration checking.

mlchung · 2024-12-18T19:14:55Z

Now I see better from JDK 23 implementation. jlink --add-modules ALL-MODULE-PATH --module-path mods --limit-modules XXX" always fails independent to what modules are set in --limit-modules. The reason is that it always fails to create the module finder with a non-empty limit modules for ALL-MODULE-PATH`.

      ModuleFinder finder = newModuleFinder(options.modulePath, options.limitMods, Set.of());

https://github.com/openjdk/jdk23u/blob/master/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java#L374

jerboaa · 2024-12-18T19:19:54Z

Yes. See https://bugs.openjdk.org/browse/JDK-8345573?focusedId=14729247&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14729247

mlchung · 2024-12-18T19:22:52Z

The solution is simpler -- it can just detect if ALL-MODULE-PATH is used with --limit-modules, jlink just fails.

AlanBateman · 2024-12-18T19:34:47Z

This solution is simpler -- it can just detect if ALL-MODULE-PATH is used with --limit-modules, jlink just fails.

That seems like a good suggestion to remove the complexity and need to think what it means. It would avoid complexity in the other phases too.

jerboaa · 2024-12-19T11:07:13Z

The latest update disallows --limit-modules when ALL-MODULE-PATH is specified. PTAL. Thanks!

mlchung

Looks good. Please update the copyright end year.

test/jdk/tools/jlink/basic/AllModulePath.java

jerboaa · 2024-12-19T21:27:38Z

Looks good. Please update the copyright end year.

Thanks for the review! Done in the latest update.

mlchung

Looks good. This PR uncovers unanticipated issues and thanks for looking at them thoroughly.

jerboaa · 2024-12-20T09:31:37Z

Looks good. This PR uncovers unanticipated issues and thanks for looking at them thoroughly.

Thanks for the thorough reviews as well!

jerboaa · 2024-12-20T09:32:48Z

@AlanBateman OK for you too? Thanks!

AlanBateman · 2024-12-20T11:47:28Z

@AlanBateman OK for you too? Thanks!

So --limit-modules && --add-modules ALL-MODULE-PATH is an error, as is --add-modules ALL-MODULE-PATH without specifying --module-path. Happy with that. I don't have time to go through the test but Mandy has already reviewed so I think we are good. I'm just wondering if this needs a CSR and also whether the man page needs update.

jerboaa · 2024-12-20T12:33:59Z

@AlanBateman OK for you too? Thanks!

So --limit-modules && --add-modules ALL-MODULE-PATH is an error, as is --add-modules ALL-MODULE-PATH without specifying --module-path. Happy with that. I don't have time to go through the test but Mandy has already reviewed so I think we are good. I'm just wondering if this needs a CSR and also whether the man page needs update.

Thanks. This has got a CSR.

jerboaa · 2024-12-20T12:34:39Z

/integrate

openjdk · 2024-12-20T12:35:37Z

Going to push as commit bcb1bda.
Since your change was applied there have been 7 commits pushed to the master branch:

054c644: 8346667: Doccheck: warning about missing before
2a68f74: 8346128: Comparison build fails due to difference in LabelTarget.html
cf28fd4: 8322983: Virtual Threads: exclude 2 tests
85e024d: 8346605: AIX fastdebug build fails in memoryReserver.cpp after JDK-8345655
54f3475: 8331467: FileSystems.getDefault fails with ClassNotFoundException if custom default provider is in run-time image
35fafbc: 8346106: Verify.checkEQ: testing utility for recursive value verification
b2811a0: 8340493: Fix some Asserts failure messages

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-20T12:35:43Z

@jerboaa Pushed as commit bcb1bda.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

jerboaa · 2024-12-20T12:38:28Z

whether the man page needs update.

If you think that's needed then I'm fine to follow up with it. I don't think that's warranted. There will be a release note.

mlchung · 2024-12-20T18:06:12Z

The man page does not mention about ALL-MODULE-PATH. As we believe it's rarely used, I'm okay to leave it as is.

`--add-modules` *mod*\[`,`*mod*...\]
:   Adds the named modules, *mod*, to the default set of root modules. The
    default set of root modules is empty.

8345259: When linking from the run-time image ALL-MODULE-PATH is not …

93c2392

…accepted

jerboaa commented Dec 2, 2024

View reviewed changes

test/jdk/tools/jlink/runtimeImage/BasicJlinkTest.java Outdated Show resolved Hide resolved

openjdk bot added the rfr Pull request is ready for review label Dec 2, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Dec 2, 2024

8345573: Module dependencies not resolved from run-time image when --…

4c4193e

…limit-module is being used

jerboaa added 2 commits December 6, 2024 13:57

More test changes

5234069

Another fix for the incomplete jmods in default mod-path case

bbdd431

jerboaa added 7 commits December 9, 2024 14:34

Merge branch 'master' into jdk-8345259-all-module-path-fix

1f80d6d

Remove runtimeImage specific test

373dccc

Revert "8345259: When linking from the run-time image ALL-MODULE-PATH…

e3276e3

… is not accepted" This reverts commit 93c2392.

8345573: Module dependencies not resolved from run-time image when --…

95419d9

…limit-module is being used

8345259: Disallow ALL-MODULE-PATH without explicit --module-path

99d4195

Merge branch 'master' into jdk-8345573-runtime-link-limit-mods

383d068

Merge branch 'master' into jdk-8345259-all-module-path-fix

6b1e871

jerboaa changed the base branch from master to pr/22609 December 9, 2024 18:49

jerboaa changed the title ~~8345259: When linking from the run-time image ALL-MODULE-PATH is not accepted~~ 8345259: Disallow ALL-MODULE-PATH without explicit --module-path Dec 9, 2024

Fix comment

4135dcb

Don't allow --limit-modules with ALL-MODULE-PATH

f488c63

mlchung reviewed Dec 19, 2024

View reviewed changes

test/jdk/tools/jlink/basic/AllModulePath.java Outdated Show resolved Hide resolved

test/jdk/tools/jlink/basic/AllModulePath.java Outdated Show resolved Hide resolved

jerboaa added 2 commits December 19, 2024 19:21

Merge branch 'master' into jdk-8345259-all-module-path-fix

e53add4

Copyright year and test reshuffeling

024404b

mlchung approved these changes Dec 19, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 19, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 20, 2024

openjdk bot closed this Dec 20, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 20, 2024

8345259: Disallow ALL-MODULE-PATH without explicit --module-path #22494

8345259: Disallow ALL-MODULE-PATH without explicit --module-path #22494

Conversation

jerboaa commented Dec 2, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Dec 2, 2024

openjdk bot commented Dec 2, 2024 • edited Loading

openjdk bot commented Dec 2, 2024

mlbridge bot commented Dec 2, 2024 • edited Loading

Webrevs

AlanBateman commented Dec 3, 2024

jerboaa commented Dec 3, 2024

mlchung commented Dec 3, 2024 • edited Loading

jerboaa commented Dec 3, 2024 • edited Loading

mlchung commented Dec 3, 2024

AlanBateman commented Dec 5, 2024

jerboaa commented Dec 6, 2024

AlanBateman commented Dec 6, 2024

mlchung commented Dec 6, 2024

jerboaa commented Dec 9, 2024

mlchung commented Dec 18, 2024

jerboaa commented Dec 18, 2024

AlanBateman commented Dec 18, 2024 • edited Loading

jerboaa commented Dec 18, 2024

mlchung commented Dec 18, 2024 • edited Loading

jerboaa commented Dec 18, 2024

mlchung commented Dec 18, 2024 • edited Loading

AlanBateman commented Dec 18, 2024

jerboaa commented Dec 19, 2024

mlchung left a comment

Choose a reason for hiding this comment

jerboaa commented Dec 19, 2024

mlchung left a comment

Choose a reason for hiding this comment

jerboaa commented Dec 20, 2024

jerboaa commented Dec 20, 2024

AlanBateman commented Dec 20, 2024 • edited Loading

jerboaa commented Dec 20, 2024

jerboaa commented Dec 20, 2024

openjdk bot commented Dec 20, 2024

openjdk bot commented Dec 20, 2024

jerboaa commented Dec 20, 2024

mlchung commented Dec 20, 2024

jerboaa commented Dec 2, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 2, 2024 •

edited

Loading

mlbridge bot commented Dec 2, 2024 •

edited

Loading

mlchung commented Dec 3, 2024 •

edited

Loading

jerboaa commented Dec 3, 2024 •

edited

Loading

AlanBateman commented Dec 18, 2024 •

edited

Loading

mlchung commented Dec 18, 2024 •

edited

Loading

mlchung commented Dec 18, 2024 •

edited

Loading

AlanBateman commented Dec 20, 2024 •

edited

Loading