Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8253702: BigSur version number reported as 10.16, should be 11.nn #2530

Closed
wants to merge 3 commits into from

Conversation

RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Feb 11, 2021

On Mac Os X, the OSVersionTest detected a difference in the version number reported in the os.version property
and the version number provided by sw_vers -productVersion.

When the java runtime is built with XCode 11.3, the os.version is reported as 10.16
though the current version numbering is 11.nnn.

The workaround is to derive the os.version number from the ProductBuildVersion.
When the toolchain is updated to XCode 12.nnn it can be removed.
The workaround is enabled only when the environment variable SYSTEM_VERSION_COMPAT is unset.
When the SYSTEM_VERSION_COMPAT is set in the environment the version number is reported as reported by Mac OS X.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8253702: BigSur version number reported as 10.16, should be 11.nn

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2530/head:pull/2530
$ git checkout pull/2530

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 11, 2021

👋 Welcome back rriggs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Feb 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 11, 2021

@RogerRiggs The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs label Feb 11, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 11, 2021

Webrevs

} else {
nsVerStr = [NSString stringWithFormat:@"%ld.%ld.%ld",
(long)osVer.majorVersion, (long)osVer.minorVersion, (long)osVer.patchVersion];
// Copy out the char* if running on version other than pre-10.16 Mac OS (10.16 == 11.x)
Copy link
Member

@kevinrushforth kevinrushforth Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the comment backwards from what the test does? I would think it should be "if running on version other than 10.16 Mac OS ...". Or am I missing something?

Copy link
Member

@bplb bplb Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinrushforth I think you are correct. This is actually mea culpa I think as I had provided in a Slack thread a failed attempt at a patch for this which contained this comment.

Copy link
Contributor Author

@RogerRiggs RogerRiggs Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the words "other than" are too subtle?

Copy link
Member

@kevinrushforth kevinrushforth Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a double negative, unless I'm reading it incorrectly: "other than pre-10.16" I interpret as "not pre-10.16" or "10.16".

Copy link
Member

@bplb bplb Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Copy out the char* if running on version 10.x[.y], where x < 16 ?

Copy link
Member

@kevinrushforth kevinrushforth Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will also do it if running on 11.x[.y] (once Xcode is upgraded), right?

}
// Copy out the char*
osVersionCStr = strdup([nsVerStr UTF8String]);
} else if (getenv("SYSTEM_VERSION_COMPAT") == NULL) {
Copy link
Member

@kevinrushforth kevinrushforth Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If version is 10.16 and SYSTEM_VERSION_COMPAT is set, you will fall through to the pre-10.9 Mac OS code fallback. Just checking to see if that's what you intended.

Copy link
Member

@kevinrushforth kevinrushforth Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, it seems to work OK using the legacy fallback path (reports 10.16 if I set SYSTEM_VERSION_COMPAT=1).

Copy link
Contributor Author

@RogerRiggs RogerRiggs Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same version string is available from both APIs, reading from the SystemVersion.plist is a bit slower.
It would be clearer to move the checking of SYSTEM_VERSION_COMPAT to the first test (line:252)
so the version info does not need to be read from the files.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 11, 2021

I tested this, and get the following behavior on macOS 11.0.1 with a JDK compiled with Xcode 11.3.1 and your patch:

SYSTEM_VERSION_COMPAT not set : 11.0
SYSTEM_VERSION_COMPAT=1 : 10.16
SYSTEM_VERSION_COMPAT=0 : 11.0.1

So the fallback path reports what could be considered a more accurate version, at least on my laptop. Since I'm not sure what is expected, you can decide what to do with this information.

bplb
bplb approved these changes Feb 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 11, 2021

@RogerRiggs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8253702: BigSur version number reported as 10.16, should be 11.nn

Reviewed-by: bpb, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 22 new commits pushed to the master branch:

  • 33fcd32: 8261659: JDK-8261027 causes a Tier1 validate-source failure
  • 3aa1b4c: 8261623: reference to javac internals in Extern class
  • 350303d: 8260221: java.util.Formatter throws wrong exception for mismatched flags in %% conversion
  • 6475d47: 8261655: [PPC64] Build broken after JDK-8260941
  • c0e805a: 8261654: Missing license header in Signatures.java
  • b670efd: 8261072: AArch64: Fix MacroAssembler::get_thread convention
  • 59b8d59: 8261481: Cannot read Kerberos settings in dynamic store on macOS Big Sur
  • 9f81ca8: 8261230: GC tracing of page sizes are wrong in a few places
  • 40ae993: 8261027: AArch64: Support for LSE atomics C++ HotSpot code
  • 9ffabf3: 8252971: WindowsFileAttributes does not know about Unix domain sockets
  • ... and 12 more: https://git.openjdk.java.net/jdk/compare/1740de2a0d41d0689ae9bb069b93ab267a2b9495...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Feb 11, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good.

@RogerRiggs RogerRiggs changed the title 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0 8253702: BigSur version number reported as 10.16, should be 11.nn Feb 12, 2021
@RogerRiggs
Copy link
Contributor Author

@RogerRiggs RogerRiggs commented Feb 12, 2021

/integrate

@openjdk openjdk bot closed this Feb 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 12, 2021

@RogerRiggs Since your change was applied there have been 22 commits pushed to the master branch:

  • 33fcd32: 8261659: JDK-8261027 causes a Tier1 validate-source failure
  • 3aa1b4c: 8261623: reference to javac internals in Extern class
  • 350303d: 8260221: java.util.Formatter throws wrong exception for mismatched flags in %% conversion
  • 6475d47: 8261655: [PPC64] Build broken after JDK-8260941
  • c0e805a: 8261654: Missing license header in Signatures.java
  • b670efd: 8261072: AArch64: Fix MacroAssembler::get_thread convention
  • 59b8d59: 8261481: Cannot read Kerberos settings in dynamic store on macOS Big Sur
  • 9f81ca8: 8261230: GC tracing of page sizes are wrong in a few places
  • 40ae993: 8261027: AArch64: Support for LSE atomics C++ HotSpot code
  • 9ffabf3: 8252971: WindowsFileAttributes does not know about Unix domain sockets
  • ... and 12 more: https://git.openjdk.java.net/jdk/compare/1740de2a0d41d0689ae9bb069b93ab267a2b9495...master

Your commit was automatically rebased without conflicts.

Pushed as commit 6675775.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Feb 15, 2021

Thanks for the fix! This is probably the best we can get without spawning an external process to e.g. call sw_vers to get the actual, non-compat version number.

Unfortunately, the test java/lang/System/OsVersionTest.java will still fail on systems where the MacOS version has a patch number (>1), e.g. the current MacOS 11.2.1 - in line with Kevin's report about the behavior on an 11.0.1 system.

I will open another ticket and propose a test update that relaxes the check in the test a little...

@RogerRiggs RogerRiggs deleted the 8253702-osversion-test branch Mar 5, 2021
@marcphilipp
Copy link

@marcphilipp marcphilipp commented Sep 24, 2021

With macOS 11.6 this fix no longer works. @sormuras will create a proper bug report. 😉

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Sep 24, 2021

Hi Marc,
cc @sormuras,

not necessary, it was already reported & fixed: https://bugs.openjdk.java.net/browse/JDK-8269850 😄

Cheers
Christoph

@marcphilipp
Copy link

@marcphilipp marcphilipp commented Sep 24, 2021

Thanks! I searched the bug tracker but couldn't find it. Will that be backported to JDK 11 and/or 17?

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Sep 24, 2021

Yup:
openjdk/jdk17u#85
openjdk/jdk11u#12

For OpenJDK 11u I hope to get it into the October Update. Can't speak for 17u as this is maintained by Oracle. Maybe @sormuras can help here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
5 participants