refactored targetSdkVersionFromManifest

[sravanmedarapu]

PR #186 not working with some applications.
Issue: #appium/appium#7349, #appium/appium#7353

• Granting permissions at run time works only when both below 2 cases matched

1. Device API should be 23 and above (From device point of view)
2. Application's targetSdkVersion version should be 23 and above (From Application point of view)

But there is no direct way to fetch targetSdkVersion from application's APK. Here we are relying on dumping apk info using (aapt dump badging APK). But unfortunately in legacy applications we do not find targetSdkVersion info.

Till we find the better alternatives to figure out targetSdkVersion, removing the targetSdkVersion to grant permissions.

cc: @jlipps, @imurchie

[mykola-mokhnach]

I'd rather keep it. This method should be simply fixed by checking the the android:minSdkVersion value (it is mandatory to set) in case targetSdkVersion is omitted in the manifest file. Check https://developer.android.com/guide/topics/manifest/uses-sdk-element.html for more details.

[sravanmedarapu]

@mykola-mokhnach, in legacy developed applications these properties are not respected.

[jlipps]

another option would be to attempt to retrieve the target sdk version, but simply ignore the error in the calling function. something like this:

let targetSdk = null;
try {
  targetSdk = await this.targetSdkVersionFromManifest(apk);
} catch (e) {
  logger.warn(`Ran into problem getting target SDK; ignoring: ${e.message}`);
}
if (apiLevel >= 23 && (!targetSdk || targetSdk >= 23)) {

[jlipps]

@sravanmedarapu how old does a target application have to be for this to fail?

[sravanmedarapu]

@jlipps, typically it is about best practices. All manifest would typically have either of the properties(targetSdkVersion, minSdkVersion). But there are cases where these properties neglected.

In Google docs mentioned that, it is certain to declare these properties and generally all applications does have these.

[sravanmedarapu]

What we could do in the absence of either of these properties, we can continue to grant the permissions. But need sometime to test this different apks and API combinations. As module broken in current state, I think we can continue with condition skip as a hot fix.

[jlipps]

This isn't exactly what I had in mind.

What you've done here is to make targetSdkVersionFromManifest emit a warning instead of throwing an error.

But I think this function should throw an error if something fails.

It's in the calling function below that I think we should explicitly ignore the error. In other words, targetSdkVersionFromManifest is a library function, and as such it should indeed throw any errors it encounters. But based on how we are using it in install here, we may or may not care about the error. That in my mind is a more flexible structure.

What do you think?

[sravanmedarapu]

Makes sense, Sure.

[jlipps]

published in 2.9.1