[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.

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestTest.cs

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestTest.cs

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

src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs

src/Xamarin.Android.Build.Tasks/Tasks/GetJavaPlatformJar.cs

src/Xamarin.Android.Build.Tasks/Properties/Resources.resx

src/Xamarin.Android.Build.Tasks/Tasks/GetJavaPlatformJar.cs

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

src/Xamarin.Android.Build.Tasks/Tasks/GetJavaPlatformJar.cs

-										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

src/Xamarin.Android.Build.Tasks/Tasks/GetJavaPlatformJar.cs

+								}
 							}
-							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-sdk android: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

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/XASdkTests.cs

...n.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/XamarinAndroidApplicationProject.cs

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/DotNetXamarinProject.cs

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.

src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets

src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs

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

src/Xamarin.Android.Build.Tasks/Tasks/GetJavaPlatformJar.cs

+								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

src/Xamarin.Android.Build.Tasks/Tasks/GetJavaPlatformJar.cs

+								}
+
+								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 b70ed5f Compare May 10, 2023 16:16

jonathanpeppers reviewed May 10, 2023

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestTest.cs Outdated Show resolved Hide resolved

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

jonpryor reviewed May 11, 2023

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs Outdated Show resolved Hide resolved

jonpryor reviewed May 11, 2023

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs Outdated Show resolved Hide resolved

jonpryor reviewed May 11, 2023

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs Outdated Show resolved Hide resolved

jonpryor reviewed May 11, 2023

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestTest.cs Outdated Show resolved Hide resolved

pjcollins force-pushed the dev/pjc/minsdknet branch from 434a36c to 2d9d8ee Compare 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 21 May 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 SupportedOSPlatformVersion May 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 minSdkVersion May 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

src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs Outdated Show resolved Hide resolved

src/Xamarin.Android.Build.Tasks/Tasks/GetJavaPlatformJar.cs Outdated Show resolved Hide resolved