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

Remove uses-sdk from AndroidManifest.xml #664

Merged
merged 4 commits into from
Feb 12, 2019

Conversation

brody4hire
Copy link

@brody4hire brody4hire commented Feb 10, 2019

since uses-sdk values are now superseded by Gradle files

see #629

P.S. Changes should be merged as a squash merge.

P.P.S. Here is the context:

since uses-sdk values are now superseded by Gradle files

resolves apache#629
@oliversalzburg
Copy link

LGTM

@janpio
Copy link
Member

janpio commented Feb 12, 2019

This PR removes the handling of the values/attributes that should not be used any more. Do we also have the part where the replacement is set in build.gradle somewhere? I am missing this context here.

@brody4hire
Copy link
Author

I just added the context to the description, hope it is clear enough now.

@janpio
Copy link
Member

janpio commented Feb 12, 2019

So merging this will break the code without #655 being merged as well?

@brody4hire
Copy link
Author

Merging this should not break anything that is not already broken. There exists issue #508 since Gradle does not use the information from AndroidManifest.xml. So my idea is to fix it in multiple steps:

Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@brody4hire brody4hire merged commit bb45f4f into apache:master Feb 12, 2019
@brody4hire brody4hire deleted the remove-uses-sdk branch February 12, 2019 16:22
@brody4hire
Copy link
Author

Thanks @janpio for the quick review!

@brody4hire
Copy link
Author

Something went wrong. I discovered that a result of this change is that the APK is generated with a nonsense target SDK value. This is because while the existing Gradle scripts do supersede the minSdkValue from AndroidManifest.xml, it do not supersede the targetSdkValue from AndroidManifest.xml. I will raise a new issue to describe the problem and discuss alternative solutions really soon.

My apologies.

brody4hire pushed a commit to brody4hire/cordova-android that referenced this pull request Feb 13, 2019
brody4hire pushed a commit that referenced this pull request Feb 13, 2019
Revert "Remove uses-sdk from AndroidManifest.xml (#664)"
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 this pull request may close these issues.

3 participants