On Wed, 12 Jul 2023 10:58:49 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review for this change which proposes to fix the issue >> reported in https://bugs.openjdk.org/browse/JDK-8206890? >> >> The `jlink` command allows a `--endian` option to specify the byte order in >> the generated image. Before this change, when such a image was being >> launched, the code would assume the byte order in the image to be the native >> order of the host where the image is being launched. That would result in >> failure to launch java, as noted in the linked issue. >> >> The commit in this PR, changes relevant places to not assume native order >> and instead determine the byte order by reading the magic bytes in the image >> file's header content. >> >> A new jtreg test has been added which reproduces the issue and verifies the >> fix. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 45 commits: > > - move copyright before imports in the new test > - add a new test for jlink --endian usages > - merge latest from master branch > - use newly introduced Architecture.byteOrder() API > - merge latest from master branch > - update jdk.tools.jlink.internal.Platform class to be aware of non-current > platform's endianness > - remove no longer needed constructor > - merge latest from master branch > - foo > - merge latest from master branch > - ... and 35 more: https://git.openjdk.org/jdk/compare/753bd563...962d542d Looks okay with a suggestion to determine the endianness outside `ImageHelper`. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Jlink.java line 168: > 166: this.output = output; > 167: this.modules = Objects.requireNonNull(modules); > 168: this.endian = endian; `JlinkConfiguration` does not need to know the endianness. Only the image builder needs it. Probably good to take the `endian` parameter out from `JlinkConfiguration` and instead pass the value of `--endian` to `JlinkTask::createImageProvider` like other jlink options. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 412: > 410: > 411: // First create the image provider > 412: ImageHelper imageProvider = createImageProvider(config, formatting nit: line 413-415 should be adjusted to align the parameters. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 568: > 566: Map<String, Path> mods = cf.modules().stream() > 567: .collect(Collectors.toMap(ResolvedModule::name, > JlinkTask::toPathLocation)); > 568: return new ImageHelper(cf, mods, config.getByteOrder(), > retainModulesPath, ignoreSigning, Also we can move the logic of determining the endianess of the target platform out of `ImageHelper`. Evaluate the target platform and endianness here and pass to `ImageHelper`. Suggestion: // endian is the value specified via --endian and a new parameter to createImageProvider Platform targetPlatform = targetPlatform(cf, modsPaths); ByteOrder targetOrder = endian != null ? endian : targetPlatform.arch().byteOrder(); return new ImageHelper(cf, mods, targetOrder, retainModulesPath, ignoreSigning, src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 856: > 854: // returns true if the current platform's "jmods" directory is > the parent of the > 855: // passed javaBasePath > 856: private static boolean isJavaBaseFromCurrentPlatform(Path > javaBasePath) throws IOException { This method checks if we are jlinking the JMOD files from the default module path. This is not exactly checking if `java.base` matches the current platform since there are other possible setups too. Suggest to rename the method name to `isJavaBaseFromDefaultModulePath`. So the comments should also be updated to reflect that. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 857: > 855: // passed javaBasePath > 856: private static boolean isJavaBaseFromCurrentPlatform(Path > javaBasePath) throws IOException { > 857: Path currentPlatformJmods = getDefaultModulePath(); Suggestion: Path defaultModulePath = getDefaultModulePath(); ------------- PR Review: https://git.openjdk.org/jdk/pull/11943#pullrequestreview-1527094883 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1261636015 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1261642743 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1261654437 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1261639546 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1261640202
