On Wed, 27 Nov 2024 20:01:01 GMT, Severin Gehwolf <sgehw...@openjdk.org> wrote:
>> Please review this fix to how patched modules are being handled when linking >> from the run-time image. During review of >> [JDK-8311302](https://bugs.openjdk.org/browse/JDK-8311302) it was pointed >> out that module patching should be detected earlier and the link should get >> aborted in that case. >> >> This patch implements it, by using `ModuleBootstrap.patcher().hasPatches()`. >> After this patch module patching is being detected before any archives are >> being read or the actual linking process starts (contrary to the previous >> solution). >> >> Testing: >> - [x] GHA testing (mac aarch64 test failures are infra related) >> - [x] Local testing of existing test, which covers it >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 15 additional > commits since the last revision: > > - Merge branch 'master' into jdk-8343839-detect-patch-module > - Use ModuleBootStrap for detecting patches > - Revert "8344560: Add system property for patched runtime" > > This reverts commit 1d2395f39ee95a80937c63713e1f874ecc4ae76e. > - Revert "Move and amend test" > > This reverts commit c27c874b4c722aa4cfa5f6c71f9231e92a30db0c. > - Revert "Set the property to false for unpatched, expand tests" > > This reverts commit c635fdc0ad09c68ef652afb516c221b3c3a6299f. > - Revert "Mention jdk.patched in System class" > > This reverts commit a11e26637d89d5ae840753eabcfe6deb2ac025be. > - Merge branch 'jdk-8344560-jdk.patched-property' into > jdk-8343839-detect-patch-module > - Mention jdk.patched in System class > - Set the property to false for unpatched, expand tests > - Move and amend test > - ... and 5 more: https://git.openjdk.org/jdk/compare/ec553cfb...77695cea src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 226: > 224: } > 225: String message = switch (e.getReason()) { > 226: case PATCH_MODULE -> > helper.getMessage("err.runtime.link.patched.module", e.getFile()); As `MODIFIED_FILE` is the only reason to throw RILE, the enum `Reason` is no longer needed. This can be simplified. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/RuntimeImageLinkException.java line 38: > 36: > 37: public static enum Reason { > 38: PATCH_MODULE, /* link exception due to patched module */ Given there is only one reason to throw `RuntimeImageLinkException`, I wonder if `RuntimeImageLinkException` is still needed. Is RILE needed because checking the mismatch is done in stream? Maybe `JRTArchive::addNonClassResources` can return true/false to indicate if the file is modified. src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 129: > 127: err.runtime.link.modified.file={0} has been modified > 128: err.runtime.link.patched.module=The current runtime has been patched > with --patch-module.\ > 129: \ --patch-module is not supported when linking from the run-time image Suggestion: err.runtime.link.patched.module=jlink does not support linking from the run-time image when running on a patched runtime with --patch-module This error message seems more direct. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22037#discussion_r1866390686 PR Review Comment: https://git.openjdk.org/jdk/pull/22037#discussion_r1866433250 PR Review Comment: https://git.openjdk.org/jdk/pull/22037#discussion_r1866410574
