On Wed, 12 Jul 2023 19:24:22 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> 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 > > 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. Done. The updated PR no longer has the endian value in the `JlinkConfiguration` > 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. Fixed > 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, I've updated the PR to use this suggestion. With this suggested change, the code is much more cleaner and I now just pass the `targetPlatform` to the `ImageHelper` constructor. > 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. Renamed as suggested and updated the comments as well in the updated version of the PR. > 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(); Done ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1262425330 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1262426029 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1262427486 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1262425722 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1262425873
