Fix dest overwrite #539

kkirby · 2018-11-08T18:32:32Z

Platforms affected

Android

What does this PR do?

Makes sure that we don't overwrite the target-dir of a source-file if the project base is android studio and the target-dir is the proper install path and doesn't need to be re-mapped.

What testing has been done on this change?

I've added a new unit test, confirmed the problem exists, fixed the issue and re-ran the tests.

Checklist

Reported an issue in the JIRA database
Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
Added automated test coverage as appropriate for this change.

More details

Latest release (7.1.2) breaks our build. After investigation it appears that source-file.install handler was updated to extract the functionality of path remapping for android studio. However, if the path doesn't need to be remapped, the function returns undefined, thus updating dest to be undefined.

Related lines

7a97fd7...7.1.2#diff-4f73e5f70cec312ae82aa7d023e3c88bR320
7a97fd7...7.1.2#diff-4f73e5f70cec312ae82aa7d023e3c88bR50

Resolution

Just change line 50 to studioPathRemap(obj) || dest

brodycj · 2018-11-09T13:38:47Z

~~Can you confirm the following:~~

Does the test case show the functionality working on 7.1.1 but not 7.1.2? Answer is yes.
Is this an issue on the master branch? Answer is yes.

~~I think this should solve the issue in #540.~~

brodycj · 2018-11-09T16:59:23Z

Unfortunately this proposal does not resolve the issue reported in #540.

dpogue · 2018-11-09T18:45:42Z

Thanks! I'd like to get this merged in.

There's one other spot in uninstall where the same pattern appears, does that also need to be updated?

brodycj · 2018-11-09T18:52:08Z

There's one other spot in uninstall where the same pattern appears, does that also need to be updated?

I think so, would like to see unit test coverage to ensure it does not break again. Not sure if we need to do this in the hotfix.

This fix is definitely needed in master as well.

jcesarmobile · 2018-11-09T19:20:46Z

Supposedly, studioPathRemap should not remap if the path starts with app/src/main as that's how they should start. I think this PR breaks that remapping for old plugins that didn't update the target-dir paths yet.

EDIT: Ah, shit, just noticed studioPathRemap doesn't return the original path when it doesn't start with app/src/main

brodycj · 2018-11-09T19:28:43Z

I think this PR breaks that remapping for old plugins that didn't update the target-dir paths yet

The update is only for the case when studioPathRemap returns null. Without this change there is a case, reproduced in the proposed test update, where a working "target-dir" path would be overwritten with a null value.

I think it should not break a supported case that is already working. If I am mistaken a specific example would be really helpful.

jcesarmobile · 2018-11-09T22:05:58Z

Yeah, sorry, see my edit, the remap function should not return null, it should return the path unmodified, that’s why I thought it will break it as I only checked the changes, once I checked the code I noticed that was missing

kkirby · 2018-11-10T03:25:43Z

@jcesarmobile I didn't want to introduce too many changes to the codebase. Passing in the existing dest and returning it doesn't make sense to me. I think if anything we should check if the path needs to be modified and then call studioPathRemap. Though, like I said, I didn't want to introduce too many changes. I just wanted to put in a quick fix.

jcesarmobile

Looks good to me, but send the PR to master and we will cherry pick

for Java source only (GH-539) Co-Authored-By: Christopher J. Brody <[email protected]> Co-Authored-By: Kyle Kirbatski <[email protected]>

brodycj · 2018-11-11T19:39:51Z

I just pushed changes to add unit test and fix the issue in case of uninstall.

Unfortunately I am not able to rebase this fix onto master since the code in master seems to assume that it always has Android Studio mapping. I will investigate a possible solution.

for Java source only (apacheGH-539) Co-Authored-By: Christopher J. Brody <[email protected]> Co-Authored-By: Kyle Kirbatski <[email protected]>

Fallback to old path mapping if no Android Studio path mapping exists Co-authored-by: Christopher J. Brody <[email protected]> Co-authored-by: Kyle Kirbatski <[email protected]>

brodycj · 2018-11-11T20:41:25Z

bin/templates/cordova/lib/pluginHandlers.js

@@ -34,7 +34,7 @@ var handlers = {
            // a later plugins release.  This is for legacy plugins to work with Cordova.

            if (options && options.android_studio === true) {
-                dest = studioPathRemap(obj);
+                dest = studioPathRemap(obj) || dest;


It is not possible to cherry-pick or rebase this change to work on master. This change will be clobbered by another solution that works more easily on master.

Fallback to old path mapping if no Android Studio path mapping exists (with minor change needed to work on for 7.1.x branch) Co-authored-by: Christopher J. Brody <[email protected]> Co-authored-by: Kyle Kirbatski <[email protected]>

brodycj · 2018-11-11T20:58:01Z

I just pushed an update that would be much easier to cherry-pick between 7.1.x and master. Note that this update basically clobbers the change proposed by @kkirby in ed0991c.

At this point I can think of 2 ways forward:

we rebase this PR onto master and take it out of PR Resolve GH-539 & GH-540 on master #542
we keep this PR on 7.1.x, merge PR Resolve GH-539 & GH-540 on master #542 to fix on master, and cherry-pick other changes back to 7.1.x

@jcesarmobile please tell me which way you want to proceed with this.

I would like to express appreciation to @kkirby for pointing out the issue and contributing the unit test, even if we cannot use the originally proposed solution.

jcesarmobile · 2018-11-11T22:04:37Z

I prefer the other one as it’s going to be easier to maintain in case we need more hot fixes before we have a 8.0.0 ready

brodycj · 2018-11-11T22:11:52Z

What do you mean by "the other one"?

brodycj · 2018-11-11T22:50:09Z

I just rebased on master (actually pushed the first 3 changes from PR #542 to resolve the original issue on master). Fix should be straightforward to cherry-pick on 7.1.x.

@jcesarmobile please give final confirmation so that I can merge this one into master.

brodycj · 2018-11-11T22:59:01Z

P.S. If someone else does a squash merge, please keep the "Co-authored-by" comments from the commits to give due credit to both @kkirby and myself.

jcesarmobile · 2018-11-11T23:48:51Z

The “other” was #542, but as you keep changing both I don’t know anymore

brodycj · 2018-11-12T00:23:18Z

@jcesarmobile I was hoping to close in on an approved solution for this one and #540, sorry I did not really understand you before. I would slightly favor keeping this one separate from #540 since they are two different problems. A final review of the changes I pushed would be really helpful as well.

brodycj · 2018-11-12T17:21:49Z

Closing now in favor of PR #542. I just put this change back onto the 7.1.x branch (including the original commits by @kkirby).

I am closing this because the proposed change is only a partial solution for the issues we need to fix in #542, to resolve #540 and maybe some other issues with plugins using the old project structure.

@kkirby will absolutely get co-author credit for the added test and solution for the case when studioPathRemap returns null.

Resolve GH-539 & GH-540 on master

for Java source only (apacheGH-539) Co-Authored-By: Christopher J. Brody <[email protected]> Co-Authored-By: Kyle Kirbatski <[email protected]>

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 <[email protected]> Co-authored-by: Kyle Kirbatski <[email protected]>

for Java source only (apacheGH-539) Co-Authored-By: Christopher J. Brody <[email protected]> Co-Authored-By: Kyle Kirbatski <[email protected]>

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 <[email protected]> Co-authored-by: Kyle Kirbatski <[email protected]>

for Java source only (apacheGH-539) Co-Authored-By: Christopher J. Brody <[email protected]> Co-Authored-By: Kyle Kirbatski <[email protected]>

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 <[email protected]> Co-authored-by: Kyle Kirbatski <[email protected]>

brodycj · 2018-11-21T23:53:12Z

Now fixed in the latest patch release

kkirby added 2 commits November 8, 2018 12:23

Add a unit test to test source-file target-dir /app/src/main/…

ed95ad2

Make sure to we don’t overwrite dest if studioPathRemap returns null

ed0991c

kkirby changed the title ~~Hotfix/fix dest overwrite~~ Fix dest overwrite Nov 8, 2018

brodycj mentioned this pull request Nov 9, 2018

Issue to copy library from plugin after updating to v7.1.2 #540

Closed

brodycj mentioned this pull request Nov 9, 2018

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

Merged

jcesarmobile approved these changes Nov 10, 2018

View reviewed changes

unit test uninstall of <source-file> with app dest

006f684

for Java source only (GH-539) Co-Authored-By: Christopher J. Brody <[email protected]> Co-Authored-By: Kyle Kirbatski <[email protected]>

brodycj mentioned this pull request Nov 11, 2018

[WIP] Reproduce GH-540 on 7.1.2 #541

Closed

jcesarmobile changed the base branch from 7.1.x to master November 11, 2018 19:45

jcesarmobile changed the base branch from master to 7.1.x November 11, 2018 19:46

brodycj reviewed Nov 11, 2018

View reviewed changes

GH-539 fix destination path fallback

ca84ca2

Fallback to old path mapping if no Android Studio path mapping exists (with minor change needed to work on for 7.1.x branch) Co-authored-by: Christopher J. Brody <[email protected]> Co-authored-by: Kyle Kirbatski <[email protected]>

brodycj changed the base branch from 7.1.x to master November 11, 2018 22:45

brodycj mentioned this pull request Nov 12, 2018

cordova-plugin-purchase plugin issues #546

Closed

brodycj changed the base branch from master to 7.1.x November 12, 2018 17:17

brodycj closed this Nov 12, 2018

brodycj pushed a commit that referenced this pull request Nov 12, 2018

Merge pull request #542 from brodybits/gh-540-hotfix

576ad18

Resolve GH-539 & GH-540 on master

jcesarmobile pushed a commit to jcesarmobile/cordova-android that referenced this pull request Nov 13, 2018

apacheGH-539 fix destination path fallback

22ccf4b

jcesarmobile mentioned this pull request Nov 13, 2018

Cherry picks #549

Closed

3 tasks

brodycj mentioned this pull request Nov 14, 2018

7.1.3 patch updates #555

Merged

7 tasks

brodycj mentioned this pull request Nov 23, 2018

Deprecate old plugin target-dir structure support #580

Open

dpogue mentioned this pull request Nov 23, 2018

it should be checked if studioPathRemap(obj); is undefined, because target-dir starts with app/src/main/ will become undefined. #583

Closed

davidmather mentioned this pull request May 2, 2023

[Snyk] Upgrade semver from 7.3.8 to 7.4.0 davidmather/cordova-android#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dest overwrite #539

Fix dest overwrite #539

kkirby commented Nov 8, 2018

brodycj commented Nov 9, 2018 •

edited

Loading

brodycj commented Nov 9, 2018

dpogue commented Nov 9, 2018

brodycj commented Nov 9, 2018

jcesarmobile commented Nov 9, 2018 •

edited

Loading

brodycj commented Nov 9, 2018

jcesarmobile commented Nov 9, 2018

kkirby commented Nov 10, 2018

jcesarmobile left a comment

brodycj commented Nov 11, 2018

brodycj Nov 11, 2018

brodycj commented Nov 11, 2018

jcesarmobile commented Nov 11, 2018

brodycj commented Nov 11, 2018

brodycj commented Nov 11, 2018 •

edited

Loading

brodycj commented Nov 11, 2018 •

edited

Loading

jcesarmobile commented Nov 11, 2018

brodycj commented Nov 12, 2018

brodycj commented Nov 12, 2018

brodycj commented Nov 21, 2018

Fix dest overwrite #539

Fix dest overwrite #539

Conversation

kkirby commented Nov 8, 2018

Platforms affected

What does this PR do?

What testing has been done on this change?

Checklist

More details

Related lines

Resolution

brodycj commented Nov 9, 2018 • edited Loading

brodycj commented Nov 9, 2018

dpogue commented Nov 9, 2018

brodycj commented Nov 9, 2018

jcesarmobile commented Nov 9, 2018 • edited Loading

brodycj commented Nov 9, 2018

jcesarmobile commented Nov 9, 2018

kkirby commented Nov 10, 2018

jcesarmobile left a comment

Choose a reason for hiding this comment

brodycj commented Nov 11, 2018

brodycj Nov 11, 2018

Choose a reason for hiding this comment

brodycj commented Nov 11, 2018

jcesarmobile commented Nov 11, 2018

brodycj commented Nov 11, 2018

brodycj commented Nov 11, 2018 • edited Loading

brodycj commented Nov 11, 2018 • edited Loading

jcesarmobile commented Nov 11, 2018

brodycj commented Nov 12, 2018

brodycj commented Nov 12, 2018

brodycj commented Nov 21, 2018

brodycj commented Nov 9, 2018 •

edited

Loading

jcesarmobile commented Nov 9, 2018 •

edited

Loading

brodycj commented Nov 11, 2018 •

edited

Loading

brodycj commented Nov 11, 2018 •

edited

Loading