[MAJOR] Do not overwrite all targets with same id (minimal) #708

NiklasMerz · 2019-11-05T08:53:12Z

Platforms affected

iOS

Motivation and Context

This is a simplified version of #545 with the minimal changes to fix #538

Description

See discussion in #545

Testing

Manual tests with complex project

Checklist

I've run the tests to see all new and existing tests pass
I added automated test coverage as appropriate for this change
Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
I've updated the documentation if necessary

…th the old behavior it were not possible to add apple watch targets and build it because cordova bricked the bundleids for these targets. * Add new build parameter multipleProvisioningProfiles. Here the usage within the build.json: "multipleProvisioningProfiles": [ { "key": "de.demo.html5", "value": "DemoME-Enterprise" }, { "key": "de.demo.html5.watchkitapp", "value": "DemoWE-Enterprise" }, { "key": "de.demo.html5.watchkitapp.watchkitextension", "value": "DemoWE Extension-Enterprise" } ], This one is needed too otherwise the signing step fails because cordova generates a wrong bundleid to prov.profile mapping. With these settings the exportOptions.plist is written correctly for multiple targets. This patch is needed to make the apple watch targets work with the cordova-ios workflow (apple watch targets added within a post process scripts using npm xcode)

# Conflicts: # bin/templates/scripts/cordova/lib/prepare.js

bin/templates/scripts/cordova/lib/prepare.js

…sion

Extension should work on iPads

NiklasMerz · 2019-11-06T09:03:26Z

I changed the the original PR that way that only the bundle id gets checked for the target. Other settings should be applied to other targets, too.

For my type of extensions this necessary. @msari-ipe-ext-1 Does this work for watch extensions?

NiklasMerz · 2019-12-16T12:54:10Z

Would be great to get this and this apache/cordova-node-xcode#79 fixed for the next release. How can I help to get this ready?

brodycj · 2019-12-16T18:26:28Z

Hi @NiklasMerz I may need some extra time to understand the history of issue #538, previous attempts such as #545, this proposal, and apache/cordova-node-xcode#79, before I am ready to answer your question. I would also like to point to the sustainability issue I raised in apache/cordova#163. Thanks for your understanding and thanks for your recent contributions!

brodycj · 2019-12-16T19:00:02Z

I added [MAJOR] to the title and set the milestone to the next major release (6.0.0), due to the risk of this change breaking something else.

brodycj

I think test cases are needed to help ensure this functionality does not get broken in the future.
I think this change should be done in a major release due to the risk of breaking something, as I said in another comment.
I find the added code pretty hard to understand, wonder if there is anything we can do to make it easier to understand.
Use of the displayName variable name does not make sense to me. Please explain it and please consider changing it.

Please feel free to follow up on GitHub, mailing list, or Slack (see footer of cordova.io for contact information). As I said before and in apache/cordova#163, committers are overloaded due to resource issues.

NiklasMerz · 2019-12-16T19:41:56Z

Thanks @brodybits for your review. Good points.

I am aware of the current maintainer situation. I will do my best to clear this code up and explain the open questions as soon as I find time at work with my dev environment on hand.

Tests are a good idea since this should be a fix for a regression as described in #583. To put that confusing discussion into short words: Cordova previously did not touch additional targets in Xcode projects (like Watch extension, call directroy extension ..). With Cordova 5 that changed and thos additinal targets get the name of the host app for example. This is undesirable for certain complex apps with additional targets.

This PR tries to bring back the old behavior basically. I think the whole replacing placeholder stuff in Xcode projects changed in Cordova 5. That's why apache/cordova-node-xcode#79 is needed to fix the PRODUCT_BUNDLE_IDENTIFIER placeholder while adding targets with hooks.

As said before I am going to work on the code and I am open to discussions. We are using to extensions to our Cordova app and this is really important. A new release with this fix is not urgent because using forks works is fine for a while but I don't want to maintain these forks forever.

NiklasMerz · 2019-12-16T20:15:04Z

I updated the displayName variable to make this clear.

