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

[One .NET] implement $(SupportedPlatformOSVersion) for minSdkVersion #6111

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jul 21, 2021

Context: #6107 (comment)

In .NET 6, we are wanting AndroidManifest.xml in project templates
to no longer contain:

<uses-sdk android:minSdkVersion="21" android:targetSdkVersion="30" />

This is because we can define these values such as:

<TargetFramework>net6.0-android30</TargetFramework>
<SupportedPlatformOSVersion>21.0</SupportedPlatformOSVersion>

There is not really a reason to put this information in two places.
$(SupportedPlatformOSVersion) is also required for enabling C#
compiler warnings such as:

MainActivity.cs(28,4): warning CA1416: This call site is reachable on: 'Android' 21.0 and later. 'View.AccessibilityTraversalAfter.get' is only supported on: 'android' 22.0 and later.

This is an example of calling an API from 22, with a
$(SupportedPlatformOSVersion) for 21.

If <uses-sdk/> is still used in AndroidManifest.xml in a
project, it will be preferred as the final source of truth.

One other improvement is that $(SupportedPlatformOSVersion) is
expected to include .0, or you get the warning:

UnnamedProject.AssemblyInfo.cs(21,12): warning CA1418: Version '21' is not valid for platform 'Android'. Use a version with 2-4 parts for this platform.

Android unique in that this value is always an integer. In order for
users to be able to specify 21, I added a check in
Microsoft.Android.Sdk.DefaultProperties.targets so that we append
.0 when needed.

I added some tests around this scenario.

@jonathanpeppers jonathanpeppers force-pushed the dotnet-supportedosplatformversion branch from b4c3a27 to 635f2f3 Compare July 23, 2021 13:52
@jonathanpeppers jonathanpeppers marked this pull request as ready for review July 23, 2021 13:53
Copy link
Contributor

@dellis1972 dellis1972 left a comment

Choose a reason for hiding this comment

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

looks ok

@jonathanpeppers
Copy link
Member Author

This broke a lot of tests:

(_CheckForSupportedOSPlatformVersionHigherThanTargetPlatformVersion target) -> 
/Users/runner/Library/Android/dotnet/sdk/6.0.100-rc.1.21372.10/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(185,18):
error MSB4184: The expression "[MSBuild]::VersionGreaterThan(.0, 30.0)" cannot be evaluated. Version string was not in a correct format. 

I'm a bit confused on why I didn't see it locally, looking into it...

Context: xamarin#6107 (comment)

In .NET 6, we are wanting `AndroidManifest.xml` in project templates
to no longer contain:

    <uses-sdk android:minSdkVersion="21" android:targetSdkVersion="30" />

This is because we can define these values such as:

    <TargetFramework>net6.0-android30</TargetFramework>
    <SupportedPlatformOSVersion>21.0</SupportedPlatformOSVersion>

There is not really a reason to put this information in two places.
`$(SupportedPlatformOSVersion)` is also required for enabling C#
compiler warnings such as:

    MainActivity.cs(28,4): warning CA1416: This call site is reachable on: 'Android' 21.0 and later. 'View.AccessibilityTraversalAfter.get' is only supported on: 'android' 22.0 and later.

This is an example of calling an API from 22, with a
`$(SupportedPlatformOSVersion)` for 21.

If `<uses-sdk/>` is still *used* in `AndroidManifest.xml` in a
project, it will be preferred as the final source of truth.

One other improvement is that `$(SupportedPlatformOSVersion)` is
expected to include `.0`, or you get the warning:

    UnnamedProject.AssemblyInfo.cs(21,12): warning CA1418: Version '21' is not valid for platform 'Android'. Use a version with 2-4 parts for this platform.

Android unique in that this value is always an integer. In order for
users to be able to specify `21`, I added a check in
`Microsoft.Android.Sdk.DefaultProperties.targets` so that we append
`.0` when needed.

I added some tests around this scenario.
@jonathanpeppers jonathanpeppers force-pushed the dotnet-supportedosplatformversion branch from 635f2f3 to 0033d2f Compare July 26, 2021 19:00
@jonathanpeppers jonathanpeppers marked this pull request as ready for review July 29, 2021 20:52
@jonathanpeppers
Copy link
Member Author

The problem before was I needed to check if $(SupportedOSPlatformVersion)was blank:

<!-- $(SupportedOSPlatformVersion) must be '21.0', but we should support integer values like '21' -->
<SupportedOSPlatformVersion Condition=" '$(SupportedOSPlatformVersion)' != '' and !$(SupportedOSPlatformVersion.Contains('.')) ">$(SupportedOSPlatformVersion).0</SupportedOSPlatformVersion>

@jonathanpeppers jonathanpeppers merged commit a66013b into xamarin:main Jul 30, 2021
@jonathanpeppers jonathanpeppers deleted the dotnet-supportedosplatformversion branch July 30, 2021 15:44
jonpryor pushed a commit to jonpryor/xamarin-android that referenced this pull request Aug 25, 2021
…amarin#6111)

Context: xamarin#6107 (comment)

In .NET 6, we are wanting `AndroidManifest.xml` in project templates
to no longer contain:

    <uses-sdk android:minSdkVersion="21" android:targetSdkVersion="30" />

This is because we can define these values such as:

    <TargetFramework>net6.0-android30</TargetFramework>
    <SupportedPlatformOSVersion>21.0</SupportedPlatformOSVersion>

There is not really a reason to put this information in two places.
`$(SupportedPlatformOSVersion)` is also required for enabling C#
compiler warnings such as:

    MainActivity.cs(28,4): warning CA1416: This call site is reachable on: 'Android' 21.0 and later. 'View.AccessibilityTraversalAfter.get' is only supported on: 'android' 22.0 and later.

This is an example of calling an API from 22, with a
`$(SupportedPlatformOSVersion)` for 21.

If `<uses-sdk/>` is still *used* in `AndroidManifest.xml` in a
project, it will be preferred as the final source of truth.

One other improvement is that `$(SupportedPlatformOSVersion)` is
expected to include `.0`, or you get the warning:

    UnnamedProject.AssemblyInfo.cs(21,12): warning CA1418: Version '21' is not valid for platform 'Android'. Use a version with 2-4 parts for this platform.

Android unique in that this value is always an integer. In order for
users to be able to specify `21`, I added a check in
`Microsoft.Android.Sdk.DefaultProperties.targets` so that we append
`.0` when needed.

I added some tests around this scenario.
jonpryor pushed a commit that referenced this pull request Aug 26, 2021
…6111)

Context: #6107 (comment)

In .NET 6, we are wanting `AndroidManifest.xml` in project templates
to no longer contain:

    <uses-sdk android:minSdkVersion="21" android:targetSdkVersion="30" />

This is because we can define these values such as:

    <TargetFramework>net6.0-android30</TargetFramework>
    <SupportedPlatformOSVersion>21.0</SupportedPlatformOSVersion>

There is not really a reason to put this information in two places.
`$(SupportedPlatformOSVersion)` is also required for enabling C#
compiler warnings such as:

    MainActivity.cs(28,4): warning CA1416: This call site is reachable on: 'Android' 21.0 and later. 'View.AccessibilityTraversalAfter.get' is only supported on: 'android' 22.0 and later.

This is an example of calling an API from 22, with a
`$(SupportedPlatformOSVersion)` for 21.

If `<uses-sdk/>` is still *used* in `AndroidManifest.xml` in a
project, it will be preferred as the final source of truth.

One other improvement is that `$(SupportedPlatformOSVersion)` is
expected to include `.0`, or you get the warning:

    UnnamedProject.AssemblyInfo.cs(21,12): warning CA1418: Version '21' is not valid for platform 'Android'. Use a version with 2-4 parts for this platform.

Android unique in that this value is always an integer. In order for
users to be able to specify `21`, I added a check in
`Microsoft.Android.Sdk.DefaultProperties.targets` so that we append
`.0` when needed.

I added some tests around this scenario.
@jaldinger
Copy link

I understand this "new" way of specifying the minSdkVersion and targetSdkVersion, however, where can I now specify the targetFrameworkVersion that is used for compiling? The UI in VS2022 (17.3.0 preview 1) does not expose that in a .NET MAUI project, and all of the properties I commented have failed to fix the following warning, also hiding the API methods I require from previous versions:
image

@jonathanpeppers
Copy link
Member Author

TargetFrameworkVersion should always be 6 for .NET 6. You can see the design about this here:

https://github.com/dotnet/designs/blob/main/accepted/2021/net6.0-tfms/net6.0-tfms.md

For Android class libraries, you can put net6.0-android31 or net6.0-android32 if you really need this. If it is an Android application, I'd recommend putting net6.0-android and let the Android workload choose the default for you.

In the future, you might consider opening an issue instead of commenting on an old PR or comment in the #android channel on Discord. You might get a quicker response that way, thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants