Fix xcode9 provision, provide --quiet by default, enable -allowProvisioningUpdates by PanayotCankov · Pull Request #3070 · NativeScript/nativescript-cli

PanayotCankov · 2017-08-17T12:32:00Z

Depends on:
#3069

This PR will:

Update the export options plist and list the provisioning profile so Xcode9 can export when working with manual signing style and the --provision switch.
Provide --quiet to xcodebuild and -quiet to the gradle tools so they log only errors by default. The command line args with which these commands are called will be printed in place of the verbose output. You can switch to --log trace for full output.
Provide -allowProvisioningUpdates to xcodebuild when working with Xcode version > 9.0 so when passing team, xcodebuild may generate automatically managed provisioning profiles for you
(THIS ONE IS MY FAVOURITE!!!)
--teamId now behaves like --provision, it will require prepare if the last time other teamId was issued, it will print all teams if no arg is provided, it will switch to automatic signing
using --teamId and --provision will throw that they are mutually exclusive
teamId should work with team name, if we find a provision that has the team name and id
(we should improve that by somehow looking in the accounts registered in Xcode)

…iet flags to xcodebuild and gradle, provide -allowProvisioningUpdates to xcodebuild > 9.0

…re mutually exclusive, teamId now should work with name if a provision that has the team id for the provided team name is found

…-project-service

- Make tests passing with NativeScript/nativescript-cli#3070 - Enable —provision tests for Xcode9

Mitko-Kerezov · 2017-08-22T12:42:29Z

 			const gradlew = this.$hostInfo.isWindows ? "gradlew.bat" : "./gradlew";

+			const localArgs = [...gradleArgs];
+			if (this.$logger.getLevel() === "INFO") {


You can use $loggingLevels.info here instead of a string

let's keep it this way, $loggingLevels has a different purpose. I'll handle the logging levels in a separate PR and will change this code with a constant in it.

Mitko-Kerezov · 2017-08-22T12:50:23Z

 		return { devices, match };
 	}
+
+	public async getDevelopmentTeams(): Promise<{ id: string, name: string }[]> {


{ id: string, name: string } could be extracted in an interface.
This is a personal preference though as I find named interfaces improve code readability

This is used once, if I have to write it out in 2-3 places perhaps I will make it a separate interface. For now type inference works so well, this type is never written out anywhere else in the code.

We have a scenario where we'll need interface, but let's discuss it personally and handle it in a separate PR once it is needed.

Mitko-Kerezov · 2017-08-22T12:52:03Z

 	}
