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

Fix IsOSVersionAtLeast when build or revision are not provided #108748

Merged
merged 17 commits into from
Oct 24, 2024

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Oct 10, 2024

Description

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

Breaking change notice: dotnet/docs#43156

Copy link
Member

@am11 am11 left a comment

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
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

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
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@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
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@filipnavara
Copy link
Member

I confirmed that normalizing the build component on MacCatalys will affect the value of Environment.OSVersion.Version.ToString() (18.0 vs. 18.0.0), which would require filling a breaking change notice.

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

The rationale is that it would align the behavior between iOS and Mac Catalyst. The apps are often built from the same source tree just with different TFM, so I would expect the APIs to behave as close as possible. Internally, LLVM version checks are also normalizing the values but I cannot think of any way that would be user exposed.

@kotlarmilos
Copy link
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

@am11
Copy link
Member

am11 commented Oct 18, 2024

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.

@jkotas
Copy link
Member

jkotas commented Oct 18, 2024

@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
Copy link
Member

jkotas commented Oct 18, 2024

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
Copy link
Member

filipnavara commented Oct 18, 2024

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 prerelease. 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
Member Author

/ba-g browser-wasm timeouts

@kotlarmilos kotlarmilos merged commit 8e8143e into dotnet:main Oct 24, 2024
143 of 146 checks passed
@kotlarmilos
Copy link
Member Author

/backport to release/9.0-staging

Copy link
Contributor

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

Copy link
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
Copy link
Member Author

kotlarmilos commented Oct 24, 2024

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
Member Author

/backport to release/9.0-staging

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. 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