-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(cli): support manual signing on build command #7769
base: main
Are you sure you want to change the base?
Conversation
Im open to any suggestions for better names for the new CLI flags as well. |
<key>${config.app.appId}</key> | ||
<string>${buildOptions.xcodeProvisioningProfile ?? ''}</string> | ||
</dict> | ||
<key>signingCertificate</key> | ||
<string>${buildOptions.xcodeSigningCertificate ?? ''}</string>`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will these be OK as empty strings, or would it fail the build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that block is only added if signing style is manual, and if that's chosen, presumably, the developer would be supplying a provisioning profile and/or signing certificate. If what they supply is wrong (or if it's empty), then it won't work.
I believe the signing certificate can be optional, so I may push up a change to remove that key and string if the value is empty.
Honestly, they look great to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be some validation before trying to build as the build takes a lot of time to end up failing if the signing is set to manual but the user didn't provide the certificate and provisioning profile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should also be possible to provide the values from the Capacitor config file as we do for Android
Lines 218 to 225 in f00349e
const buildOptions = { | |
keystorePath: extConfig.android?.buildOptions?.keystorePath, | |
keystorePassword: extConfig.android?.buildOptions?.keystorePassword, | |
keystoreAlias: extConfig.android?.buildOptions?.keystoreAlias, | |
keystoreAliasPassword: extConfig.android?.buildOptions?.keystoreAliasPassword, | |
signingType: extConfig.android?.buildOptions?.signingType, | |
releaseType: extConfig.android?.buildOptions?.releaseType, | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should delete the buildOptions
from the generated capacitor.config.json
as we do for Android since it can be sensitive.
https://github.com/ionic-team/capacitor/blob/main/cli/src/tasks/copy.ts#L156
Also, should we ask for the development team?
archive fails if I don't have one selected on the Xcode project, it's not related to this change since it also fails with the automatic builds, but I don't think having a team selected should be a requirement for doing a CLI build.
Cordova has a param for it, so we should probably also ask for it.
And I don't think the provisioning profile is working as expected, if I have one selected on Xcode that is invalid the IPA build fails despite the one I passed as param is valid, if I make the Xcode one match the one I pass as param then it works, but defeats the purpose of allowing to pass a provisioning profile if it has to be the same Xcode already has selected.
closes: #7625