I can agree the updateBuildPropertyLocal method is not that easy to understand but it is handling info.plist file wizardry I don't understand either. The method is written by @wolfmanfx in #545 and just put together in this PR.

@wolfmanfx What do you think of this PR? Do you have ideas how to improve this method?

brodycj · 2019-12-16T20:33:06Z

Thanks @NiklasMerz for the recent fix, looks nice. And thanks for your help and understanding of our maintainer situation.

I completely understand that you are basically adapting a bug fix from someone else, with the obvious challenges of taking over code with missing test cases.

I would like to continue the discussion in #538 which I have just pinned in order to get a better understanding of the whole situation of this functionality.

NiklasMerz · 2019-12-16T20:40:50Z

Yes the missing test cases part is crucial. That kind of app is not considered in the tests and I am having a hard time thinking of adding it right now.

But I must admit that using extensions with Cordova might not be a often used features except for plugins like open with .

codecov-io · 2019-12-17T21:21:36Z

Codecov Report

Merging #708 into master will increase coverage by 0.2%.
The diff coverage is 91.66%.

@@            Coverage Diff            @@
##           master     #708     +/-   ##
=========================================
+ Coverage   74.42%   74.63%   +0.2%     
=========================================
  Files          11       11             
  Lines        1830     1853     +23     
=========================================
+ Hits         1362     1383     +21     
- Misses        468      470      +2

Impacted Files	Coverage Δ
bin/templates/scripts/cordova/lib/prepare.js	`84.4% <91.66%> (+0.34%)`	⬆️

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 266d339...41ac8e3. Read the comment docs.

timbru31 · 2019-12-17T21:27:25Z

I've restarted the timed out Travis test, CI is now green

bin/templates/scripts/cordova/lib/prepare.js

Co-Authored-By: エリス <erisu@users.noreply.github.com>

Used a throw to fall back to default. Changed a for loop to use for-of See: #1

erisu

👍 JS stuff LGTM

@msari-ipe-ext-1

Combined patch from apache#708 from @msari-ipe-ext-1 with refactorings Co-authored-by: Murat Sari <murat.sari@coderabbit.at>

NiklasMerz · 2020-03-14T19:28:59Z

This code somehow works for my project, but is not correct. We need to find a better solution possibly involving a change in cordova-node-xcode.

I would love to contribute this but I think it is beyon my capabilities and understandings of Cordova. I tried to work on this a few times but I head always explodes. I appreciate if someone with more experience has the capacity to work on this and I will do my best to support that.

msari-ipe-ext-1 and others added 6 commits February 22, 2019 10:20

* Do not use the short name to retrieve the pbx target

160542d

Adapt style

0c24c4b

More linter fixes

342396b

Merge branch 'master' of github.com:apache/cordova-ios

86ef135

# Conflicts: # bin/templates/scripts/cordova/lib/prepare.js

Remove 'multipleProvisioningProfiles' again

55a06f9

NiklasMerz mentioned this pull request Nov 5, 2019

Do not overwrite all targets with same id #545

Closed

5 tasks

NiklasMerz marked this pull request as ready for review November 5, 2019 09:30

timbru31 requested a review from dpogue November 5, 2019 15:51

dpogue reviewed Nov 5, 2019

View reviewed changes

bin/templates/scripts/cordova/lib/prepare.js Outdated Show resolved Hide resolved

NiklasMerz added 2 commits November 6, 2019 09:56

Revert updateBuildPropertyLocal for deployment target and swift ver…

41d4767

…sion

Set device family the old way, too

9e66101

Extension should work on iPads

Merge branch 'master' into bundleid

4c3b0ef

brodycj changed the title ~~Do not overwrite all targets with same id (minimal)~~ [MAJOR] Do not overwrite all targets with same id (minimal) Dec 16, 2019

brodycj added this to the 6.0.0 milestone Dec 16, 2019

brodycj suggested changes Dec 16, 2019

View reviewed changes

NiklasMerz requested review from dpogue December 16, 2019 19:47

Rename "displayName" to "targetName"

88ae1f9

NiklasMerz mentioned this pull request Dec 16, 2019

