search

dotnet / android

.NET for Android provides open-source bindings of the Android SDK for use with .NET managed languages such as C#
MIT License
1.93k stars 531 forks source link

Provide warnings and errors around compileSdkVersion(TargetFrameworkVersion), targetSdkVersion, and minSdkVersion #1768

Closed JonDouglas closed 5 years ago

JonDouglas commented 6 years ago

Google has the following guidance for these three values:

minSdkVersion (lowest possible) <= targetSdkVersion == compileSdkVersion (latest SDK)
  1. We should throw a warning if the targetSdkVersion is lower than compileSdkVersion(TargetFrameworkVersion).

targetSdkVersion < compileSdkVersion(TargetFrameworkVersion)

Sample warning message text:

Your application is running on a version of Android that is more recent than your targetSdkVersion specifies. Set your targetSdkVersion to the highest version of Android available to match your TargetFrameworkVersion.

EX:

Your application is running on a version of Android (26) that is more recent than your targetSdkVersion specifies (24). Set your targetSdkVersion to the highest version of Android available to match your TargetFrameworkVersion (26).

Currently there is a warning in this area, but it doesn't let the developer know exactly what "ACW compilation is" or how they can fix the issue:

warning XA4211: AndroidManifest.xml //uses-sdk/@android:targetSdkVersion '19' is less than $(TargetFrameworkVersion) ''. Using API-26 for ACW compilation.

  1. We should throw an error if the compileSdkVersion(TargetFrameworkVersion) is lower than targetSdkVersion.

compileSdkVersion(TargetFrameworkVersion) < targetSdkVersion

Sample error message text:

The TargetFrameworkVersion should not be lower than targetSdkVersion.

EX:

The TargetFrameworkVersion (26) should not be lower than targetSdkVersion (27)

  1. We should throw an error if the minSdkVersion is greater than targetSdkVersion.

minSdkVersion > targetSdkVersion

Sample error message text:

The minSdkVersion is greater than targetSdkVersion. Please change the value such that minSdkVersion is less than or equal to targetSdkVersion.

EX:

The minSdkVersion (26) is greater than targetSdkVersion. Please change the value such that minSdkVersion is less than or equal to targetSdkVersion (25).

mattleibow commented 6 years ago

Another bug is that the XA4211 warning has a blank '' where the version should be - I think something is missing or not being resolved properly.

dellis1972 commented 6 years ago

PR is in progress https://github.com/xamarin/xamarin-android/pull/2050

jonpryor commented 6 years ago

I think we need to rethink the messages, because as I read them today, I'm finding them confusing.

Aside:

Google has the following guidance for these three values:

minSdkVersion (lowest possible) <= targetSdkVersion == compileSdkVersion (latest SDK)

Where specifically is this guidance? I haven't been able to find it.

Consider this proposed message:

Your application is running on a version of Android (26) that is more recent than your targetSdkVersion specifies (24). Set your targetSdkVersion to the highest version of Android available to match your TargetFrameworkVersion (26).

For starters, the "tense" of this message is entirely wrong. This is a build-time message; as such, saying that the "application is running on a version of Android" is off-putting and wrong; it's not running on anything at the time this message is produced.

I would suggest "building against" instead of "running on":

Your application is building against a version of Android (26) that is more recent than your targetSdkVersion specifies (24). Set your targetSdkVersion to the highest version of Android available to match your TargetFrameworkVersion (26).

The TargetFrameworkVersion (26) should not be lower than targetSdkVersion (27)

Should this be a warning or an error?

If this is to be an error, we should use "must not" or "shall not", not "should not". If this should be a warning, then "should not" is appropriate. See also: https://www.ietf.org/rfc/rfc2119.txt

JonDouglas commented 6 years ago

@jonpryor Google best practices can be found here, it's also sprinkled within the documentation and Google I/O talks:

https://medium.com/google-developers/picking-your-compilesdkversion-minsdkversion-targetsdkversion-a098a0341ebd

We can alter the wording however we see fit. By no means are these the final copy and rather sample text.

Regarding whether the following should be a warning or an error:

The TargetFrameworkVersion should not be lower than targetSdkVersion.

