[.NET 5] fix warning for $(TFV) of 5.0 #4598
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pjcollins
approved these changes
Apr 21, 2020
Nirmal4G
suggested changes
Apr 21, 2020
| /// <summary> | ||
| /// This is used instead of TargetFrameworkVersion for .NET 5 builds | ||
| /// </summary> | ||
| public int ApiLevel { get; set; } |
There was a problem hiding this comment.
Suggested change
| public int ApiLevel { get; set; } | |
| public int TargetApiLevel { get; set; } |
This is more clear in intent than the previous name.
Contributor
There was a problem hiding this comment.
Should this instead be string TargetAndroidId (plus associated chicanery and conversions), so that it can “reasonably” support preview API levels, such as this year’s current R (and presumably next year’s S, and...)?
AndroidManifest.xml has historically allowed //uses-sdk/@android:targetSdkVersion to contain values such as R, and we’ve historically allowed $(TargetFrameworkVersion) to be e.g. v10.0.99 to use the preview API level, and we need some form of equivalent in .NET 5, especially since we’ll be losing $(TargetFrameworkVersion)...
Member
Author
There was a problem hiding this comment.
@jonp for .NET 5 the incoming value here will be $(_AndroidApiLevel), which is currently coming from:
I don't think we have a way to set this to an unstable API level yet in .NET 5, but the value would be set to 30 instead of R.
I think the name ApiLevel is OK, because we've used this for other tasks that take $(_AndroidApiLevel) as input:
A couple older tasks use the name AndroidSdkPlatform, though.
|
|
||
| var compileSdk = MonoAndroidHelper.SupportedVersions.GetApiLevelFromFrameworkVersion (TargetFrameworkVersion); | ||
| var compileSdk = string.IsNullOrEmpty (TargetFrameworkVersion) ? | ||
| ApiLevel : |
There was a problem hiding this comment.
Suggested change
| ApiLevel : | |
| TargetApiLevel : |
| <Target Name="_CheckGoogleSdkRequirements" | ||
| Condition="Exists('$(IntermediateOutputPath)android\AndroidManifest.xml') And '$(AndroidEnableGooglePlayStoreChecks)' == 'true' "> | ||
| <CheckGoogleSdkRequirements | ||
| ApiLevel="$(_AndroidApiLevel)" |
There was a problem hiding this comment.
Suggested change
| ApiLevel="$(_AndroidApiLevel)" | |
| TargetApiLevel="$(_AndroidApiLevel)" |
dellis1972
approved these changes
Apr 22, 2020
In our current .NET 5 builds, you always hit:
warning XA1008: The TargetFrameworkVersion (Android API level 21) is lower than the targetSdkVersion (29).
Please increase the `$(TargetFrameworkVersion)` or decrease the `android:targetSdkVersion` in the `AndroidManifest.xml` so that the API levels match.
It is giving a warning for Google Play, because
`$(TargetFrameworkVersion)` is equal to 5.0.
To fix this, I modified the `<CheckGoogleSdkRequirements/>` MSBuild
task so that `TargetFrameworkVersion` is optional. For .NET 5 builds,
we can pass in `$(_AndroidApiLevel)` instead.
This way we will still get some important warnings for .NET 5 such as
`targetSdkVersion` or `minSdkVersion` that need to be changed in the
`AndroidManifest.xml` file.
I also fixed up `LastBuildOutput`, we needed the value for .NET 5
MSBuild tests and I removed unnecessary usage of `yield return`.
jonathanpeppers
force-pushed
the
net5-googlesdk-req
branch
from
dfdbe08 to
39e3154
Compare
jonpryor
pushed a commit
that referenced
this pull request
Apr 23, 2020
Context: https://github.com/dotnet/designs/blob/master/accepted/2020/net5/net5.md Historically, Xamarin.Android has used `$(TargetFrameworkVersion)` to control the `Mono.Android.dll` binding which is used in apps, and was treated as the Android version number, e.g. `$(TargetFrameworkVersion)`=v5.0 caused Xamarin.Android pull in the `Mono.Android.dll` with bindings for *Android* 5.0 (API-21). With .NET 5, `$(TargetFrameworkVersion)` *instead* is the version of .NET, i.e. .NET 5 will have `$(TargetFrameworkVersion)`=v5.0. The (new?) `$(TargetPlatformVersion)` MSBuild property will now control which `Mono.Android.dll` binding version should be used. Unfortunately, not everything within the Xamarin.Android build system is cognizant of this change. Consequently, when building within a .NET 5 environment, *all* builds will emit this XA1008 warning: warning XA1008: The TargetFrameworkVersion (Android API level 21) is lower than the targetSdkVersion (29). Please increase the `$(TargetFrameworkVersion)` or decrease the `android:targetSdkVersion` in the `AndroidManifest.xml` so that the API levels match. The XA1008 warning is emitted because Google Play requires a minimum version of API-29 (Android v10.0), and `$(TargetFrameworkVersion)` was previously used to infer a minimum API level, and Android v5.0/API-21 is less than the required API-29. To fix this, I modified the `<CheckGoogleSdkRequirements/>` MSBuild task so that: 1. `CheckGoogleSdkRequirements.TargetFrameworkVersion` is optional. 2. A new `CheckGoogleSdkRequirements.ApiLevel` property is available, and allows explicitly providing the API level. In .NET 5 builds, `CheckGoogleSdkRequirements.TargetFrameworkVersion` won't be set; `CheckGoogleSdkRequirements.ApiLevel` will instead be set based on `$(_AndroidApiLevel)`. This way we will still get some important warnings for .NET 5 such as `targetSdkVersion` or `minSdkVersion` that need to be changed in the `AndroidManifest.xml` file. I also fixed up `LastBuildOutput`, we needed the value for .NET 5 MSBuild tests and I removed unnecessary usage of `yield return`.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In our current .NET 5 builds, you always hit:
It is giving a warning for Google Play, because
$(TargetFrameworkVersion)is equal to 5.0.To fix this, I modified the
<CheckGoogleSdkRequirements/>MSBuildtask so that
TargetFrameworkVersionis optional. For .NET 5 builds,we can pass in
$(_AndroidApiLevel)instead.This way we will still get some important warnings for .NET 5 such as
targetSdkVersionorminSdkVersionthat need to be changed in theAndroidManifest.xmlfile.I also fixed up
LastBuildOutput, we needed the value for .NET 5MSBuild tests and I removed unnecessary usage of
yield return.