Skip to content
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

Fixes $(PRODUCT_BUNDLE_IDENTIFIER) not being resolved for a product archive #494

Merged
merged 5 commits into from
Jan 14, 2019

Conversation

shazron
Copy link
Member

@shazron shazron commented Jan 10, 2019

Platforms affected

iOS

What does this PR do?

Fixes #407 (not sure if it does it for everything yet)

What testing has been done on this change?

Manual

Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌 Thanks for tracking this down and resolving it Shazron!

@shazron
Copy link
Member Author

shazron commented Jan 10, 2019

Note that this does not resolve all types of variables in the CFBundleIdentifier, just the simple case of the value being one variable

@shazron
Copy link
Member Author

shazron commented Jan 10, 2019

Bah linting errors, will fix

@knight9999
Copy link
Contributor

knight9999 commented Jan 10, 2019

Thanks @shazron.

The ios-deploy successfully works! with following command

$ npx cordova@nightly run --device --codeSignIdentity="My certificate id" --provisioningProfile="My provisioning ID"

However I had still following manual changes.

  1. In cordova/lib/run.js
var fs = require('fs-extra');
...
                        // shell.rm('-rf', appFile);
                        fs.removeSync(appFile);
  1. change automatic signing to manual signing.
    (this corresponds to the following code
< 						ProvisioningStyle = Automatic;
---
> 						ProvisioningStyle = Manual;

in project.pbxproj file)

@codecov-io
Copy link

codecov-io commented Jan 10, 2019

Codecov Report

Merging #494 into master will decrease coverage by 0.81%.
The diff coverage is 11.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
- Coverage   75.56%   74.75%   -0.82%     
==========================================
  Files          11       11              
  Lines        1801     1822      +21     
==========================================
+ Hits         1361     1362       +1     
- Misses        440      460      +20
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/run.js 24.07% <50%> (+0.7%) ⬆️
bin/templates/scripts/cordova/lib/build.js 51.79% <8.33%> (-5.92%) ⬇️

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 8e718c0...04211da. Read the comment docs.

@shazron
Copy link
Member Author

shazron commented Jan 10, 2019

Yeah I should add that cordova/lib/run.js fix in.

Changing from automatic signing to manual signing still needs to be user interactive for now. I'll leave this PR open in case I figure how to do it from code (using node-xcode). Two things need to be changed, that setting you mentioned (see screenshot), and CODE_SIGN_STYLE=Manual (from Automatic).

screen shot 2019-01-10 at 4 25 42 pm

@dpogue
Copy link
Member

dpogue commented Jan 11, 2019

I'll leave this PR open in case I figure how to do it from code (using node-xcode).

This should work...

# line 209
 if (buildOpts.provisioningProfile && bundleIdentifier) {
     exportOptions.provisioningProfiles = { [ bundleIdentifier ]: String(buildOpts.provisioningProfile) };
     exportOptions.signingStyle = 'manual';

+    events.emit('verbose', 'Set CODE_SIGN_STYLE to Manual.');
+    project.xcode.updateBuildProperty('CODE_SIGN_STYLE', 'Manual');
+    project.xcode.addTargetAttribute('ProvisioningStyle', 'Manual');
+
+    fs.writeFileSync(locations.pbxproj, project.xcode.writeSync(), 'utf-8');
 }

@dpogue
Copy link
Member

dpogue commented Jan 11, 2019

Been playing with this and updating the Xcode project, but I still haven't managed to get a working build using automatic signing. It's just making a build with no embedded.mobileprovision.

As part of #489 I moved most of the build.json properties that had been going into buildExtras.xcconfig into the xcodeproj (for consistency with Xcode-generated projects).

@shazron
Copy link
Member Author

shazron commented Jan 14, 2019

Thanks @dpogue! I'll add those settings and test

- If a provisioning profile is set, set CODE_SIGN_STYLE build setting to 'Manual'
- If a provisioning profile is set, set ProvisioningStyle target attribute to 'Manual'
- if provisioning profile is NOT set AND 'automaticProvisioning' build option is set to TRUE, reverse the manual settings from above to 'Automatic'
@shazron
Copy link
Member Author

shazron commented Jan 14, 2019

@dpogue I had to move your code upwards one in the promise chain, before the build runs. If the --provisioningProfile build option is set, the code sets the build settings to Manual. If no --provisioningProfile build option is set and --automaticProvisioning build option is set to true, the code reverses the build settings to Automatic.

@shazron shazron merged commit c2cab27 into master Jan 14, 2019
@shazron shazron deleted the shazron-patch-1 branch January 14, 2019 05:29
erisu pushed a commit to erisu/cordova-ios that referenced this pull request Jan 16, 2019
…rchive (apache#494)

* Fix shelljs error in not removing symlinks during run command
* Changed build settings in Xcode project for a manual build
- If a provisioning profile is set, set CODE_SIGN_STYLE build setting to 'Manual'
- If a provisioning profile is set, set ProvisioningStyle target attribute to 'Manual'
- if provisioning profile is NOT set AND 'automaticProvisioning' build option is set to TRUE, reverse the manual settings from above to 'Automatic'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants