Skip to content
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

Inconsistent handling of min/target SDK values #508

Closed
dpogue opened this issue Oct 2, 2018 · 7 comments
Closed

Inconsistent handling of min/target SDK values #508

dpogue opened this issue Oct 2, 2018 · 7 comments

Comments

@dpogue
Copy link
Member

dpogue commented Oct 2, 2018

cordova-android currently has a few different ways to specify a minimum SDK version and the target SDK version, and they are not equally exposed at various levels of tooling, and they can override each other in non-obvious ways (to non-native Android devs).

Traditionally, we've relied on the <uses-sdk> element in AndroidManifest.xml to specify the minimum, target, and (optionally) the maximum SDK versions. These can be customized via <preference> elements in an app's config.xml (android-minSdkVersion, android-targetSdkVersion, and android-maxSdkVersion respectively).

We also allow specifying these values via Gradle variables (cdvMinSdkVersion, and cdvCompileSdkVersion). If these variables are not specified, we default them.

According to Google's documentation:

If you specify API level requirements directly in your app's manifest file, the corresponding settings in the build files will override the settings in the manifest file. [...] [T]o avoid potential overwriting when the manifest is merged, you should remove these attributes from the <uses-sdk> element and define your API level settings in the Gradle build files instead.

In particular, I'm concerned about the possibility of someone specifying a minimum SDK version in config.xml, and that getting overridden by the default minimum SDK version of 19 via gradle.

Google seems to recommend using Gradle properties, which was also brought up in my PR about updating the Gradle build files.
It seems to me like the correct thing to do is to take the SDK versions specified in config.xml and transform them into both <uses-sdk> attributes (as is done currently), but also into gradle properties. We don't currently have a good way to do that, but @erisu's PR #465 provides a gradle.properties file that we could inject cdvMinSdkVersion and cdvCompileSdkVersion into.

Thoughts/Concerns/Feedback?

@brodybits
Copy link
Contributor

I think the most straightforward solution would be the following:

I would love to see this cleaned up before we finish the Cordova 9 release (apache/cordova#10).

It would be nice if we could come up with a more elegant solution. I personally have no time to look into this, doubt that any other members would have the extra time either.

@dpogue
Copy link
Member Author

dpogue commented Jan 18, 2019

How do people configure it via gradle if they're treating the platform as a build artifact?

We should be setting the values in gradle based on what's specified in config.xml (and we actually have a way to do that now via GradlePropertiesParser if we so desire)

@brodybits
Copy link
Contributor

How do people configure it via gradle if they're treating the platform as a build artifact?

Use --gradleArg in cordova build / cordova run command, as documented in:

We should be setting the values in gradle based on what's specified in config.xml (and we actually have a way to do that now via GradlePropertiesParser if we so desire)

That would be really nice. Some pointers how to do this would be great. I personally have no extra time to work on this one.

@erisu
Copy link
Member

erisu commented Feb 8, 2019

@brodybits

We should be setting the values in gradle based on what's specified in config.xml (and we actually have a way to do that now via GradlePropertiesParser if we so desire)

That would be really nice. Some pointers how to do this would be great. I personally have no extra time to work on this one.

The defaults are added around this line. We could add our defaults here for the min and target SDK.

Currently, users who want to modify the gradle.properties file would have to modify it directly but this is not the best case. Users may not commit their platform directory. This mean changes are lost each time they remove and add back a platform..

This file is created and modified by the cordova prepare step. The first time the file is created would come right after the cordova platform add step which triggers the prepare step.

If the user wanted to modify this file and they didn't commit the platforms, the current best option is to use an after_prepare hook script.

We could do something like this: master...erisu:user-custom-gradle-properties-defines

This would use one of our existing methods for setting the min and target SDK. The above is a working example of updating the Gradle Properties files, but I suspect there would be a lot more refactoring involved.

Hope this helps...

@dpogue
Copy link
Member Author

dpogue commented Feb 8, 2019

I had started doing something along these lines, but never got to the point of hooking it up in the prepare step. I added methods to the GradlePropertiesParser to allow setting variables, which you could pull from this commit: dpogue@d18f8d1

@brodybits
Copy link
Contributor

I would now favor limiting the scope of our changes to read android-minSdkVersion from config.xml, and direct users to configure the other items in Gradle attributes (as already documented in https://cordova.apache.org/docs/en/latest/guide/platforms/android/index.html#setting-gradle-properties).

I think the most common issue has been with apps that need to increase the minimum SDK value. We should set target/compile SDK to recent value by default in cordova-android itself.

I have encountered enough issues with compile / target SDK values in Gradle. I think there is need to compound these troubles by attempting to read from config.xml as well.

@erisu
Copy link
Member

erisu commented Apr 6, 2019

Closing as PR #699 should have resolve this and is merged into master. Should be available with next release.

@erisu erisu closed this as completed Apr 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants