[XABT] Prefer `SupportedOSPlatformVersion` over `minSdkVersion` by pjcollins · Pull Request #8026 · dotnet/android

pjcollins · 2023-05-09T22:06:46Z

Context: #8040

I noticed that we were not writing a valid minSdkVersion value to the
generated AndroidManifest.xml file for projects which included a
manifest that declared a targetSdkVersion. In these cases, we would
always write a minSdkVersion of 19 to the manifest file, as this is
the min version that our NDK supports.

Fix this to always use the value of $(SupportedOSPlatformVersion) as
the minSdkVersion element in the AndroidManifest.xml file. If this
value is not explicitly set in the project file, it will now default to
$(AndroidMinimumSupportedApiLevel) instead of
$(TargetPlatformVersion).

The XA4216 error/warning code has been repurposed.

The warning that would display when the minSdkVersion element in
AndroidManifest.xml was less than our minimum supported API level has
been converted into an error:

error XA4216: The deployment target '19.0' is not supported (the minimum is '21'). Please increase (or remove) the //uses-sdk/@android:minSdkVersion value in your AndroidManifest.xml.

An error condition has been added for when the
$(SupportedOSPlatformVersion) property value is less than
$(AndroidMinimumSupportedApiLevel):

error XA4216: The deployment target '19.0' is not supported (the minimum is '21'). Please increase the $(SupportedOSPlatformVersion) property value in your project file.

An error condition has been added for when the minSdkVersion element
in AndroidManifest.xml does not match $(SupportedOSPlatformVersion):

error XA1036: AndroidManifest.xml //uses-sdk/@android:minSdkVersion '19' does not match the $(SupportedOSPlatformVersion) value '21.0' 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 AndroidManifest.xml to match the $(SupportedOSPlatformVersion) value, or remove the value in the AndroidManifest.xml (and add a $(SupportedOSPlatformVersion) value to the project file if it doesn't already exist).

A few multidex related tests have been updated/removed as they are only
valid with a minSdkVersion of 20 or lower.

I noticed that we were not writing a valid `minSdkVersion` value to the generated `AndroidManifest.xml` file for projects which included a manifest that declared a `targetSdkVersion`. In these cases, we would always write a `minSdkVersion` of `19` to the manifest file, as this is the min version that our NDK supports. Fix this to always write the value of `$(SupportedOSPlatformVersion)`. If this value is not explicitly set in the project file, it will default to `$(TargetPlatformVersion)`. A couple of multidex related tests have been added to the ignore list as they are only valid with a minSdkVersion of 20 or lower.

… to 21

jonpryor · 2023-05-19T19:03:06Z

- message: Properties.Resources.XA4216_MinSdkVersion,
- messageArgs: new object [] { min_sdk?.Value, XABuildConfig.NDKMinimumApiAvailable }
- );
+ if (min_sdk != null) {


Aside: the amount of indentation going on here has my crying out for a helper method 3 levels of indentation ago…

jonpryor · 2023-05-19T19:07:30Z

+ }
 }
