Do not overwrite all targets with same id #545

wolfmanfx · 2019-02-22T09:26:07Z

Prepare script update build properties for the main target only. Wi…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)

Platforms affected

iOS

Motivation and Context

When cordova (using 5.0.0) projects contains watch targets (watchkit, watch extension) which have their own bundle id setup correctly, cordova overwrites all targets with the same id.
So the motivation is clear to have an hybrid app as watchos host without maintaining a manual created xcode project.

Description

See commit message

Testing

Tested against a real project with an watchos app.

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)

wolfmanfx · 2019-02-22T09:29:03Z

Link #538

dpogue · 2019-02-25T06:24:19Z

bin/templates/scripts/cordova/build

@@ -42,6 +42,7 @@ var buildOpts = nopt({
    'codeSignIdentity': String,
    'codeSignResourceRules': String,
    'provisioningProfile': String,
+    'multipleProvisioningProfiles': Array,


I wonder if we should add a new property here for multipleProvisioningProfiles vs changing the existing provisioningProfile property to accept either a single string or an array.

I guess thats a better approach - i will change that.

I have to revert the idea - merging the two options do not work and require more refactoring. So I suggest to add this additional parameter which is only used for the export plist.

dpogue · 2019-02-25T06:25:38Z

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

+function updateBuildPropertyLocal(proj, displayName, prop, value, build) {
+    try {
+        // Check if we have a valid target - during prepare we do not have it
+        var target = proj.pbxTargetByName(displayName);


Does this definitely use the display name, and not the full name? In Cordova apps those are usually the same, but it's not guaranteed (and we provide a way to change the display name).

Can you give me an example how to change the name - I will test that case.

In config.xml:

<name short="MyApp">My Full Application Name</name>

In that example, "MyApp" would be the short name (which is what you're putting into the displayName variable).

Yes you were right - I have fixed the pbx target retrieval (tried both variants)

codecov-io · 2019-02-25T22:11:07Z

Codecov Report

Merging #545 into master will increase coverage by 0.11%.
The diff coverage is 77.14%.

@@            Coverage Diff             @@
##           master     #545      +/-   ##
==========================================
+ Coverage   74.75%   74.86%   +0.11%     
==========================================
  Files          11       11              
  Lines        1822     1850      +28     
==========================================
+ Hits         1362     1385      +23     
- Misses        460      465       +5

Impacted Files	Coverage Δ
bin/templates/scripts/cordova/lib/build.js	`50.75% <0%> (-1.05%)`	⬇️
bin/templates/scripts/cordova/lib/prepare.js	`84.5% <96.42%> (+0.6%)`	⬆️

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 5e688ca...342396b. Read the comment docs.

codecov-io · 2019-02-25T22:11:08Z

Codecov Report

Merging #545 into master will increase coverage by 0.11%.
The diff coverage is 77.14%.

@@            Coverage Diff             @@
##           master     #545      +/-   ##
==========================================
+ Coverage   74.19%   74.31%   +0.11%     
==========================================
  Files          11       11              
  Lines        1829     1857      +28     
==========================================
+ Hits         1357     1380      +23     
- Misses        472      477       +5

Impacted Files	Coverage Δ
bin/templates/scripts/cordova/lib/build.js	`50.24% <0%> (-1.03%)`	⬇️
bin/templates/scripts/cordova/lib/prepare.js	`84.28% <96.42%> (+0.61%)`	⬆️

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 f43812a...86ef135. Read the comment docs.

msari-ipe-ext-1 · 2019-03-01T17:51:26Z

So we are using the fork on a day by day basis everythinh working (with the embeded watch app).
I would love to switch back to the main repo anytime :)

janpio · 2019-03-01T18:01:52Z

Maybe create a companion PR that adds some documentation on that at https://github.com/apache/cordova-docs/blob/master/www/docs/en/dev/guide/platforms/ios/index.md#signing-an-app (I guess?).

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

NiklasMerz · 2019-11-04T12:16:37Z

What's the state of this PR? I am encountering this issue as well.

This PR solved the issue.

timbru31 · 2019-11-04T21:11:06Z

What's the state of this PR? I am encountering this issue as well.

This PR solved the issue.

A code review or a confirmation that this PR solves the issue would help us to merge this faster :)

NiklasMerz · 2019-11-05T07:23:54Z

The problem with overwriting bundle ids is solved with this PR. My tests were successful.

I am not sure if this change with multipleProvisioningProfiles should be part of this PR. I don't get why this is related and it does not work for me, but I have to manage provsioning profiles manually because of two other plugins with app extensions anyway.

I would split that in a seperate PR.

msari-ipe-ext-1 · 2019-11-05T07:30:11Z

The flag overwrites the correct profiles for the given bundle ids - I can build my app within the console without to open xcode. The value is name of the provision profile.
It does make sense to split the PR because no CI would able to build apps without manual editing which is not my usecase (must be builded by travis)

dpogue · 2019-11-05T07:38:13Z

The more I think about this, the more I feel like this is probably outside the scope of what Cordova should be handling.

I'm hesitant to add this for several reasons:

We know that provisioning profiles are already a source of a lot of frustration for Cordova users, and a source of a lot of problems every time a new Xcode version is released. We're trying to encourage everyone to move to automatic signing instead of specifying provisioning profiles manually. Adding this support makes the documentation more confusing and seems to encourage the practice we're trying to move away from.
As you can see from the ongoing discussions about the future of UIWebView and WKWebView, there's a real shortage of people able to contribute time to maintaining and upgrading Cordova. Adding this support might seem simple, but it's yet more code that needs to be maintained. This code has no tests, its use-case is far from the standard usage, and there's a danger that every cleanup effort anyone takes in the future will break it in non-obvious ways.
The purpose of Cordova is ostensibly to package web applications up in a native app wrapper. Yes, it supports native code plugins, but those are typically meant to expose a JS interface to the web application.

If you're at a point where you have multiple targets in Xcode and are juggling multiple provisioning profiles, you should probably be managing the Xcode project yourself (either based on the one generated by Cordova or using a similar tool like Capacitor).

NiklasMerz · 2019-11-05T07:49:22Z

@dpogue I can agree you with some points but have to disagree with others.

I am with you that this change should be minimal to avoid breaking stuff and keep maintainance cost low. That's why I would suggest to simplify this PR and remove the provisioning profile stuff. I am happy to do and/or test this. Is this viable?

But I would strongly argue to fix the core issue "Do not overwrite bundle ids". Cordova iOS 5 does not work for our app but 4.5.5 did before these changes.

You are probably right that at some point projects with multiple targets grow out of the original scope of Cordova. But this can happen organically. Our app now has two plugins which add extensions, because we need those features. Switching to Capacitor is not an option at this point because of other design choices.

Managing profiles and signing manually in Xcode is fine for me, but everything else should work with cordova build. That's why I want to fix the bundleid issue to make upgrading to cordova-ios 5 possible.

msari-ipe-ext-1 · 2019-11-05T07:59:30Z

I see your points - but overwriting existing profiles is a bug, I have combined it with a feature to automate profile settings.
Supporting iOS extensions would be great but I also see that the cordova repos (including plugins) in general are loosing ground (maintainers).
So Capacitor is also or midterm target - we have wasted a lot of time with broken CI builds or forking unmaintained plugins to make it work.

You could split this PR and extract the bug fix and remove the feature.

best regards Murat

NiklasMerz · 2019-11-05T08:56:04Z

Thanks Murat. I would like to see this bug fixed. I took you branch and removed the multipleProvisioningProfiles part. Now this change looks good to me for default Cordova projects with no extensions. I will do some tests and update #708 accordingly.

brodycj

I think too many things are changed at the same time in this proposal. It looks like @NiklasMerz made a nicer subset of the changes in PR [MAJOR] Do not overwrite all targets with same id (minimal) #708.
Test cases would be needed.
I would agree with the comments by @dpogue in Do not overwrite all targets with same id #545 (comment)
I think changes to this extent should only be considered for a major release.

Thanks anyways for the contribution.

NiklasMerz · 2020-03-14T19:30:15Z

Closing this because of the reasons above and many conflicts. We need to address this differently. We should discuss more in the issue when time is ready.

HarelM · 2020-07-26T13:22:08Z

@NiklasMerz @msari-ipe-ext-1 is the multiple provisioning profile available in latest release of cordova-ios? I see that when using the fovea's openwith share extension the exportOptions.plist is create with a single provisioning profile of the shared extention...
Any chance to update the docs of iOS into how to use this...?
https://cordova.apache.org/docs/en/latest/guide/platforms/ios/index.html

wolfmanfx mentioned this pull request Feb 22, 2019

Cordova overwrites bundleIdentifier for all existing targets #538

Closed

janpio changed the title ~~Patch for https://github.com/apache/cordova-ios/issues/538~~ Do not overwrite all targets with same id Feb 22, 2019

janpio added the enhancement label Feb 22, 2019

dpogue reviewed Feb 25, 2019

View reviewed changes

msari-ipe-ext-1 added 3 commits February 25, 2019 22:30

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

160542d

Adapt style

0c24c4b

More linter fixes

342396b

dpogue added this to the 5.0.1 milestone Mar 15, 2019

dpogue modified the milestones: 5.0.1, 5.0.2 Apr 17, 2019

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

86ef135

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

NiklasMerz mentioned this pull request Nov 5, 2019

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

Closed

5 tasks

brodycj suggested changes Dec 16, 2019

View reviewed changes

brodycj removed this from the 5.0.2 milestone Dec 16, 2019

NiklasMerz closed this Mar 14, 2020

NiklasMerz linked an issue Mar 14, 2020 that may be closed by this pull request

Cordova overwrites bundleIdentifier for all existing targets #538

Closed

HarelM mentioned this pull request Jul 26, 2020

Wrongly created exportOptions.plist file - multiple provisioning profile support #953

Closed

3 tasks

davidmather mentioned this pull request Jun 20, 2023

[Snyk] Security upgrade semver from 7.3.5 to 7.5.2 davidmather/cordova-ios#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not overwrite all targets with same id #545

Do not overwrite all targets with same id #545

wolfmanfx commented Feb 22, 2019 •

edited

Loading

wolfmanfx commented Feb 22, 2019

dpogue Feb 25, 2019

msari-ipe-ext-1 Feb 25, 2019

wolfmanfx Feb 25, 2019

dpogue Feb 25, 2019

msari-ipe-ext-1 Feb 25, 2019

dpogue Feb 25, 2019

wolfmanfx Feb 25, 2019

codecov-io commented Feb 25, 2019

codecov-io commented Feb 25, 2019 •

edited

Loading

msari-ipe-ext-1 commented Mar 1, 2019

janpio commented Mar 1, 2019

NiklasMerz commented Nov 4, 2019 •

edited

Loading

timbru31 commented Nov 4, 2019 •

edited

Loading

NiklasMerz commented Nov 5, 2019

msari-ipe-ext-1 commented Nov 5, 2019

dpogue commented Nov 5, 2019

NiklasMerz commented Nov 5, 2019

msari-ipe-ext-1 commented Nov 5, 2019

NiklasMerz commented Nov 5, 2019

brodycj left a comment

NiklasMerz commented Mar 14, 2020

HarelM commented Jul 26, 2020

Do not overwrite all targets with same id #545

Do not overwrite all targets with same id #545

Conversation

wolfmanfx commented Feb 22, 2019 • edited Loading

Platforms affected

Motivation and Context

Description

Testing

Checklist

wolfmanfx commented Feb 22, 2019

dpogue Feb 25, 2019

Choose a reason for hiding this comment

msari-ipe-ext-1 Feb 25, 2019

Choose a reason for hiding this comment

wolfmanfx Feb 25, 2019

Choose a reason for hiding this comment

dpogue Feb 25, 2019

Choose a reason for hiding this comment

msari-ipe-ext-1 Feb 25, 2019

Choose a reason for hiding this comment

dpogue Feb 25, 2019

Choose a reason for hiding this comment

wolfmanfx Feb 25, 2019

Choose a reason for hiding this comment

codecov-io commented Feb 25, 2019

Codecov Report

codecov-io commented Feb 25, 2019 • edited Loading

Codecov Report

msari-ipe-ext-1 commented Mar 1, 2019

janpio commented Mar 1, 2019

NiklasMerz commented Nov 4, 2019 • edited Loading

timbru31 commented Nov 4, 2019 • edited Loading

NiklasMerz commented Nov 5, 2019

msari-ipe-ext-1 commented Nov 5, 2019

dpogue commented Nov 5, 2019

NiklasMerz commented Nov 5, 2019

msari-ipe-ext-1 commented Nov 5, 2019

NiklasMerz commented Nov 5, 2019

brodycj left a comment

Choose a reason for hiding this comment

NiklasMerz commented Mar 14, 2020

HarelM commented Jul 26, 2020

wolfmanfx commented Feb 22, 2019 •

edited

Loading

codecov-io commented Feb 25, 2019 •

edited

Loading

NiklasMerz commented Nov 4, 2019 •

edited

Loading

timbru31 commented Nov 4, 2019 •

edited

Loading