-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Resolve GH-539 & GH-540 on master #542
Conversation
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. |
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? |
Closing for now, will raise the question on the email forum. |
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? |
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 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 |
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. |
86f422f
to
771cb30
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the proper fix should just check if targetDir includes app, as the old path didn't, it will cover both cases app/src/main and app/lib, or any other case we might have missed.
But also, if it ends in .jar or .aar (maybe some other?) we should put it in app/target-dir instead of putting it in app/src/main/target-dir.
And the PR should be against master as it's also broken there, and then cherry-pick for the hotfix
for Java source only (apacheGH-539) Co-Authored-By: Christopher J. Brody <chris.brody@gmail.com> Co-Authored-By: Kyle Kirbatski <kkirbatski@gmrmarketing.com>
Fallback to old path mapping if no Android Studio path mapping exists Co-authored-by: Christopher J. Brody <chris.brody@gmail.com> Co-authored-by: Kyle Kirbatski <kkirbatski@gmrmarketing.com>
for JAR and AAR (apacheGH-540) Co-Authored-By: Kyle Kirbatski <kkirbatski@gmrmarketing.com> Co-Authored-By: Christopher J. Brody <chris.brody@gmail.com> Co-Authored-By: @afdev82 (Antonio Facciolo)
for Java source, JAR, and AAR Co-authored-by: Christopher J. Brody <chris.brody@gmail.com> Co-authored-by: Kyle Kirbatski <kkirbatski@gmrmarketing.com> Co-authored-by: Antonio Facciolo <afdev82@users.noreply.github.com>
771cb30
to
ef493b4
Compare
Updated as follows:
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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. |
Can you think of any examples of plugins where we need to do this?
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. |
Any plugin doing something like this: <source-file src="src/android/TestLib.jar" target-dir="libs" /> Example plugins facing this case: |
Thanks @dpogue. The plugins you listed seem to use 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. |
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 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. |
I just remembered a TODO item is that the pluginHandler tests need to be renumbered, leaving this for the next PR. |
Didn't know that, thanks. |
for Java source only (apacheGH-539) Co-Authored-By: Christopher J. Brody <chris.brody@gmail.com> Co-Authored-By: Kyle Kirbatski <kkirbatski@gmrmarketing.com>
Fallback to old path mapping if no Android Studio path mapping exists (with slight difference on 7.1.x branch) Co-authored-by: Christopher J. Brody <chris.brody@gmail.com> Co-authored-by: Kyle Kirbatski <kkirbatski@gmrmarketing.com>
for JAR and AAR (apacheGH-540) Co-Authored-By: Kyle Kirbatski <kkirbatski@gmrmarketing.com> Co-Authored-By: Christopher J. Brody <chris.brody@gmail.com> Co-Authored-By: @afdev82 (Antonio Facciolo)
for Java source, JAR, and AAR Co-authored-by: Christopher J. Brody <chris.brody@gmail.com> Co-authored-by: Kyle Kirbatski <kkirbatski@gmrmarketing.com> Co-authored-by: Antonio Facciolo <afdev82@users.noreply.github.com>
(new app target-dir scheme)
for Java source only (apacheGH-539) Co-Authored-By: Christopher J. Brody <chris.brody@gmail.com> Co-Authored-By: Kyle Kirbatski <kkirbatski@gmrmarketing.com>
Fallback to old path mapping if no Android Studio path mapping exists (with slight difference on 7.1.x branch) Co-authored-by: Christopher J. Brody <chris.brody@gmail.com> Co-authored-by: Kyle Kirbatski <kkirbatski@gmrmarketing.com>
for JAR and AAR (apacheGH-540) Co-Authored-By: Kyle Kirbatski <kkirbatski@gmrmarketing.com> Co-Authored-By: Christopher J. Brody <chris.brody@gmail.com> Co-Authored-By: @afdev82 (Antonio Facciolo)
for Java source, JAR, and AAR Co-authored-by: Christopher J. Brody <chris.brody@gmail.com> Co-authored-by: Kyle Kirbatski <kkirbatski@gmrmarketing.com> Co-authored-by: Antonio Facciolo <afdev82@users.noreply.github.com>
(new app target-dir scheme)
Changes now proposed on master branch:
commits with hotfix &unit test updates proposed in Fix dest overwrite #539 (PR Fix dest overwrite #539)revert 8a69e32 - CB-14125:(android) Increase old plugin compatibilitytarget-dir="app/lib"any target-dir with app pathExplanation
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 optionsfor the master branch, for the next major release:Include all changes except for revert of CB-14125Include all changes in the master branchI 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.