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

Ensure MinimumOSVersion is is consistent withSupportedOSPlatformVersion #12336

Closed
mhutch opened this issue Aug 3, 2021 · 15 comments · Fixed by #12638
Closed

Ensure MinimumOSVersion is is consistent withSupportedOSPlatformVersion #12336

mhutch opened this issue Aug 3, 2021 · 15 comments · Fixed by #12638
Assignees
Labels
dotnet An issue or pull request related to .NET (6) dotnet-pri0 .NET 6: required for stable release
Milestone

Comments

@mhutch
Copy link
Member

mhutch commented Aug 3, 2021

We should ensure that the MinimumOSVersion property in the Info.plist is consistent with the SupportedOSPlatformVersion MSBuild property. SupportedOSPlatformVersion is a standardized across platforms and controls the SDK's platform analyzers.

https://github.com/dotnet/designs/search?q=SupportedOSPlatformVersion

A few options:

  • emit a build error if the MinimumOSVersion property and the SupportedOSPlatformVersion MSBuild property do not match
  • merge the SupportedOSPlatformVersion MSBuild property into the MinimumOSVersion plist value during app build
  • set the SupportedOSPlatformVersion MSBuild property from the MinimumOSVersion plist property in a build target at start of build. the downside would be that it would not be available during evaluation.
@mhutch
Copy link
Member Author

mhutch commented Aug 3, 2021

It appears we do emit the SupportedOSPlatformAttribute annotations, so the SupportedOSPlatformVersion property will be used by the platform check analyzers when building in Mac/iOS projects.

@spouliot spouliot added this to the .NET 6 milestone Aug 3, 2021
@spouliot spouliot added dotnet-pri0 .NET 6: required for stable release dotnet An issue or pull request related to .NET (6) labels Aug 3, 2021
@rolfbjarne rolfbjarne self-assigned this Aug 4, 2021
@rolfbjarne
Copy link
Member

  1. Support a platform-specific version (iOSMinimumVersion, tvOSMinimumVersion, etc), and if set, use it to set SupportedOSPlatformVersion:

    <SupportedOSPlatformVersion Condition="'$(iOSMinimumVersion)'!=''">$(iOSMinimumVersion)</SupportedOSPlatformVersion>

    One question is which value should take precedence if both are specified: I feel that iOSMinimumVersion is more specific than SupportedOSPlatformVersion, and if I set both, I'd expect iOSMinimumVersion to be applied to iOS project (only).

    We do this before .NET defaults SupportedOSPlatformVersion to TargetPlatformVersion (in Xamarin.Shared.Sdk.props for instance),

  2. This means that we'll always have a value for SupportedOSPlatformVersion. Now the question becomes what to do if the Info.plist specifies a value (where it's historically been set). Typically all current projects will have a value set, so this will be an issue for all legacy projects that have been upgraded to .NET.

    • Show an error if the values don't match.
      • Con: this can be annoying for upgraded projects that already have a value in Info.plist, if that value doesn't happen to match whatever SupportedOSPlatformVersion ends up being.
      • Pro: predictable behavior. We should be able to write a good error message that tells people what to do.
    • Make Info.plist override whatever is specified in SupportedOSPlatformVersion.
      • Con: this can result in confusing/incorrect behavior if other build logic reads SupportedOSPlatformVersion before we can set it to the value from the Info.plist.
      • Pro: this matches how other values in Info.plist work (whatever is set in the Info.plist takes precedence over any MSBuild properties).

    I lean towards the first option, because:

    • It's matches how other platforms work.
    • Any problems with upgraded projects is a one-time cost, and a good error message should mitigate any problems.
    • Less chance of random/confusing/incorrect behavior if someone reads SupportedOSPlatformVersion before we've been able to set it to the correct value.

@dalexsoto
Copy link
Member

dalexsoto commented Aug 23, 2021

I lean towards the first option

+1, while painful on migration it is a one time thing instead of the can of worms of option 2

@mhutch
Copy link
Member Author

mhutch commented Aug 26, 2021

  1. Support a platform-specific version (iOSMinimumVersion, tvOSMinimumVersion, etc), and if set, use it to set SupportedOSPlatformVersion:

