On Mon, 15 May 2023 22:51:52 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Refactor the Platform class in jdk.jpackage to use the internal >> OperatingSystem, Architecture, and Version classes. >> The OperatingSystem.isXXX() and Architecture.isYYY() methods replace >> comparisons in the Platform class. >> The checks of the os.version are replaced but may not be needed if OpenJDK >> no longer supports them. >> >> It is recommended to remove os version checks that apply only to Mac >> versions before 10.15. >> Mac OS X 10.15 is the oldest version supported. > > Roger Riggs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 34 commits: > > - Merge branch 'master' into 8304914-os-jpackage > - The OperatingSystem enum treats AIX separately from Linux. The original > Platform used the same enum for both. > Whereever OperatingSystem.isLinux is used in jpackage it should include > isAix as well. > - Minor source code style cleanup > - Merge branch 'master' into 8304914-os-jpackage > - Merge branch '8306678-os-version' into 8304914-os-jpackage > - Merge branch '8304915-arch-enum' into 8306678-os-version > - Correct comment on isPPC64() and refer to isLittleEndian() instead of > mentioning PPC64LE > - Simplify initialization in ClassLoaderHelper and fix VersionTest. > - Revert changes to MacOsX sun.nio.fs.BsdFileStore; the version check is > being removed in another PR. > - Review comment updates > - ... and 24 more: https://git.openjdk.org/jdk/compare/7b0b9b57...64ab7126 src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxDebBundler.java line 512: > 510: @Override > 511: public boolean supported(boolean runtimeInstaller) { > 512: return (OperatingSystem.isLinux() | OperatingSystem.isAix()) Ditto. src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxRpmBundler.java line 335: > 333: @Override > 334: public boolean supported(boolean runtimeInstaller) { > 335: return (OperatingSystem.isLinux() | OperatingSystem.isAix()) Why use `|`? I think from the code logic, `||` is better. Suggestion: return (OperatingSystem.isLinux() || OperatingSystem.isAix()) src/jdk.jpackage/share/classes/jdk/jpackage/internal/AddLauncherArguments.java line 144: > 142: } > 143: > 144: if (OperatingSystem.isLinux() | OperatingSystem.isAix()) { Ditto. src/jdk.jpackage/share/classes/jdk/jpackage/internal/ApplicationLayout.java line 188: > 186: } > 187: > 188: if (OperatingSystem.isLinux() | OperatingSystem.isAix()) { Ditto. src/jdk.jpackage/share/classes/jdk/jpackage/internal/I18N.java line 46: > 44: > 45: static { > 46: if (OperatingSystem.isLinux() | OperatingSystem.isAix()) { Ditto. src/jdk.jpackage/share/classes/jdk/jpackage/internal/ToolValidator.java line 50: > 48: args = new ArrayList<>(); > 49: > 50: if (OperatingSystem.isLinux() | OperatingSystem.isAix()) { Ditto. src/jdk.jpackage/share/classes/jdk/jpackage/internal/ValidOptions.java line 142: > 140: } > 141: > 142: if (OperatingSystem.isLinux() | OperatingSystem.isAix()) { Ditto. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13586#discussion_r1197846502 PR Review Comment: https://git.openjdk.org/jdk/pull/13586#discussion_r1197843828 PR Review Comment: https://git.openjdk.org/jdk/pull/13586#discussion_r1197858672 PR Review Comment: https://git.openjdk.org/jdk/pull/13586#discussion_r1197858780 PR Review Comment: https://git.openjdk.org/jdk/pull/13586#discussion_r1197858999 PR Review Comment: https://git.openjdk.org/jdk/pull/13586#discussion_r1197859099 PR Review Comment: https://git.openjdk.org/jdk/pull/13586#discussion_r1197859447