- if (target_sdk != null && (!int.TryParse (target_sdk.Value, out int targetSdkVersion) || targetSdkVersion < XABuildConfig.NDKMinimumApiAvailable)) {
+ if (target_sdk != null && (!int.TryParse (target_sdk.Value, out int targetSdkVersion) || targetSdkVersion < XABuildConfig.AndroidMinimumDotNetApiLevel)) {


Similar/different to https://github.com/xamarin/xamarin-android/pull/8026/files#r1199283399 , if targetSdkVersion is < $(AndroidMinimumDotNetApiLevel), should this be an error?
This doesn't feel reasonable, and I wonder what Java would do with:
<uses-sdkandroid:minSdkVersion="21"android:targetSdkVersion="19" />
Maybe this should remain a warning? This just feels bananas to me.

Maybe both XA4216 warnings should be repurposed and converted to errors?

It seems that Android studio does not produce a warning or error when targetSdkVersion < minSdkVersion, so we can probably leave the targetSdkVersion condition untouched.
I'll try to merge XA1036 into XA4216_MinSdkVersion however, as keeping that warning is redundant.

I've repurposed XA4216, it will continue to warn in the case of targetSdkVersion, but will now produce errors for:
SupportedOSPlatformVersion < minimum version supported
minSdkVersion < minimum version supported

jonpryor · 2023-05-19T19:20:18Z

"Meta" thought: please rebase atop 04e4bb1 or later, as 04e4bb1 enables a whole slew of CA1305 string-related errors which may (will?) be implicated with all the int.TryParse() calls here.

pjcollins · 2023-05-24T15:50:25Z

/azp run

azure-pipelines · 2023-05-24T15:50:47Z

Azure Pipelines successfully started running 1 pipeline(s).

pjcollins · 2023-05-24T22:21:30Z

Looks like the only test failures are InstallAndroidDependenciesTest which are happening elsewhere, I think this should be good to go.

jonpryor · 2023-05-26T17:24:17Z

+ var failedToParseMinSdk = !int.TryParse (min_sdk.Value, out int minSdkVersion);
+
+ if (failedToParseMinSdk || minSdkVersion < XABuildConfig.AndroidMinimumDotNetApiLevel) {
+ Log.LogCodedError ("XA4216", Properties.Resources.XA4216_MinSdkVersion, min_sdk?.Value, XABuildConfig.AndroidMinimumDotNetApiLevel);


Is it possible to provide a filename for the error message?

We could extend https://github.com/xamarin/xamarin-android-tools/blob/main/src/Microsoft.Android.Build.BaseTasks/MSBuildExtensions.cs#L142 for the AndroidManifest case, but I don't know of an easy way to report what file is setting SupportedOSPlatformVersion (it will likely be set in the project file, but could be in a directory.build.props or any other import).

I added dotnet/android-tools#209 so that we can better link to the manifest file in this error.

jonpryor · 2023-05-26T17:24:23Z

+ }
+
+ if (failedToParseMinSdk || minSdkVersion != supportedOsPlatformVersionAsInt) {
+ Log.LogCodedError ("XA1036", Properties.Resources.XA1036, min_sdk?.Value, SupportedOSPlatformVersion);


Is it possible to provide a filename for the error message?

I don't know of an easy way to report what file is setting SupportedOSPlatformVersion (it will likely be set in the project file, but could be in a directory.build.props or any other import).

jonpryor · 2023-05-26T17:32:54Z

squash-and-merge commit message:

Fixes: https://github.com/xamarin/xamarin-android/issues/8040
I noticed that we were not writing a supported (by us)
`//uses-sdk/@android:minSdkVersion` value to the generated
`AndroidManifest.xml` file for projects which included a manifest
that declared a `targetSdkVersion`. In these cases, we would
always write a `//uses-sdk/@android:minSdkVersion` value of `21` to
`AndroidManifest.xml`, as this is the minimum version that MonoVM
supports.
Fix this to always use the value of `$(SupportedOSPlatformVersion)`
as the `minSdkVersion` attribute in the `AndroidManifest.xml` file.
If this value is not explicitly set in the project file, it will now
default to `$(AndroidMinimumSupportedApiLevel)` instead of
`$(TargetPlatformVersion)`.
The `XA4216` error/warning code has been expanded+repurposed.
The warning that would display when the `minSdkVersion` attribute in
`AndroidManifest.xml` was less than our minimum supported API level
has been converted into an error:
error XA4216: The deployment target '19' is not supported (the minimum is '21').Please increase (or remove) the //uses-sdk/@android:minSdkVersion value in your project file.
A similar error is now reported when `$(SupportedOSPlatformVersion)`
is less than `$(AndroidMinimumSupportedApiLevel)`error XA4216: The deployment target '19' is not supported (the minimum is '21').Please increase the $(SupportedOSPlatformVersion) property value in your project file.
An error condition has been added for when
`//uses-sdk/@android:minSdkVersion` in `AndroidManifest.xml` does not
match `$(SupportedOSPlatformVersion)`:
error XA1036: AndroidManifest.xml //uses-sdk/@android:minSdkVersion '19' does not match the $(SupportedOSPlatformVersion) value '21.0' 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 AndroidManifest.xml to match the $(SupportedOSPlatformVersion) value, or remove the value in the AndroidManifest.xml (and add a $(SupportedOSPlatformVersion) value to the project file if it doesn't already exist).
A few multidex related tests have been updated/removed as they are
only valid with a minSdkVersion of 20 or lower.

pjcollins · 2023-05-26T18:52:40Z

One comment for the message:

 In these cases, we would
always write a `//uses-sdk/@android:minSdkVersion` value of `21` to
`AndroidManifest.xml`, as this is the minimum version that MonoVM
supports.

We were previously writing minSdkVersion="19" to manifests, and with these changes we will now default to 21 if a value is not provided.

* main: [XABT] Prefer `SupportedOSPlatformVersion` over `minSdkVersion` (dotnet#8026) Bump to xamarin/Java.Interop/main@72b041a (dotnet#8089) Bump LibZipSharp to 3.0.0 (dotnet#8061) Bump to xamarin/Java.Interop/main@8c9eece (dotnet#8073) [profiled-aot] update AOT profile for .NET 8 Preview 5 (dotnet#8077) Localized file check-in by OneLocBuild Task (dotnet#8078) Bump to dotnet/installer@6150605bd0 8.0.100-preview.6.23276.3 (dotnet#8083)

pjcollins requested review from dellis1972 and jonathanpeppers as code owners May 9, 2023 22:06

pjcollins force-pushed the dev/pjc/minsdknet branch from c13f092 to b70ed5fCompare May 10, 2023 16:16

jonathanpeppers reviewed May 10, 2023
View reviewed changes

Comment threadsrc/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestTest.cs Outdated

pjcollins force-pushed the dev/pjc/minsdknet branch from b70ed5f to 85ea94cCompare May 10, 2023 22:00

jonpryor reviewed May 11, 2023
View reviewed changes

Comment threadsrc/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs Outdated

jonpryor reviewed May 11, 2023
View reviewed changes

Comment threadsrc/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs Outdated

jonpryor reviewed May 11, 2023
View reviewed changes

Comment threadsrc/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs Outdated

jonpryor reviewed May 11, 2023
View reviewed changes

Comment threadsrc/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestTest.cs Outdated

pjcollins force-pushed the dev/pjc/minsdknet branch from 434a36c to 2d9d8eeCompare May 11, 2023 19:59

pjcollins requested review from jonathanpeppers and jonpryor May 12, 2023 18:57

Remove custom multidex tests/asserts
488d4d1

pjcollins changed the title ~~[XABT] Do not prefer NDKMinimumApiAvailable~~[XABT] Update NDKMinimumApiAvailable to 21May 15, 2023

pjcollins marked this pull request as draft May 15, 2023 15:44

pjcollins changed the title ~~[XABT] Update NDKMinimumApiAvailable to 21~~[XABT] Do not prefer NDKMinimumApiAvailable over SupportedOSPlatformVersionMay 15, 2023

pjcollins mentioned this pull request May 15, 2023
SupportedOSPlatformVersion and minSdkVersion interactions #8040
Closed

pjcollins added 2 commits May 15, 2023 17:43

Remove NDKMinimumApiAvailable, set SupportedOSPlatformVersion default…
aec6d5f
… to 21

Add error conditions
cdf5b41

pjcollins changed the title ~~[XABT] Do not prefer NDKMinimumApiAvailable over SupportedOSPlatformVersion~~[XABT] Prefer SupportedOSPlatformVersion over minSdkVersionMay 17, 2023

pjcollins added 4 commits May 17, 2023 16:56

Fix error text in readme
5275998

Fix tests
a8315b9

Typo
5127ce4

Fix tests
a3b5cad

pjcollins marked this pull request as ready for review May 18, 2023 15:04

pjcollins requested a review from grendello as a code owner May 18, 2023 15:04

pjcollins requested review from jpobst and rolfbjarne May 18, 2023 15:23

rolfbjarne approved these changes May 18, 2023
View reviewed changes

Comment threadsrc/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs Outdated
Comment threadsrc/Xamarin.Android.Build.Tasks/Tasks/GetJavaPlatformJar.cs Outdated