search

apache / cordova-android

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

Fix for old plugins with non-Java sources (GH-547) #550

Closed brodybits closed 5 years ago

brodybits commented 5 years ago

Proposed changes

The proposed changes will be rebased along with PR #542 (fix for GH-540) on 7.1.x if accepted.

Testing

npm test

cordova-sqlite-storage 2.1.5

cordova-sqlite-storage@2.1.5 has old JAR and .so target-dir scheme, which does not work on cordova-android@7.0.0 (I already make a workaround in cordova-sqlite-storage@2.2.0). But cordova-sqlite-storage@2.1.5 works on https://github.com/brodybits/cordova-android#gh-547-bugfix with the proposed bug fix.

How tested in clone of https://github.com/brodybits/cordova-sqlite-test-app:

cordova-plugin-purchase v7.1.0

This cordova-plugin-purchase version has AIDL & XML files with old target-dir scheme, does not work on cordova-android@7.0.0. They had to do some ugly workarounds to work on both cordova-android@6 and cordova-android@7.

How tested in a new Cordova project:

cordova-plugin-hikvisionVedio

https://github.com/dylearning/cordova-plugin-hikvisionVedio fails to build on cordova-android@7.1.2 since its JAR files as specified in source-file elements not installed in the right place.

How tested in a new Cordova project:

Quirk

JAR library files are mapped into app/lib subdirectory, while .so (NDK) library files are mapped into app/src subdirectory. I wonder if there is an easy way to resolve this inconsistency.

I think we should encourage plugins to use lib-file instead of source-file for AAR, JAR, and .so files, and add a deprecation message in case mapping of these library files is needed. I hope to propose this change in the near future.

Next steps

jcesarmobile commented 5 years ago

Looking at the plugin you linked in the comment, it also has .h and .c files that are treated as .java files. They were src/c/common/mylib/parts and he updated them to app/src/main/java/c/common/mylib

We could add those two extensions in the .java check.

codecov-io commented 5 years ago

Codecov Report

Merging #550 into master will increase coverage by 0.11%. The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #550      +/-   ##
==========================================
+ Coverage   61.98%   62.09%   +0.11%     
==========================================
  Files          17       17              
  Lines        1978     1984       +6     
  Branches      369      371       +2     
==========================================
+ Hits         1226     1232       +6     
  Misses        752      752
Impacted Files Coverage Δ
bin/templates/cordova/lib/pluginHandlers.js 87.05% <92.3%> (+0.47%) :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 576ad18...61500ac. Read the comment docs.

brodybits commented 5 years ago

app/src/main is used multiple times, might be worth to put it in a variable

I can do this, would favor doing the same thing with some other prefixes such as app for the sake of consistency. Ultimate solution would be to do this for all destination prefixes.

[...] it also has .h and .c files that are treated as .java files. They were src/c/common/mylib/parts and he updated them to app/src/main/java/c/common/mylib

We could add those two extensions in the .java check.

Interesting idea, how should we test it? Anything beyond a unit test?

I will try to do the rework later tonight, would like to get it into the 7.1.3 patch if possible.

jcesarmobile commented 5 years ago

I don’t know any plugin with such files (other than the one you linked and it’s fixed already), so unit tests should be enough

brodybits commented 5 years ago

[...] it also has .h and .c files that are treated as .java files. They were src/c/common/mylib/parts and he updated them to app/src/main/java/c/common/mylib

We could add those two extensions in the .java check.

I would not favor making a special case for these files. Right now cordova-android knows nothing about these files by itself and I think we should keep it that way. If a plugin wants to put these files in a special location I think it should specify the full prefix starting with app. I will add them to the unit test, though.

jcesarmobile commented 5 years ago

It knows nothing about any file extension at all, this PR is the one placing files based on the extension

brodybits commented 5 years ago

I think the primary rule should be to place files based on target-dir prefix, i.e. app vs src vs lib, and extensions should be treated as special cases. This is basically how it is implemented in this PR.

I would rather not make a special case for .h and .c files since cordova-android does not need to do anything with them. Use of .h and .c files should remain plugin-specific, plugin provides its own Gradle file to handle them.

For example: https://github.com/dpa99c/cordova-plugin-hello-c specifies the native build with .h and .c files in the following places:

I would be happy to add these files to the unit test, for the sake of verification and understanding.

jcesarmobile commented 5 years ago

So, what's the point of adding them to the tests if we are not handling them? There is already a test for the files that don't fall into any of the elses, so there is no point of adding other one

brodybits commented 5 years ago

app/src/main is used multiple times, might be worth to put it in a variable

Done. CanI get a final approval to merge?

jcesarmobile commented 5 years ago

I still see the thanks comment&link, can you remove it?

brodybits commented 5 years ago

Just removed the comment, my bad.

brodybits commented 5 years ago

I just added these changes to PR #555 on 7.x for a patch release, hope we can make it soon!

janpio commented 5 years ago

How can you add a non-merged PR to a new PR? Did you just cherry pick the individual commits? If so, I would advise against that (which would act as a "do not merge" blocker on the other PR) as you do not really know if this PR will be merged that way - especially as this one here should probably be squashed.

brodybits commented 5 years ago

@janpio I was thinking to do a merge commit to keep the cleanup changes separate from the behavior change. I think it makes it easier to understand, and less to potentially revert just in case. If anyone has objections please do not hesitate.

In general I have been testing the changes on both master and 7.1.x to help ensure the patch goes smoothly. Proposal for patch updates in #555 is marked WIP to avoid a premature merge. In case this PR is not merged for some reason it would be really easy to remove the unwanted commits from #555.

Considering that this change has gone through review and is waiting for final approval, I would like to get it into the upcoming patch if possible. I think it would be a major benefit for a number of existing plugins. I hope we can merge and release soon. I put some client work on hold, at personal cost, in order to make the fixes and get some plugins working again.

brodybits commented 5 years ago

Thanks @jcesarmobile for the detailed review!

janpio commented 5 years ago

Was @jcesarmobile's feedback on how to merge or not merge this taken into account?

brodybits commented 5 years ago

Was @jcesarmobile's feedback on how to merge or not merge this taken into account?

I think the edit was too late. Not sure if I would have done it exactly that way, but probably would have been good to combine some of the commits.

I think the biggest downside to combining commits is in case there is a need to revert some but not all of the changes for some reason.

janpio commented 5 years ago

So, No.

I think the biggest downside to combining commits is in case there is a need to revert some but not all of the changes for some reason.

I prefer a clean and comprehensible commit history versus already planning having to revert stuff.