-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8359830: Incorrect os.version reported on macOS Tahoe 26 (Beta) #25865
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
Conversation
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
@jaikiran 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:
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 14 new commits pushed to the
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 |
Webrevs
|
I got access to a macOS 26 Beta and ran
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Give it a day or so for others to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I tested this on macOS 26. Without this patch os.version
is set to 16
; with this patch, it is correctly set to 26
.
I left one possible suggestion, if you want to consider it.
const char* envVal = getenv("SYSTEM_VERSION_COMPAT"); | ||
const bool versionCompatEnabled = envVal != NULL && strncmp(envVal, "1", 1) == 0; | ||
const bool requiresSpecialHandling = ((long) osVer.majorVersion == 10 && (long) osVer.minorVersion >= 16) | ||
|| ((long) osVer.majorVersion == 16 && (long) osVer.minorVersion >= 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we know Apple jumped from 15 --> 26, would it be more future-proof to change this test to something like this?
osVer.majorVersion >= 16 && osVer.majorVersion < 26
This would allow us to work when macOS 27 comes out, in case it reports itself as "17.0" if we don't update the version of Xcode before then.
I can also see the argument for leaving it as you have done. And my guess is that Apple will report macOS 27 as 27 regardless, so this probably doesn't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Kevin,
I can also see the argument for leaving it as you have done.
I gave your suggestion some thought. I think it would be simpler (and less confusing) to leave that check in the current form and update it as and when needed depending on whether macOS does report future versions as two different values.
Are there changes for os_bsd.cpp too? |
Hello Alan,
That's a good catch. I did not know that we have This |
It looks like an existing test |
Hello Alan, I've been pursuing this further and there's a way to address this issue in |
For those who are curious, the work in progress (untested) changes look like this 0d7f593 |
I spoke to Alan about this and Alan says it's OK to do the |
tier1, tier2 and tier3 testing in our CI with this change and tier1 testing on macOS 26 Beta has completed without any related issues. I'll need a re-review please to go ahead with the integration. |
Thank you all for the reviews and inputs on this one. |
/integrate |
Going to push as commit 8df6b2c.
Your commit was automatically rebased without conflicts. |
/backport :jdk25 |
@jaikiran the backport was successfully created on the branch backport-jaikiran-8df6b2c4-jdk25 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk25, just click the following link: The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:
If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk:
|
I've created https://bugs.openjdk.org/browse/JDK-8360708 to track this. |
Can I please get a review of this change which proposes to address the issue noted in https://bugs.openjdk.org/browse/JDK-8359830?
macOS operating system's newer version 26 (currently in Beta) is reported as a 16 by older versions of XCode. JDK internally uses the
NSProcessInfo
andNSOperatingSystemVersion
APIs to identify the macOS version and set theos.version
system property to that value. The current recommended version to build the JDK on macOS is XCode 15.4. TheNSOperatingSystemVersion
API on that version of XCode reports macOS version as 16 instead of 26.The commit in this PR updates the JDK code to handle this mismatch and set the
os.version
appropriately to 26. This fix is similar to what we did with macOS BigSur when the macOS version 10.16 was meant to mean 11 https://bugs.openjdk.org/browse/JDK-8253702.The existing
OsVersionTest
has been updated for some trivial clean up. Existing tests in tier1, tier2 and tier3 continue to pass with this change. If anyone has access to a macOS 26 Beta, I request them to build this change and runtier1
tests to help verify that there aren't any failures.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25865/head:pull/25865
$ git checkout pull/25865
Update a local copy of the PR:
$ git checkout pull/25865
$ git pull https://git.openjdk.org/jdk.git pull/25865/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25865
View PR using the GUI difftool:
$ git pr show -t 25865
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25865.diff
Using Webrev
Link to Webrev Comment