-
-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactored targetSdkVersionFromManifest #187
Conversation
@@ -89,18 +89,6 @@ manifestMethods.packageAndLaunchActivityFromManifest = async function (localApk) | |||
} | |||
}; | |||
|
|||
manifestMethods.targetSdkVersionFromManifest = async function (localApk) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mykola-mokhnach, in legacy developed applications these properties are not respected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) {
@sravanmedarapu how old does a target application have to be for this to fail? |
@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. |
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. |
@@ -98,7 +98,8 @@ manifestMethods.targetSdkVersionFromManifest = async function (localApk) { | |||
let targetSdkVersion = new RegExp(/targetSdkVersion:'([^']+)'/g).exec(stdout); | |||
return parseInt(targetSdkVersion[1], 10); | |||
} catch (e) { | |||
log.errorAndThrow(`targetSdkVersionFromManifest failed. Original error: ${e.message}`); | |||
log.warn(`Ran into problem getting target SDK; ignoring: ${e.message}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, Sure.
ae5cb72
to
478d97e
Compare
published in 2.9.1 |
PR #186 not working with some applications.
Issue: #appium/appium#7349, #appium/appium#7353
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 thetargetSdkVersion
to grant permissions.cc: @jlipps, @imurchie