Yes, the spec originally recommended this {PlatformIdentifier}MinimumVersion per-platform property naming convention, but I'm no longer sure we should use it. There are many other properties that may need to be set per platform, and it would be inconsistent and confusing if this pattern were only available for some properties on some platforms.

It would be much better if MSBuild had a clean, consistent way to specify per-TFM and per-platform property values. It might look something like this:

<!-- this kind of platform condition doesn't actually exist -->
<PropertyGroup Platform="ios">
   <SupportedOSPlatformVersion>13.0<SupportedOSPlatformVersion>
</PropertyGroup>

Unfortunately, this kind of simple platform condition does not exist today. There were several discussions on this topic but we did not settle upon a solution. So right now there isn't a good option for this.

If you tried to do a condition like this with MSBuild today, you might try something like this:

<!-- this kind of platform condition cannot be used for properties in a csproj -->
<PropertyGroup Condition="'$(TargetPlatformIdentifier)'=='ios'">
   <SupportedOSPlatformVersion>13.0<SupportedOSPlatformVersion>
</PropertyGroup>

This platform condition is indeed valid, but only works in some places. It doesn't work for properties and property groups in a csproj, but does work in most targets files. This is because the TargetPlatformIdentifier property is not an intrinsic feature, but is set by the SDK targets, which are evaluated after the csproj file, and decompose the TargetFramework into its components.

Somewhat confusingly, the TargetPlatformIdentifier property is able to be used in conditions on items and item groups. This is because item evaluation takes place in a second pass after the property evaluation pass.

So how do you do it for real? The "correct" way is this incredibly ugly and difficult to read condition:

<!-- this platform condition is correct but ugly -->
<PropertyGroup Condition="'$([MSBuild]::GetTargetPlatformIdentifier(&quot;$(TargetFramework)&quot;'))'=='ios'">
   <SupportedOSPlatformVersion>13.0<SupportedOSPlatformVersion>
</PropertyGroup>

Checking the verbatim TargetFramework value looks cleaner but does not accurately express intent, and it's fragile, as it will break if you update the framework version or platform version on the TFM without updating all the conditions:

<!-- this platform condition is correct but fragile -->
<PropertyGroup Condition="'$(TargetFramework)'=='net6.0-ios15.0'">
   <SupportedOSPlatformVersion>13.0<SupportedOSPlatformVersion>
</PropertyGroup>

Right now, in my own projects I'd would probably define TargetPlatformIdentifier myself:

