Skip to content

Fix IsOSVersionAtLeast when build or revision are not provided#108748

Merged
kotlarmilos merged 17 commits into
dotnet:mainfrom
kotlarmilos:bugfix/maccatalyst-getosversion
Oct 24, 2024
Merged

Fix IsOSVersionAtLeast when build or revision are not provided#108748
kotlarmilos merged 17 commits into
dotnet:mainfrom
kotlarmilos:bugfix/maccatalyst-getosversion

Conversation

@kotlarmilos

@kotlarmilos kotlarmilos commented Oct 10, 2024

Copy link
Copy Markdown
Member

Description

This PR fixes #108694 by updating IsOSVersionAtLeast to treat unspecified build or revision components as zero.

Breaking change notice: dotnet/docs#43156

@am11 am11 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍
It's not a regression so not sure if it is eligible for backport.

kotlarmilos and others added 2 commits October 10, 2024 13:29
…Version.MacCatalyst.cs

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@kotlarmilos

Copy link
Copy Markdown
Member Author

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos kotlarmilos changed the title Fix GetOSVersion on MacCatalyst when build or revision are not provided Fix IsOSVersionAtLeast when build or revision are not provided Oct 10, 2024
@kotlarmilos kotlarmilos changed the title Fix IsOSVersionAtLeast when build or revision are not provided Fix IsOSVersionAtLeast when minor/build/revision are not provided Oct 10, 2024
@kotlarmilos

Copy link
Copy Markdown
Member Author

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment thread src/libraries/System.Private.CoreLib/src/System/OperatingSystem.cs Outdated
@kotlarmilos kotlarmilos changed the title Fix IsOSVersionAtLeast when minor/build/revision are not provided Fix IsOSVersionAtLeast when build or revision are not provided Oct 10, 2024
@kotlarmilos

Copy link
Copy Markdown
Member Author

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment thread src/libraries/System.Private.CoreLib/src/System/OperatingSystem.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/OperatingSystem.cs Outdated
kotlarmilos and others added 2 commits October 10, 2024 15:37
…m.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…m.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@kotlarmilos

Copy link
Copy Markdown
Member Author

I still think that's a change that we should make, but I don't feel too strongly about it.

@jkotas What is the cost of filling a breaking change notice?

Not related to this PR, but something that seems a bit inconsistent. When using the full constructor overload, it requires the build and revision components to be >= 0, otherwise they are unspecified.

This causes an issue on MacCatalyst with the following code:

Version current = Environment.OSVersion.Version;
Version newVersion = new Version(current.Major, current.Minor, current.Build, current.Revision); // This will throw ArgumentOutOfRange exception

Comment thread src/libraries/System.Private.CoreLib/src/System/OperatingSystem.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/OperatingSystem.cs Outdated
@am11

am11 commented Oct 18, 2024

Copy link
Copy Markdown
Member

This causes an issue on MacCatalyst with the following code:

Version current = Environment.OSVersion.Version;
Version newVersion = new Version(current.Major, current.Minor, current.Build, current.Revision); // This will throw ArgumentOutOfRange exception

I think user should check the values in this case, or they can use current.Clone(); to get a copy.

kotlarmilos and others added 2 commits October 18, 2024 13:01
…m.cs

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
…m.cs

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@jkotas

jkotas commented Oct 18, 2024

Copy link
Copy Markdown
Member

@jkotas What is the cost of filling a breaking change notice?

https://github.com/dotnet/runtime/blob/main/docs/project/breaking-change-process.md has the instructions

@jkotas

jkotas commented Oct 18, 2024

Copy link
Copy Markdown
Member

The rationale is that it would align the behavior between iOS and Mac Catalyst.

Why is the behavior between iOS and Mac Catalyst different at the OS level?

@filipnavara

filipnavara commented Oct 18, 2024

Copy link
Copy Markdown
Member

Why is the behavior between iOS and Mac Catalyst different at the OS level?

It’s really not different but the version values are retrieved in a different way. Both iOS and macOS use version numbers with 2 or 3 components and we were always normalizing that to 3 (by retrieving the version as NSOperatingSystemVersion and then constructing System.Version from 3 individual numbers).

Mac Catalyst is just macOS app like any other calling the same macOS libraries like a normal Mac app would and having the same structure. The principal difference is that the SDK exposes different parts of the API surface but at the end of the day it still calls the same libraries. There are some quirk flags to make some APIs behave like iOS but that mostly appeared when Apple added support for running unmodified iOS apps on ARM macOS.

When it comes to Mac Catalyst versioning there is a mapping between a macOS version and the supported iOS API surface version. Retrieving NSOperatingSystemVersion would return the macOS version, not the Mac Catalyst version. The iOS support version is stored in an XML PList file where we read it from (same as LLVM does in the support libraries for OS version checks). We get the version as a string and naively pass it to Version.Parse. We just never normalized it to 3 components in this case. If the PList files has version with 2 components then it's parsed as 2.

@jeffhandley jeffhandley added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Oct 20, 2024
@dotnet-policy-service dotnet-policy-service Bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 20, 2024
@kotlarmilos kotlarmilos removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 23, 2024
@kotlarmilos

Copy link
Copy Markdown
Member Author

/ba-g browser-wasm timeouts

@kotlarmilos kotlarmilos merged commit 8e8143e into dotnet:main Oct 24, 2024
@kotlarmilos

Copy link
Copy Markdown
Member Author

/backport to release/9.0-staging

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/11496583371

@github-actions

Copy link
Copy Markdown
Contributor

@kotlarmilos an error occurred while backporting to release/9.0-staging, please check the run log for details!

Error: The specified backport target branch release/9.0-staging wasn't found in the repo.

@kotlarmilos

kotlarmilos commented Oct 24, 2024

Copy link
Copy Markdown
Member Author

The IsOSVersionAtLeast​ fix will be backported to .NET 9 in a servicing release without the breaking change once the release/9.0-staging becomes available.

@kotlarmilos

Copy link
Copy Markdown
Member Author

/backport to release/9.0-staging

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/11571315974

@github-actions github-actions Bot locked and limited conversation to collaborators Nov 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Runtime breaking-change Issue or PR that represents a breaking API or functional change over a previous release. os-maccatalyst MacCatalyst OS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MacCatalyst] OperatingSystem is currently not working

7 participants