On Mon, 9 Dec 2024 13:21:31 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Please review these changes to jpackage in light of [JEP >> 493](https://openjdk.org/jeps/493). When this feature is enabled, then some >> of the `jpackage` tests fail. The failures fall into the following >> categories: >> >> - `ALL-DEFAULT` notion from `jpackage` which includes all modules that >> export an API, which includes `jdk.jlink`, which is prevented from being >> included when linking from the run-time image (see the [JEP >> 493](https://openjdk.org/jeps/493) restrictions). The proposal is to not >> include `jdk.jlink` and `jdk.jpackage` by default on a JDK build with JEP >> 493 enabled. A regular JDK build doesn't have this filtering. We could make >> this consistent across JDK builds by unconditionally filtering them, but I >> wasn't sure, so I've opted for the proposed solution for now. >> - Don't issue a warning when there is no `jmods` folder in the JDK install >> and we have a JEP 493 enabled build. In that case issuing the warning isn't >> appropriate as it's the expected behaviour. >> - `ALL-MODULE-PATH` changes: `BasicTest.java` verifies the `--add-modules` >> argument to `jpackage`. Using `ALL-MODULE-PATH` for JDK modules won't be >> supported for JEP 493-enabled builds. So I've changed this test to skip the >> test using `ALL-MODULE-PATH` when we have such an enabled build. Other >> tests, such as `RuntimeImageTest.java` and >> `RuntimeImageSymbolicLinksTest.java` tests verify something else not related >> to `ALL-MODULE-PATH` or `--add-modules`. It seems more appropriate to use >> the smaller set of modules to use for the runtime JDK image. >> - `JLinkOptionsTest.java`: That test verifies options passed to `jlink` via >> the `ToolProvider` API. For some reason, it uses `--bind-services` >> extensively and that - in turn - and, when not limited with the >> `--limit-modules` option as well, will include `jdk.jlink` in the resulting >> image, again running afoul the JEP 493 restriction of not allowing >> `jdk.jlink` for now. I propose to use suitable options including >> `--limit-modules` which would then no longer include `jdk.jlink` in the >> runtime image and the link from a run-time image works as well. These >> changes depend on [JDK-8345573](https://bugs.openjdk.org/browse/JDK-8345573) >> for it to work fully. >> >> Testing: >> - [ ] GHA >> - [x] running tests in `test/jdk/tools/jpackage` on a JEP 493 enabled JDK. >> As far as I could see the failures that I was seeing weren't any more >> related to JEP 493 (some RPM requirements showing up that it didn't expect >> to). >> >> Thoughts? Opinions? > > src/jdk.jlink/share/classes/module-info.java line 89: > >> 87: jdk.tools.jlink.internal.plugins.SaveJlinkArgfilesPlugin; >> 88: >> 89: exports jdk.tools.jlink.internal to jdk.jpackage; > > I think we should try to have alternatives that avoid jpackage having a > dependency on jlink internals. Yes, I feared this might be a concern. If we change how `jpackage` defines `ALL-DEFAULT` we could reduce it. The remaining use would be the case for issuing a warning for missing `jmods` directory, for which it's probably not worth keeping the dependency. Input from `jpackage` devs would be appreciated. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22644#discussion_r1875986081
