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

SupportedOSPlatformVersion and minSdkVersion interactions #8040

Closed
pjcollins opened this issue May 15, 2023 · 7 comments
Closed

SupportedOSPlatformVersion and minSdkVersion interactions #8040

pjcollins opened this issue May 15, 2023 · 7 comments
Assignees
Labels
Area: App+Library Build Issues when building Library projects or Application projects.
Milestone

Comments

@pjcollins
Copy link
Member

pjcollins commented May 15, 2023

We have an issue where we will sometimes generate an AndroidManifest.xml file with a minSdkVersion element of 19, a version lower than our supported minimum of 21. I started on a fix in #8026 but wanted to discuss the user experience around this further.

I would propose the following:

  1. If the user does not set minSdkVersion explicitly in their manifest, we should always set minSdkVersion to SupportedOSPlatformVersion, regardless of whether the manifest has a targetSdkVersion set or not. We shouldn't flip this value arbitrarily to 19 (or 21) if targetSdkVersion is set.
  2. We should introduce build errors for cases where the user sets SupportedOSPlatformVersion < 21 or minSdkVersion < 21. The .NET SDK may have one we can hook into for the former, but we will probably need to write our own for the latter.
  3. If the user sets a minSdkVersion explicitly in their manifest and it does not match SupportedOSPlatformVersion, we should prefer SupportedOSPlatformVersion and warn the user.

The # 3 proposal above is potentially the larger behavior change to consider. Folks may be more accustomed to setting minSdkVersion in their manifests, as I don’t believe we’ve had a corresponding MSBuild property for this in the past. However, I think we want to be moving in a direction where we steer folks away from these kind of platform specific settings. In that regard preferring SupportedOSPlatformVersion over minSdkVersion feels like the better behavior.

@pjcollins pjcollins added the Area: App+Library Build Issues when building Library projects or Application projects. label May 15, 2023
@pjcollins pjcollins added this to the .NET 8 milestone May 15, 2023
@pjcollins pjcollins self-assigned this May 15, 2023
@ghost ghost added the needs-triage Issues that need to be assigned. label May 15, 2023
@pjcollins
Copy link
Member Author

Also looping in @rolfbjarne and @Redth for any thoughts/comments or feature parity considerations on other platforms.

@pjcollins pjcollins removed the needs-triage Issues that need to be assigned. label May 15, 2023
@jonathanpeppers
Copy link
Member

The only note is that you can leave SupportedOSPlatformVersion blank in your project.

And what the .NET SDK does in this case is just use the target version as minimum, so these:

<TargetFramework>net7.0-android</TargetFramework>
<TargetFramework>net7.0-android33</TargetFramework>
<TargetFramework>net7.0-android33.0</TargetFramework>

All default to SupportedOSPlatformVersion=33.0 if it was left blank.

So is <uses-sdk android:minSdkVersion="33" android:targetSdkVersion="33" /> ok for that case?

We could maybe attempt to set a default for SupportedOSPlatformVersion somehow, to make it 21 if blank.

@jpobst
Copy link
Contributor

jpobst commented May 15, 2023

So is <uses-sdk android:minSdkVersion="33" android:targetSdkVersion="33" /> ok for that case?

I don't think there's ~ever a case where this is desired. Setting a minSdkVersion to something like 33 basically means you can't run your app anywhere.

We could maybe attempt to set a default for SupportedOSPlatformVersion somehow, to make it 21 if blank.

This feels like what we should be trying to do.

@pjcollins
Copy link
Member Author

pjcollins commented May 15, 2023

So is <uses-sdk android:minSdkVersion="33" android:targetSdkVersion="33" /> ok for that case?

I don't think there's ~ever a case where this is desired. Setting a minSdkVersion to something like 33 basically means you can't run your app anywhere.

This is our current behavior for projects which both a) include an AndroidManifest.xml that doesn't declare a uses-sdk element, and b) do not explicitly set SupportedOSPlatformVersion. Anyone who happened to remove the SupportedOSPlatformVersion value from our template projects today would end up in this scenario.

It sounds like it may be a better experience to set SupportedOSPlatformVersion to AndroidMinimumDotNetApiLevel (21) if unset however, rather than have it default to TargetPlatformVersion?

