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

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

Merged
merged 7 commits into from
Nov 14, 2018

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Nov 13, 2018

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

  • npm run unit-tests passes
  • npm test passes on Travis CI
  • npm test passes on AppVeyor CI

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 add cordova-sqlite-storage@2.1.5
  • cordova platform add https://github.com/brodybits/cordova-android#gh-547-bugfix
  • Able to build and run on a test device
  • Click on self test button results in an alert that the self test is OK (self test of SQLite CRUD operations in sqlite plugin)
  • cordova plugin remove cordova-sqlite-storage is successful

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 add https://github.com/j3k0/cordova-plugin-purchase#v7.1.0 --variable BILLING_KEY="MIIB...AQAB"
  • cordova platform add https://github.com/brodybits/cordova-android#gh-547-bugfix
  • cordova build android is successful
  • cordova plugin remove cc.fovea.cordova.purchase is successful

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:

  • cordova plugin add https://github.com/dylearning/cordova-plugin-hikvisionVedio
  • cordova platform add https://github.com/brodybits/cordova-android#gh-547-bugfix
  • cordova build android is successful
  • cordova plugin rm cordova-plugin-hikvisionVedio is successful

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

Christopher J. Brody added 4 commits November 12, 2018 22:54
Possibly related to: CB-13830: Add handlers for plugins
that use non-Java source files, such as Camera
of plugin source file installed into app/src/main with
old target-dir scheme

NOTE: These tests do *not* check compatibility of
plugins with old lib target-dir scheme.
Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

This looks okay to me, but I can't vouch for every possible use case

return path.join('app/src/main/aidl', obj.targetDir.substring(4), path.basename(obj.src));
} else if (obj.targetDir.includes('libs')) {
if (obj.src.endsWith('.so')) {
// THANKS FOR GUIDANCE:
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer some official link about how to handle .so files than a thanks comment. If we don't have some official link, then just remove the comments as the code is pretty complex and this comments don't add value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not see an official link, will remove the comment. I would like to give credit somehow, maybe in a comment in this PR?

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Just a comment.

And also, app/src/main is used multiple times, might be worth to put it in a variable so if it changes again in the future we only have to change it in one place.

@jcesarmobile
Copy link
Member

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
Copy link

codecov-io commented Nov 13, 2018

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%) ⬆️

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.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 13, 2018

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
Copy link
Member

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

@brodycj brodycj mentioned this pull request Nov 14, 2018
7 tasks
@brodycj
Copy link
Contributor Author

brodycj commented Nov 14, 2018

[...] 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
Copy link
Member

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

@brodycj
Copy link
Contributor Author

brodycj commented Nov 14, 2018

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
Copy link
Member

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

@brodycj
Copy link
Contributor Author

brodycj commented Nov 14, 2018

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
Copy link
Member

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

(source-file entries)

including aidl, aar, jar, and so files
@brodycj
Copy link
Contributor Author

brodycj commented Nov 14, 2018

Just removed the comment, my bad.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 14, 2018

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

@janpio
Copy link
Member

janpio commented Nov 14, 2018

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.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 14, 2018

@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.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

LGTM

About merge commit or squash, considering there are 7 commit's how it that easier if you undo than a single commit?
Probably most of this commits could be manually squashed to two, one for fixing the issue and other for fixing/adding tests, or 3, fixing the issue, fixing the tests and adding new tests, but a few of them are meaningless (like Fix comments in getInstallDestination).
If you want to do a merge, manually squash a few of them first, otherwise, do a full squash, we should keep the history as clean as possible, one commit per issue when possible.

@brodycj brodycj merged commit 21ae48e into apache:master Nov 14, 2018
@brodycj
Copy link
Contributor Author

brodycj commented Nov 14, 2018

Thanks @jcesarmobile for the detailed review!

@janpio
Copy link
Member

janpio commented Nov 15, 2018

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

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

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
Copy link
Member

janpio commented Nov 15, 2018

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.

brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
Possibly related to: CB-13830: Add handlers for plugins
that use non-Java source files, such as Camera
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
of plugin source file installed into app/src/main with
old target-dir scheme

NOTE: These tests do *not* check compatibility of
plugins with old lib target-dir scheme.
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
(source-file entries)

including aidl, aar, jar, and so files
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
Possibly related to: CB-13830: Add handlers for plugins
that use non-Java source files, such as Camera
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
of plugin source file installed into app/src/main with
old target-dir scheme

NOTE: These tests do *not* check compatibility of
plugins with old lib target-dir scheme.
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
(source-file entries)

including aidl, aar, jar, and so files
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.

5 participants