search

apache / cordova-android

Apache Cordova Android
https://cordova.apache.org/
Apache License 2.0
3.59k stars 1.52k forks source link

Resolve GH-539 & GH-540 on master #542

Closed brodybits closed 5 years ago

brodybits commented 5 years ago

Changes now proposed on master branch:

Explanation

Issues reported in #539 & #540, as reproduced in #541, were introduced in 7.1.2 due to the following changes:

The major change in CB-13830 was to factor some Android Studio path remapping code into a studioPathRemap function, but it had a small issue that is resolved in PR #539.

The change in CB-14125 caused the issue reported in GH-540 and should not have been present in a patch release.

TODO

Cherry-pick needed on 7.1.x branch

Corresponding tests are needed for uninstall case.

One of the following options for the master branch, for the next major release:

  1. Include all changes except for revert of CB-14125
  2. Include all changes in the master branch

I would favor option 2, which reverts CB-14125 in the next major release as well. Old plugins should be updated to reflect the new directory structure.

dpogue commented 5 years ago

The change in CB-14125 was one of the main reasons for doing a patch release. There are hundreds of plugins that will never be updated, and we have lots of people stuck on the unsupported Cordova Android 6.x because of that.

brodybits commented 5 years ago

The change in CB-14125 was one of the main reasons for doing a patch release.

Understood, but it did break another plugin as reported in #540 which I think is unacceptable in a patch release and should not be done in a minor release. Any ideas how we can resolve this?

brodybits commented 5 years ago

Closing for now, will raise the question on the email forum.

dpogue commented 5 years ago

It kinda seems like this new issue is specific to .jar files, so we could maybe special-case them the same way we're handling the .java files?

jcesarmobile commented 5 years ago

This is not a breaking change as .jar/.aar files should be in lib-file tags, not in source-file tag. That's what we broke in cordova-android 7.0.0, all plugins that broke was because of usage of source-file tags instead of other proper tags (resource-file for resources, lib-file for libraries, etc)

They "fixed" it for 7.x.x by updating their source-file path from lib to app/lib in the plugin, so it's not handled properly by the released changes at it doesn't expect them to have app on the path.

But not 100% sure our fix is ok for old plugins when using source-file tags for libs, we should check if the new lib path is app/src/main/lib or app/lib, if it's the second then we have to add an else to handle libs as @dpogue suggested as current code will put them in app/src/main/+the target-dir

brodybits commented 5 years ago

Reopening as a WIP PR for now. I will take another look over the weekend.

I think PR #539 should be merged by itself, keeping the commits here for now since they are needed to help resolve GH-540.

A side point is that I did not see any test updates for CB-14125 (unit tests pass whether I keep or revert CB-14125 in my local workarea). Please correct me if I am mistaken. I think tests are really needed to help ensure it does not break in the future.

brodybits commented 5 years ago

I just updated with a simple solution: do not do the path remapping if target-dir="app/lib" in source-file element. CB-14125 is no longer reverted in this proposal.

Since use of target-dir="app/lib" indicates that the plugin is using the new file structure, I think there should be no reason to mess with it.

I think this should not break anything. If I am mistaken a specific case would be really helpful.

An even simpler solution may be to not do the path remapping if the target-dir starts with "app".

Test case is still needed for uninstall case. I can work on it over the weekend.

brodybits commented 5 years ago

Updated as follows:

codecov-io commented 5 years ago

Codecov Report

Merging #542 into master will increase coverage by 0.1%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #542     +/-   ##
=========================================
+ Coverage   61.87%   61.98%   +0.1%     
=========================================
  Files          17       17             
  Lines        1975     1978      +3     
  Branches      367      369      +2     
=========================================
+ Hits         1222     1226      +4     
+ Misses        753      752      -1
Impacted Files Coverage Δ
bin/templates/cordova/lib/pluginHandlers.js 86.58% <100%> (+0.87%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7da5374...ef493b4. Read the comment docs.

jcesarmobile commented 5 years ago

Looks good, but we still need to handle it ends in .jar or .aar to remap to app/target-dir instead of app/src/main/target-dir. Or edit gradle to look for libraries there too. But it can be done in a separate PR

brodybits commented 5 years ago

we still need to handle it ends in .jar or .aar to remap to app/target-dir instead of app/src/main/target-dir

Can you think of any examples of plugins where we need to do this?

Or edit gradle to look for libraries there too.

I would vote against this option. I suspect it would be easier for this to go wrong and possibly harder to fix in case it does. I also suspect it would be harder to get this option right in a patch release. But I may be mistaken, still not a Gradle expert.

brodybits commented 5 years ago

But it can be done in a separate PR

I would favor that.

I am now waiting for final approval to merge #539 before merging this one. Quick review of #544 would also be appreciated.

Again I would really appreciate examples of plugins where we need to fix mapping of target-dir for JAR & AAR.

My time will be pretty limited for a couple days due to other commitments, should have some more time afterwards.

dpogue commented 5 years ago

Again I would really appreciate examples of plugins where we need to fix mapping of target-dir for JAR & AAR.

Any plugin doing something like this:

<source-file src="src/android/TestLib.jar" target-dir="libs" />

Example plugins facing this case:

brodybits commented 5 years ago

Thanks @dpogue. The plugins you listed seem to use .so files, for which I recall that the installation location is not the same between cordova-android@6 and cordova-android@7.

I think that any plugins that did work on 7.1.1 but not 7.1.2 should be added to #540, along with reproduction steps. I think any plugins that have never worked on cordova-android@7 (and not fixed by 7.1.2 update) should be listed in a new issue, along with reproduction steps.

jcesarmobile commented 5 years ago

All the plugin examples linked by @dpogue have .jar source-files except for the last one.

They also happen to have .so files, not sure how we should handle them, I think proper folder is app/src/main/jniLibs, but most of those plugins were just putting it in libs folder, so should be app/libs.

I think we should fix .jar and .aar for now in a separate PR and investigate the .so

brodybits commented 5 years ago

I think we should fix .jar and .aar for now in a separate PR and investigate the .so

I just raised #547 for that.

Merging now.

brodybits commented 5 years ago

I just remembered a TODO item is that the pluginHandler tests need to be renumbered, leaving this for the next PR.

afdev82 commented 5 years ago

This is not a breaking change as .jar/.aar files should be in lib-file tags, not in source-file tag. That's what we broke in cordova-android 7.0.0, all plugins that broke was because of usage of source-file tags instead of other proper tags (resource-file for resources, lib-file for libraries, etc)

Didn't know that, thanks. I can update the plugin to use the right tag in the next release.