Cordova overwrites bundleIdentifier for all existing targets #538

Closed

erisu reviewed Dec 24, 2019

View reviewed changes

bin/templates/scripts/cordova/lib/prepare.js Outdated Show resolved Hide resolved

erisu reviewed Dec 24, 2019

View reviewed changes

bin/templates/scripts/cordova/lib/prepare.js Outdated Show resolved Hide resolved

erisu reviewed Dec 24, 2019

View reviewed changes

bin/templates/scripts/cordova/lib/prepare.js Outdated Show resolved Hide resolved

erisu reviewed Dec 24, 2019

View reviewed changes

bin/templates/scripts/cordova/lib/prepare.js Outdated Show resolved Hide resolved

NiklasMerz and others added 7 commits December 24, 2019 11:01

Update bin/templates/scripts/cordova/lib/prepare.js

a7b5ca2

Co-Authored-By: エリス <erisu@users.noreply.github.com>

Update bin/templates/scripts/cordova/lib/prepare.js

dab75b2

Co-Authored-By: エリス <erisu@users.noreply.github.com>

Update bin/templates/scripts/cordova/lib/prepare.js

f721ea3

Co-Authored-By: エリス <erisu@users.noreply.github.com>

Merge branch 'master' into bundleid

7eab11e

Fix code style issues

c5f5056

Code styling

57f9fb7

chore: updated syntax

f9fd874

Used a throw to fall back to default. Changed a for loop to use for-of See: #1

NiklasMerz requested a review from erisu December 25, 2019 00:56

erisu approved these changes Dec 25, 2019

View reviewed changes

Merge branch 'master' into bundleid

41ac8e3

NiklasMerz pushed a commit to GEDYSIntraWare/cordova-ios that referenced this pull request Feb 24, 2020

Deployment target patch

67d7691

Combined patch from apache#708 from @msari-ipe-ext-1 with refactorings Co-authored-by: Murat Sari <murat.sari@coderabbit.at>

Merge branch 'master' into bundleid

cb4c651

NiklasMerz closed this Mar 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAJOR] Do not overwrite all targets with same id (minimal) #708

[MAJOR] Do not overwrite all targets with same id (minimal) #708

NiklasMerz commented Nov 5, 2019

NiklasMerz commented Nov 6, 2019 •

edited

Loading

NiklasMerz commented Dec 16, 2019

brodycj commented Dec 16, 2019

brodycj commented Dec 16, 2019

brodycj left a comment

NiklasMerz commented Dec 16, 2019 •

edited

Loading

NiklasMerz commented Dec 16, 2019

brodycj commented Dec 16, 2019

NiklasMerz commented Dec 16, 2019

codecov-io commented Dec 17, 2019 •

edited

Loading

timbru31 commented Dec 17, 2019

erisu left a comment •

edited

Loading

NiklasMerz commented Mar 14, 2020

[MAJOR] Do not overwrite all targets with same id (minimal) #708

[MAJOR] Do not overwrite all targets with same id (minimal) #708

Conversation

NiklasMerz commented Nov 5, 2019

Platforms affected

Motivation and Context

Description

Testing

Checklist

NiklasMerz commented Nov 6, 2019 • edited Loading

NiklasMerz commented Dec 16, 2019

brodycj commented Dec 16, 2019

brodycj commented Dec 16, 2019

brodycj left a comment

Choose a reason for hiding this comment

NiklasMerz commented Dec 16, 2019 • edited Loading

NiklasMerz commented Dec 16, 2019

brodycj commented Dec 16, 2019

NiklasMerz commented Dec 16, 2019

codecov-io commented Dec 17, 2019 • edited Loading

Codecov Report

timbru31 commented Dec 17, 2019

erisu left a comment • edited Loading

Choose a reason for hiding this comment

NiklasMerz commented Mar 14, 2020

NiklasMerz commented Nov 6, 2019 •

edited

Loading

NiklasMerz commented Dec 16, 2019 •

edited

Loading

codecov-io commented Dec 17, 2019 •

edited

Loading

erisu left a comment •

edited

Loading