<!-- this platform condition is correct, and limits ugliness to one line -->
<PropertyGroup>
   <TargetFrameworks>net6.0;net6.0-android;net6.0-ios</TargetFrameworks>
   <TargetPlatformIdentifier Condition="'$(TargetFramework)'==''">$([MSBuild]::GetTargetPlatformIdentifier("$(TargetFramework)")</TargetPlatformIdentifier>
</PropertyGroup>

<PropertyGroup Condition="'$(TargetPlatformIdentifier)'=='ios'">
   <SupportedOSPlatformVersion>13.0<SupportedOSPlatformVersion>
</PropertyGroup>

<PropertyGroup Condition="'$(TargetPlatformIdentifier)'=='android'">
   <SupportedOSPlatformVersion>13.0<SupportedOSPlatformVersion>
</PropertyGroup>

Obviously this isn't something we'd want projects to have to do. We might have to just go with the pattern, at least for now.

/cc @richlander @KathleenDollard @Redth @dsplaisted @terrajobst @rainersigwald

@dsplaisted
Copy link

What does MinimumOSVersion do? Why do we have it in addition to SupportedOSPlatformVersion?

@mhutch
Copy link
Member Author

mhutch commented Aug 26, 2021

What does MinimumOSVersion do? Why do we have it in addition to SupportedOSPlatformVersion?

@dsplaisted yeah there's some overloading going on here. MinimumOSVersion is a property in the Info.plist app manifest on Apple platforms. It's not an MSBuild property.

MinimumOSVersion was originally also the name I originally used for an MSBuild property in the "minimum OS version" spec, but it got unified with the more general-purpose platform analyzers feature and the spec was updated to use SupportedOSPlatformVersion instead. There was never a real MSBuild property called MinimumOSVersion AFAIK.

@dsplaisted
Copy link

I see. In that case I would suggest this option, or something similar:

  • merge the SupportedOSPlatformVersion MSBuild property into the MinimumOSVersion plist value during app build

@filipnavara
Copy link
Contributor

This was [at least to some extent] addressed with #12516.

rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Sep 6, 2021
* Add support for the SupportedOSPlatformVersion MSBuild property, and write it
  to the Info.plist for the corresponding minimum OS version.
* If there are any minimum OS version in the Info.plist, and show an error if it
  doesn't match SupportedOSPlatformVersion.

This unfortunately means that if there's any minimum OS version in any Info.plist,
then that will most likely have to be moved to the SupportedOSPlatformVersion property
(or removed entirely if that's the right choice), since it's unlikely to match the
default value for SupportedOSPlatformVersion. However, this was deemed to be the
best option for the future (it's a one-time pain during migration).

Fixes xamarin#12336.
@rolfbjarne
Copy link
Member

I've added a PR that implements support for SupportedOSPlatformVersion, where it writes it to the Info.plist: #12638. If there are any existing values in the Info.plist, then we'll show an error if the two values don't match. That's basically my suggestion here: #12336 (comment), except that it doesn't support platform-specific properties.

rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Sep 7, 2021
…to the PartialAppManifest item group in their own targets. Fixes xamarin#12336.

Also add a test.

Fixes xamarin#12336 (for the second time).
rolfbjarne added a commit that referenced this issue Sep 8, 2021
* Add support for the SupportedOSPlatformVersion MSBuild property, and write
  it to the Info.plist for the corresponding minimum OS version.
* If there are any minimum OS version in the Info.plist, we'll now show an
  error if it doesn't match SupportedOSPlatformVersion.

This unfortunately means that if there's any minimum OS version in any
Info.plist, then that will most likely have to be moved to the
SupportedOSPlatformVersion property (or removed entirely if that's the right
choice), since it's unlikely to match the default value for
SupportedOSPlatformVersion. However, this was deemed to be the best option for
the future (it's a one-time pain during migration).

Also add new tests, update existing tests, and update the templates.

Fixes #12336.
@rolfbjarne
Copy link
Member

@Redth @jonathanpeppers @mattleibow this change will probably affect existing MAUI tests, since existing projects will have to move any min OS version in the Info.plist to the SupportedOSPlatformVersion property in the project file.

@jonathanpeppers
Copy link
Member

I made a similar change on Android for this: dotnet/android@a66013b

Maybe we need to do something for dotnet/maui singleproject for this? Maybe dotnet/maui's MSBuild targets should supply the minimum OS versions by default, so you don't have to put them in the .csproj for each platform?

@rolfbjarne
Copy link
Member

Maybe we need to do something for dotnet/maui singleproject for this? Maybe dotnet/maui's MSBuild targets should supply the minimum OS versions by default, so you don't have to put them in the .csproj for each platform?

You don't have to add them to the .csproj unless you want a specific version, there's a default value. The problem is that most existing projects will specify a value in the Info.plist, so those will have to move that to the project file (or remove it to get the default). Our templates has this version as an input parameter, so customers will choose the value they want when creating a new project (iow not a problem in that case).

@rmarinho
Copy link
Member

@rolfbjarne what's the value we should put on SupportedOSPlatformVersion ? @Redth was trying but it can't make it work was asking for 15.0

@rolfbjarne
Copy link
Member

@rmarinho try 14.2. Things got complicated with Xcode 13 RC 1, because Apple dropped the macOS 12.0 SDK (which is what iOS 15.0 should map to).

@mattleibow
Copy link
Contributor

I also notice that 13 is also missing :(

@ghost ghost locked as resolved and limited conversation to collaborators Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dotnet An issue or pull request related to .NET (6) dotnet-pri0 .NET 6: required for stable release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants