search

apache / cordova-android

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

Fix dest overwrite #539

Closed kkirby closed 5 years ago

kkirby commented 5 years ago

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

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

https://github.com/apache/cordova-android/compare/7a97fd7...7.1.2#diff-4f73e5f70cec312ae82aa7d023e3c88bR320 https://github.com/apache/cordova-android/compare/7a97fd7...7.1.2#diff-4f73e5f70cec312ae82aa7d023e3c88bR50

Resolution

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

brodybits commented 5 years ago

Can you confirm the following:

I think this should solve the issue in #540.

brodybits commented 5 years ago

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

dpogue commented 5 years ago

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?

brodybits commented 5 years ago

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 commented 5 years ago

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

brodybits commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

brodybits commented 5 years ago

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.

brodybits commented 5 years ago

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:

@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 commented 5 years ago

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

brodybits commented 5 years ago

What do you mean by "the other one"?

brodybits commented 5 years ago

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.

brodybits commented 5 years ago

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 commented 5 years ago

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

brodybits commented 5 years ago

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

brodybits commented 5 years ago

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.

brodybits commented 5 years ago

Now fixed in the latest patch release