+
+	public async getDevelopmentTeams(): Promise<{ id: string, name: string }[]> {
+		const teams: { [teamName: string]: Set<string> } = {};


Same remark about the interface

This is used locally and just once. I don't want to pollute the module with interfaces. It could be IDictionary<Set<string>> but then it would be even harder to figure out what the key would be.

Mitko-Kerezov · 2017-08-22T13:40:56Z

+		if (teamId === true) {
+			await this.$iOSProvisionService.listTeams();
+			this.$errors.failWithoutHelp("Please provide team id or team name with the --teamId options.");
+			return false;


There's no need for this return statement - this.$errors.failWithoutHelp will terminate the process

Omg. It returns void. :D I will make it "never".

failWithoutHelp(message: string, ...args: any[]): never;
please use the never type for each method that terminate unconditionally the process or throw.

I made a PR for 'never' but it is completely unrelated (I think) with this current PR:
telerik/mobile-cli-lib#1006

Nice one! Yes, it is unrelated

Mitko-Kerezov · 2017-08-22T13:44:26Z

-			args.push(`CODE_SIGN_IDENTITY=${buildConfig.codeSignIdentity}`);
-		}
+		// These are now set in the .pbxproj, avoid passing them as arguments!
+		// if (buildConfig && buildConfig.codeSignIdentity) {


Do we want to keep this code commented out for future reference?

I vote for deleting it

Mitko-Kerezov · 2017-08-22T14:26:31Z

+
+				if (provision !== undefined) {
+					if (signing && signing.style === "Manual") {
+						for (let name in signing.configurations) {


let name could be const name

Can we have a lint rule for these?

We can, but enabling it leads to 1400 compile errors, so I advise against this in this PR

Mitko-Kerezov · 2017-08-22T14:26:45Z

+				if (provision !== undefined) {
+					if (signing && signing.style === "Manual") {
+						for (let name in signing.configurations) {
+							let config = signing.configurations[name];


let could be const

Mitko-Kerezov · 2017-08-22T15:06:06Z

    <string>${options.teamID}</string>
 `;
+		}
+		if (options && options.provision) {


A separate service/helper function could be extracted for getting the exportOptions.plist file and be reused here and here.
This could be done in another PR though

The time is tight, let's do this in a separate PR if we ever touch these two again.

Mitko-Kerezov · 2017-08-22T15:08:04Z

+	}

-		await this.createIpa(projectRoot, projectData, buildConfig);
+	private async setupSigningFromTeam(projectRoot: string, projectData: IProjectData, teamId?: string) {


Is the teamId argument really optional here - what will happen if it is not passed - shouldUpdateXcode would be set to true and then Automatic signing will be set without a team.

Mitko-Kerezov · 2017-08-22T15:08:29Z

+		let shouldUpdateXcode = false;
+		if (signing && signing.style === "Automatic") {
+			if (signing.team !== teamId) {
+				// Maybe the provided team is name such as "Telerik AD" and we need to convert it to CH******37


What if the team is passed as an id instead of a name - we'd still try and look for it as if it was a name

Yes. I think it is highly unlikely to have a team name matching the randomly generated team ids on the same machine. teamId probably should be just team like the provision switch is not provisionIdentifier or provisionUUID. Being able to pass "Telerik AD" for team however is really convenient.

My bad, I didn't manage to express my concern correctly - according to the docs you've written for --team-id one can either pass a team name or a team id.
So what of the case where the user knows the team id and passes it as a value to --team-id - we'd still look for it and validate it as if it was a team name

The code here will do the following:
It will check if the arg passed to team-id matches the team in the Automatic signing style in Xcode. If it does, the native project and prepare are up-to-date.
If it does not - it will consider the team-id to be actually a team name, it will check for a team with name matching the provided team-id and if it finds one it will check if Xcode is configured to sign with that team's id.

This can fail if you have a team that has team id matching one of your other team's names, that's highly unlikely. If this raises as an issue we can split a team-id and team-name switches.

I pretty much try to treat team-id as id, and eventually check the provisioning profiles if it were team name and map the name to id.

rosen-vladimirov · 2017-08-23T05:34:35Z

 			const gradlew = this.$hostInfo.isWindows ? "gradlew.bat" : "./gradlew";

+			const localArgs = [...gradleArgs];
+			if (this.$logger.getLevel() === "INFO") {


let's keep it this way, $loggingLevels has a different purpose. I'll handle the logging levels in a separate PR and will change this code with a constant in it.

rosen-vladimirov · 2017-08-23T05:35:29Z

+			const localArgs = [...gradleArgs];
+			if (this.$logger.getLevel() === "INFO") {
+				localArgs.push("--quiet");
+				this.$logger.info(`${gradlew} ${localArgs.join(" ")}`);


why do we want to print the command that we execute in the info level?

Otherwise nothing is printed. I wanted to print something that indicates that a native build had started. I am ok to go for just "Xcode build..." and "Gradle build...", should I change these?

I prefer using Gradle build...
In case you want to print dots, you can use:

const action = () => this.spawn(gradlew, localArgs, childProcessOpts, spawnFromEventOptions); this.$logger.printInfoMessageOnSameLine("Gradle build..."); await this.$progressIndicator.showProgressIndicator(action(), 2000);

Doesn't this throw garbage progress indicator symbols around if the xcodebuild or gradle build actually output stuff? I've seen rogue npm progress bars printed at random.

This actually sounds reasonable I'll give it a try.

Ok we've agreed to use "Gradle build..." and "Xcode build...".
Bear in mind that we do 3 xcodebuild executions on clean "tns run ios" on device.

rosen-vladimirov · 2017-08-23T05:56:19Z

+	<string>${exportOptionsMethod}</string>`;
+	if (options && options.provision) {
+		plistTemplate += `    <key>provisioningProfiles</key>
+<dict>


this plist seems duplicate of the one used on line 234, can we have it on a single place and reuse it

First Write, Second Copy, Third Refactor

Agreed to handle this in a separate PR

rosen-vladimirov · 2017-08-23T05:57:19Z

-			args.push(`CODE_SIGN_IDENTITY=${buildConfig.codeSignIdentity}`);
-		}
+		// These are now set in the .pbxproj, avoid passing them as arguments!
+		// if (buildConfig && buildConfig.codeSignIdentity) {


I vote for deleting it

rosen-vladimirov · 2017-08-23T05:57:46Z

+				localArgs.push("-allowProvisioningUpdates");
+			}
+		} catch (e) {
+			console.warn("Failed to use xcodebuild with -allowProvisioningUpdates due to error: " + e);


this should be $logger.warn

rosen-vladimirov · 2017-08-23T05:58:07Z

+		} catch (e) {
+			console.warn("Failed to use xcodebuild with -allowProvisioningUpdates due to error: " + e);
+		}
+		if (this.$logger.getLevel() === "INFO") {


Same as above:
let's keep it this way, $loggingLevels has a different purpose. I'll handle the logging levels in a separate PR and will change this code with a constant in it.

rosen-vladimirov · 2017-08-23T05:58:53Z

+		}
+		if (this.$logger.getLevel() === "INFO") {
+			localArgs.push("-quiet");
+			this.$logger.info(`xcodebuild ${localArgs.join(" ")}`);


and again, is it important for the user to see the command we are executing. Until now this was not shown, can we show it in trace level only?

Same,

xcodebuild will not output a single line if run with -quiet and no errors or warning were present. So far the user had seen 10 screens of output in place of this with all the flags and environment variables passed to Xcode, various subtools that compile storyboards or copy PNGs. All of it is now collapsed to this line. I think this line is important otherwise the CLI will stall for several seconds and the user won't know that the native build actually started.

Again I can change the message if you provide me a better one, but I think it is important to appear in the default logging mode when -quiet is passed.

As above

In case you want to print dots, you can use:

const action = () => this.$childProcess.spawnFromEvent("xcodebuild", ... ); this.$logger.printInfoMessageOnSameLine("Building application..."); await this.$progressIndicator.showProgressIndicator(action(), 2000);

ghost · 2017-08-23T06:03:23Z

+ * Formats the argument so it can easily be copied from the terminal and provided as value for an option.
+ * @param arg The string to format.
+ */
+function cmdEscape(arg: string): string {


Maybe quoteString can do the same work for you?

No. It does cmdEscape. Its purpose is to make the string passable as argument to an option. Telerik A D can be escaped as "Telerik A D" or it could be as Telerik\ A\ D and have no commas at all and I wouldn't care less which one would be the result of the escape. On the other hand the escaping is flawed, it would fail on Telerik "AD" as the commas will go nuts with "Telerik "AD"" :( but I don't have time to figure out better escaping.

omg :D I clicked the link, you have cmdQuote :D I will use that one :)

ghost · 2017-08-23T09:51:54Z

@rosen-vladimirov @Mitko-Kerezov
I've addressed most of the important comments in a separate commit.
Could you take a look again?

…ly message...

ghost

LGTM

dtopuzov approved these changes Aug 17, 2017

View reviewed changes

PanayotCankov added 3 commits August 18, 2017 14:19

Setup the provisioningProfiles in the exportOptions plist, provide qu…

3ab47d6

…iet flags to xcodebuild and gradle, provide -allowProvisioningUpdates to xcodebuild > 9.0

Using teamId without arg will print all teams, teamId and provision a…

35cfe6b

…re mutually exclusive, teamId now should work with name if a provision that has the team id for the provided team name is found

Add documentation on --team-id near --provision

88f8629

PanayotCankov force-pushed the fix-xcode9-provision branch from ffb0e8e to 88f8629 Compare August 18, 2017 11:24

Fix versionCompare for the '8.0' and '9.0' versions we compare in ios…

459e8fd

…-project-service

PanayotCankov requested review from Mitko-Kerezov, dtopuzov, pkoleva and rosen-vladimirov August 21, 2017 08:56

PanayotCankov self-assigned this Aug 21, 2017

pkoleva approved these changes Aug 21, 2017

View reviewed changes

dtopuzov added a commit to NativeScript/nativescript-cli-tests that referenced this pull request Aug 21, 2017

Make tests passing with NativeScript/nativescript-cli#3070

f3513a0

- Make tests passing with NativeScript/nativescript-cli#3070 - Enable —provision tests for Xcode9

dtopuzov mentioned this pull request Aug 21, 2017

Make tests passing with https://github.com/NativeScript/nativescript-cli/pull/3070 NativeScript/nativescript-cli-tests#74

Merged

dtopuzov approved these changes Aug 21, 2017

View reviewed changes

Mitko-Kerezov suggested changes Aug 22, 2017

View reviewed changes

rosen-vladimirov suggested changes Aug 23, 2017

View reviewed changes

NativeScript deleted a comment from dtopuzov Aug 23, 2017

Address PR review

646477e

Address another PR comment, native builds will now print short friend…

5742dcd

…ly message...

rosen-vladimirov approved these changes Aug 23, 2017

View reviewed changes

Mitko-Kerezov approved these changes Aug 23, 2017

View reviewed changes

PanayotCankov merged commit 9fb0efc into master Aug 23, 2017

PanayotCankov deleted the fix-xcode9-provision branch August 23, 2017 14:15

Uh oh!

Uh oh!

Conversation

PanayotCankov commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PanayotCankov Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PanayotCankov commented Aug 17, 2017 •

edited

Loading

PanayotCankov Aug 23, 2017 •

edited

Loading

ghost Aug 23, 2017 •

edited by PanayotCankov

Loading

ghost Aug 23, 2017 •

edited by PanayotCankov

Loading