RFR: 8304913: Use OperatingSystem, Architecture, and Version in jlink [v2]

[Alan Bateman]

On Thu, 4 May 2023 20:37:31 GMT, Roger Riggs <rriggs at 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