@rolfbjarne
Copy link
Member

So for iOS and our other Apple platforms:

  1. If SupportedOSPlatformVersion is not specified in the csproj, we have a default version we use. This is default version is defined here: https://github.com/xamarin/xamarin-macios/blob/cdac4507a21b19c2a9249aba6f009630b5d0de99/Make.versions#L75-L86 - note that while the comment there says we shouldn't bump the version within a .NET version, we ran into problems with that and we bumped it last year for Xcode 14.1. Note that this happens even if there's a value in Info.plist (because by the time we read Info.plist, it's too late to change SupportedOSPlatformVersion).

  2. If the SupportedOSPlatformVersion or the value in the Info.plist is too low, we show an error:

     MM0073: Microsoft.macOS 13.3.7099 does not support a deployment target of 10.10 for macOS (the minimum is 10.14). Please select a newer deployment target in your project's Info.plist.
    
  3. If the SupportedOSPlatformVersion and the Info.plist values don't match, we show an error:

     error : The LSMinimumSystemVersion value in the Info.plist (11.0) does not match the SupportedOSPlatformVersion value (10.10) in the project file (if there is no SupportedOSPlatformVersion value in the project file, then a default value has been assumed). Either change the value in the Info.plist to match the SupportedOSPlatformVersion value, or remove the value in the Info.plist (and add a SupportedOSPlatformVersion value to the project file if it doesn't already exist).
    

@pjcollins
Copy link
Member Author

@rolfbjarne excellent thanks, some follow up thoughts:

  1. I was thinking a min version mismatch could only result in a warning, but perhaps we also want an error message for parity?
  2. You prefer SupportedOSPlatformVersion over a value that a customer may set in Info.plist (or in our case AndroidManifest.xml), we probably should do the same as proposed for parity.
  3. It looks like your default SupportedOSPlatformVersion values more closely match ~latest OS versions? It seems like we're leaning towards a default value that matches the min version in dotnet/runtime (much older). What is the reasoning behind using something recent as a default rather than minimums we've defined elsewhere (ios 16.1 vs ios 11 for example)?

@rolfbjarne
Copy link
Member

I was thinking a min version mismatch could only result in a warning, but perhaps we also want an error message for parity?

  • Changing the min version in a file and not see any change in the app bundle might be confusing (if they changed the file that doesn't matter if the other file takes precedence).
  • An error is easy enough to fix for the customer.

So I think an error is best, that way we don't have apparently confusing behavior.

You prefer SupportedOSPlatformVersion over a value that a customer may set in Info.plist (or in our case AndroidManifest.xml), we probably should do the same as proposed for parity.

In the initial implementation for .NET 5, it turned out I needed a rather big refactoring in order to be able to read Info.plist early enough to set SupportedOSPlatformVersion correctly (for instance it needs to be set before the analyzers checks for API usage, but iirc there were reasons to set it even earlier than that). So I decided to require setting SupportedOSPlatformVersion in order to change the default, and then allowing a value in the Info.plist file if it existed (to make migration somewhat easier) - but only the value same as SupportedOSPlatformVersion, thinking that we could always look into the bigger refactoring if it became a problem. However, it seems to have worked rather well, there were a few people asking questions in the beginning, but it's been a while now since we've had any customers ask about this.

  1. What is the reasoning behind using something recent as a default rather than minimums we've defined elsewhere (ios 16.1 vs ios 11 for example)?
  1. Xcode's default is the latest OS version (which I guess makes sense because Apple likes to push the newest OS out).
  2. Apple also often conditions improvements on the target OS version: if an app targets iOS 10.0, it won't run as fast as an app that targets iOS 15.0, even when running on a device with iOS 15.0.
  3. It's much less common to have devices with older iOS versions around. I'm pretty sure very few of our end users have the min iOS version we support (10.0 in .NET 7, will be 11.0 in .NET 8). It's also not possible to test these older OS versions in the simulator (they won't run on newer macs, and newer Xcodes won't run on older Macs).

So I don't see a problem with having different behavior between Android and iOS for the default min version, since there are clear differences between the platforms.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: App+Library Build Issues when building Library projects or Application projects.
Projects
None yet
Development

No branches or pull requests

5 participants