It should throw an error to be on par with Gradle. Here's an example:

TargetFrameworkVersion - 27 targetSdkVersion - 28

Given the highest level of support libraries we can use in this case is v27, we still enable Android Pie features at runtime with targetSdkVersion being 28. This can cause incompatibility between the support library as it's expecting the same compatibility version at runtime (e.g. v27).

Thus the following should hold true in this case:

minSdkVersion (lowest possible) <= targetSdkVersion == compileSdkVersion (latest SDK)

Currently, a Xamarin.Android user would be able to get into one of these incompatible scenarios and find out at build, install, or runtime at the latest about the incompatibility(Possibly even publishing their app not knowing that runtime features aren't working). These warnings and errors would help ensure that they are following best practices at build time similar to Gradle.

jonpryor commented 6 years ago

@JonDouglas: I did find that Medium article, but didn't link to it as it doesn't only say what you said it does. You said:

minSdkVersion (lowest possible) <= targetSdkVersion == compileSdkVersion (latest SDK)

i.e. targetSdkVersion must match compileSdkVersion, and we should report an error if they're different.

The medium article:

minSdkVersion <= targetSdkVersion <= compileSdkVersion

Ideally, the relationship would look more like this in the steady state:

minSdkVersion (lowest possible) <= targetSdkVersion == compileSdkVersion (latest SDK)

i.e. targetSdkVersion may be less than compileSdkVersion, but isn't necessarily ideal.

These statements are not the same.

It should throw an error to be on par with Gradle. Here's an example:

What error message does Gradle generate?

Does Gradle generate an error? I do not get one with a random project I have in Android Studio:

android {
    compileSdkVersion 27
    defaultConfig {
        applicationId "com.example.myapplication"
        minSdkVersion 15
        targetSdkVersion 25
        versionCode 1
        versionName "1.0"
        testInstrumentationRunner "android.support.test.runner.AndroidJUnitRunner"
    }
}

I suspect that requiring that targetSdkVersion == $(TargetFrameworkVersion) will Break The World™, in that everything will quite suddenly break.

The default Xamarin.Android app template in Visual Studio (Windows) produces an Properties\AndroidManifest.xml file which contains a <uses-sdk/> element which contains //uses-sdk/@android:targetSdkVersion, set to the $(TargetFrameworkVersion) used at project creation time.

If you change the $(TargetFrameworkVersion) value, AndroidManifest.xml is not updated to be consistent. Consequently, merely changing $(TargetFrameworkVersion) may cause the app to stop building.

The default template on Visual Studio for Mac contains an empty <uses-sdk/> element. This is at least an improvement, as it means we'll default to using a "targetSdkVersion" value which matches $(TargetFrameworkVersion), but that value will not be stored within AndroidManifest.xml.

dellis1972 commented 6 years ago

The templates really should be only providing the minSDK right? But I guess is depends on the template. There are 3 in VSForMac.

JonDouglas commented 6 years ago

@jonpryor

Although the ideal guidelines are minSdkVersion (lowest possible) <= targetSdkVersion == compileSdkVersion (latest SDK), gradle/Android Studio treats it very much like the older guideline of minSdkVersion <= targetSdkVersion <= compileSdkVersion.

Regarding the three cases above:

  1. targetSdkVersion < compileSdkVersion(TargetFrameworkVersion) -> Throws a warning
  2. compileSdkVersion(TargetFrameworkVersion) < targetSdkVersion -> Throws an error
  3. minSdkVersion > targetSdkVersion -> Throws a warning (That's actually a gradle sync error)

Here are the cases in gradle/Android Studio:

  1. Not targeting the latest versions of Android; compatibility modes apply. Consider testing and updating this version. Consult the android.os.Build.VERSION_CODES javadoc for details. less... (Ctrl+F1) 
Inspection info:When your application runs on a version of Android that is more recent than your targetSdkVersion specifies that it has been tested with, various compatibility modes kick in. This ensures that your application continues to work, but it may look out of place. For example, if the targetSdkVersion is less than 14, your app may get an option button in the UI.  To fix this issue, set the targetSdkVersion to the highest available value.
  2. The compileSdkVersion (25) should not be lower than the targetSdkVersion (26)
  3. WARNING: minSdkVersion (28) is greater than targetSdkVersion (26) for variant "release". Please change the values such that minSdkVersion is less than or equal to targetSdkVersion.
Affected Modules: app

I suspect that requiring that targetSdkVersion == $(TargetFrameworkVersion) will Break The World™, in that everything will quite suddenly break.

Yeah, we definitely do not want to have this type of constraint. Rather a soft warning for the <= case here.

If you change the $(TargetFrameworkVersion) value, AndroidManifest.xml is not updated to be consistent. Consequently, merely changing $(TargetFrameworkVersion) may cause the app to stop building.

I believe Visual Studio team changed this behavior in 15.8. Visual Studio for Mac still needs to implement the consolidation of $(TargetFrameworkVersion) / targetSdkVersion values.

@dellis1972 The templates only depend on minSdkVersion. At creation time, it will default the targetSdkVersion and $(TargetFrameworkVersion) to the latest stable version of Android supported in Xamarin.Android.

dellis1972 commented 6 years ago

I updated the PR to reflect @JonDouglas's latest information. In other words it should now do that Android Studio does.

jonpryor commented 6 years ago

@JonDouglas wrote:

compileSdkVersion(TargetFrameworkVersion) < targetSdkVersion -> Throws an error

I wonder whether we can actually throw an error here, especially with your followup/"fine print":

If you change the $(TargetFrameworkVersion) value, AndroidManifest.xml is not updated to be consistent. Consequently, merely changing $(TargetFrameworkVersion) may cause the app to stop building.

I believe Visual Studio team changed this behavior in 15.8. Visual Studio for Mac still needs to implement the consolidation of $(TargetFrameworkVersion) / targetSdkVersion values.

Thus, if we have a developer who:

  1. Is on macOS, and
  2. Has Properties/AndroidManifest.xml with a //uses-sdk/@android:targetSdkVersion value which is greater than the $(TargetFrameworkVersion)

their build would now fail.

Can we determine if there are any customer projects for which this is the case?

I can say that there are three samples for which this is the case, and thus this change would cause at least some number of projects to generate an error, whereas no error is currently generated.

BoundServiceDemo

Note though that this sample sets //uses-sdk/@android:minSdkVersion=21. Is it really a bad thing that compileSdkVersion(TargetFrameworkVersion) is less than targetSdkVersion?

GraphicsAndAnimation

NfcSample

For giggles, I did try actually building this one. It builds, and has only 5 warnings:

warning XA0113: Google Play requires that new applications must use a TargetFrameworkVersion of v8.0 (API level 26) or above. You are currently targeting v2.3 (API level 10). 
warning XA0114: Google Play requires that application updates must use a TargetFrameworkVersion of v8.0 (API level 26) or above. You are currently targeting v2.3 (API level 10).
warning XA0101: @(Content) build action is not supported
warning XA5300: {0} path {1} is not valid; skipping. mscorlib.dll
warning ANDJS0000: No -tsa or -tsacert is provided and this jar is not timestamped. Without a timestamp, users may not be able to validate this jar after the signer certificate's expiration date (2047-07-20) or after any future revocation date.

Command used to find the above projects:

$ for f in `git grep -l targetSdkVersion`; do
    echo "## $f" ; grep targetSdkVersion $f ;
    git grep TargetFrameworkVersion `dirname "$f"`/../ ;
done
## ApplicationFundamentals/ServiceSamples/BoundServiceDemo/Properties/AndroidManifest.xml
    <uses-sdk android:minSdkVersion="21" android:targetSdkVersion="25" />
ApplicationFundamentals/ServiceSamples/BoundServiceDemo/BoundServiceDemo.csproj:    <TargetFrameworkVersion>v7.0</TargetFrameworkVersion>
...
JonDouglas commented 6 years ago

@jonpryor

Visual Studio for Mac recently added the similar behavior as Visual Studio 2017. I would say that because of this consolidation, we can move forward with the compileSdkVersion(TargetFrameworkVersion) < targetSdkVersion throwing a warning instead of an error as proposed initially. The rational for a warning instead of an error makes much more sense with all these finds until we can further justify it as an error.