On Thu, 4 May 2023 20:37:31 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Refactor the Platform class of jlink to use jdk.internal.util >> OperatingSystem and Architecture instead of os.name and os.arch. >> They are direct replacements for the Platform enums except for UNKNOWN; its >> use is refactored to report errors via exceptions. >> >> Neither os.name nor os.arch should be assumed to be changeable; >> one test case is removed because it assumes os.name can be changed on the >> command line. > > Roger Riggs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 28 commits: > > - Remove unused static methods in DefaultImageBuilder > - Merge branch 'master' into 8304913-os-arch-jlink > - Merge branch 'master' into 8304913-os-arch-jlink > - Minor cleanup > - Merge branch 'master' into 8304913-os-arch-jlink > - 8304913: Use OperatingSystem, Architecture, and Version in jlink > - Simplify exception handling > - Simplify version parsing > - 8306678: Replace use of os.version with an internal Version record > - Use and test of "s390" verified by reviewer. > - ... and 18 more: https://git.openjdk.org/jdk/compare/0c6529d2...5bf9a506 src/jdk.jlink/share/classes/jdk/tools/jlink/builder/DefaultImageBuilder.java line 180: > 178: try { > 179: this.platform = Platform.parsePlatform(value); > 180: } catch (IllegalArgumentException ile) { What is "ile", can we rename it to "iae"? src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/ExcludeVMPlugin.java line 254: > 252: case WINDOWS -> new String[]{"jvm.dll"}; > 253: case MACOS -> new String[]{"libjvm.dylib", "libjvm.a"}; > 254: default -> new String[]{"libjvm.so", "libjvm.a"}; If you line up the -> then it might be clearer to the eyes. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/ExcludeVMPlugin.java line 256: > 254: default -> new String[]{"libjvm.so", "libjvm.a"}; > 255: }; > 256: } catch (IllegalArgumentException ile) { Confusing to name it "ile" here too. test/jdk/tools/jlink/plugins/CDSPluginTest.java line 102: > 100: System.out.println(" stdout: " + out.getStdout()); > 101: out.shouldMatch("Error: Cannot generate CDS archives: target > image platform linux-.*is different from runtime platform windows-.*"); > 102: out.shouldHaveExitValue(1); Good to see this part of the test going away. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13585#discussion_r1185970733 PR Review Comment: https://git.openjdk.org/jdk/pull/13585#discussion_r1185972077 PR Review Comment: https://git.openjdk.org/jdk/pull/13585#discussion_r1185971652 PR Review Comment: https://git.openjdk.org/jdk/pull/13585#discussion_r1185975906
