On Mon, 9 Dec 2024 12:57:22 GMT, Severin Gehwolf <sgehw...@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? test/jdk/tools/jpackage/share/BasicTest.java line 323: > 321: return; > 322: } > 323: } The better alternative would be to use `@ParameterSupplier` annotation instead of the list of `@Parameter` to dynamically configure the input for testAddModules() test case. You can use https://github.com/openjdk/jdk/blob/master/test/jdk/tools/jpackage/share/ErrorTest.java or https://github.com/openjdk/jdk/blob/cc628a133e471e7edf07831ff386f0eaf57e9bff/test/jdk/tools/jpackage/helpers-test/jdk/jpackage/test/AnnotationsTest.java#L97 as an example. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22644#discussion_r1876420735
