Remove signing setup from Xamarin iOS task #6853

madhurig · 2018-04-02T12:50:22Z

No description provided.

lkillgore · 2018-04-02T14:22:40Z

Tasks/XamariniOS/xamarinios.ts

-            //determine the provisioning profile UUID
-            if (tl.filePathSupplied('provProfile') && tl.exist(provProfilePath)) {
-                provProfileUUID = await sign.getProvisioningProfileUUID(provProfilePath);
+        let provProfileUUID: string = tl.getInput('provProfileUuid');


I think both of these two variables can be const now.

brcrista · 2018-04-02T13:49:15Z

Tasks/XamariniOS/Strings/resources.resjson/en-US/resources.resjson

@@ -3,6 +3,7 @@
  "loc.helpMarkDown": "[More Information](https://go.microsoft.com/fwlink/?LinkID=613729)",
  "loc.description": "Build an iOS app with Xamarin on macOS",
  "loc.instanceNameFormat": "Build Xamarin.iOS solution $(solution)",
+  "loc.releaseNotes": "iOS signing setup has been removed from the task. Use `Secure Files` with supporting tasks `Install Apple Certificate` and `Install Apple Provisioning Profile` to setup signing. Updated options to work better with `Visual Studio for Mac`.",


nit: "set up" as a verb should be two words

brcrista · 2018-04-02T13:52:25Z

Tasks/XamariniOS/xamarinios.ts

-            //determine the provisioning profile UUID
-            if (tl.filePathSupplied('provProfile') && tl.exist(provProfilePath)) {
-                provProfileUUID = await sign.getProvisioningProfileUUID(provProfilePath);
+        let provProfileUUID: string = tl.getInput('provProfileUuid');


can be const

And pedantic nit, I think this should match the input name: provProfileUuid. Acronyms should get cased like normal words so they don't mess up the camel casing

brcrista · 2018-04-02T13:53:53Z

Tasks/XamariniOS/xamarinios.ts

@@ -152,7 +101,7 @@ async function run() {
            buildRunner.line(args);
        }
        buildRunner.argIf(codesignKeychain, '/p:CodesignKeychain=' + codesignKeychain);
-        if (buildTool === 'msbuild' && signIdentity && signIdentity.indexOf(',') > 0) {
+        if (signIdentity && signIdentity.indexOf(',') > 0) {


Instead of indexOf, you can make this signIdentity.includes(',')

Are you sure that's true? IIRC, (and it's very possible I don't RC) we use indexOf because of TS / JS version compatibility. Do we support the version of Javascript in the tasks project that can use the contains method?

Sorry this is supposed to be includes; I've updated my original comment.
String.prototype.includes was introduced with ES 2015. I tested it on Node 5 and 6 just now and it is supported on both.

brcrista · 2018-04-02T14:03:59Z

Tasks/XamariniOS/Tests/L0FallbackXbuild.ts

+    "exec": {
+        "/home/bin/xbuild src/project.sln /p:Configuration=Release /p:Platform=iPhone /t:Clean": {
+            "code": 0,
+            "stdout": "msbuild"


copy-paste error?

brcrista · 2018-04-02T14:17:20Z

Tasks/XamariniOS/xamarinios.ts

-                buildToolPath = path.join(buildToolLocation, 'xbuild');
-            }
-            if (buildTool === 'msbuild' && !buildToolLocation.toLowerCase().endsWith('msbuild')) {
+            if (buildToolLocation && !buildToolLocation.toLowerCase().endsWith('msbuild')) {


I can see we want to push the task to use msbuild from the changes to the UI strings and the logic here. But it appears that this is our logic now:

User supplies path/to/msbuild/directory/msbuild and we run path/to/msbuild/directory/msbuild

User supplies path/to/msbuild/directory and we run path/to/msbuild/directory/msbuild (task finds executable)

User supplies path/to/xbuild/directory/xbuild and we run path/to/xbuild/directory/xbuild

User supplies path/to/xbuild/directory and we try to run path/to/xbuild/directory/msbuild (and fail)

User does not supply buildToolLocation. We find msbuild on the path, else we find xbuild on the path.

Is this correct? It seems odd to me. I think it would make more sense to require the user to provide the full path instead of trying to be smart about whether they provided a directory or file.

As written, it looks like the task will be confused if the msbuild executable lives in a directory named "msbuild." So it sounds like you want to stat the path to see what it is.

In new version of the task we don't support "xbuild" for buildToolLocation at all. I think I will remove this and simplify it to look like this: https://github.com/Microsoft/vsts-tasks/blob/master/Tasks/MSBuild/task.json#L78. In Xamarin iOS first there was mdtool, then xbuild and then msbuild. Hopefully we can stick to msbuild now.

brcrista · 2018-04-02T14:24:54Z

Tests-Legacy/L0/XamariniOS/_suite.ts


        tr.run()
        .then(() => {
            assert(tr.invokedToolCount == 0, 'should not have run XamariniOS');
            assert(tr.resultWasSet, 'task should have set a result');
            assert(tr.stderr.length > 0, 'should have written to stderr');
            assert(tr.failed, 'task should have failed');
-            assert(tr.stderr.indexOf('not found build tool: /user/bin/xbuild') >= 0, 'wrong error message');            
+            assert(tr.stderr.indexOf('not found build tool: /user/bin/msbuild') >= 0, 'wrong error message');            


Can use contains. And since tr.stderr is a string, we might as well print out what the actual stderr was in the diagnostic message.

lkillgore · 2018-04-02T14:26:14Z

Tasks/XamariniOS/xamarinios.ts

@@ -170,27 +119,7 @@ async function run() {

    } catch (err) {
        tl.setResult(tl.TaskResult.Failed, tl.loc('XamariniOSFailed', err));
-    } finally {
-        //clean up the temporary keychain, so it is not used to search for code signing identity in future builds
-        if (codesignKeychain) {


I think this variable can be removed now.

lkillgore · 2018-04-02T14:26:44Z

Tasks/XamariniOS/xamarinios.ts

@@ -152,7 +101,7 @@ async function run() {
            buildRunner.line(args);
        }
        buildRunner.argIf(codesignKeychain, '/p:CodesignKeychain=' + codesignKeychain);


I think this line isn't used anymore.

Good point, we decided not to expose this in the task inputs for the Xcode task as well. I will remove it.

lkillgore · 2018-04-02T14:27:02Z

Tasks/XamariniOS/xamarinios.ts

-        }
-
-        //delete provisioning profile if specified
-        if (profileToDelete) {


I think this variable can be removed.

Remove signing setup from Xamarin iOS task

a1eb060

lkillgore reviewed Apr 2, 2018

View reviewed changes

brcrista reviewed Apr 2, 2018

View reviewed changes

lkillgore reviewed Apr 2, 2018

View reviewed changes

PR feedback

6ef844f

brcrista approved these changes Apr 2, 2018

View reviewed changes

lkillgore approved these changes Apr 2, 2018

View reviewed changes

madhurig merged commit f66bc31 into master Apr 2, 2018

madhurig deleted the users/madhurig/xios_sign branch April 2, 2018 19:58

hashtagchris mentioned this pull request Apr 18, 2019

Check for Expired iOS Codesigning Certificate #8808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove signing setup from Xamarin iOS task #6853

Remove signing setup from Xamarin iOS task #6853

madhurig commented Apr 2, 2018

lkillgore Apr 2, 2018

brcrista Apr 2, 2018

brcrista Apr 2, 2018

brcrista Apr 2, 2018

brcrista Apr 2, 2018 •

edited

Loading

lkillgore Apr 2, 2018

brcrista Apr 2, 2018

lkillgore Apr 2, 2018

brcrista Apr 2, 2018

brcrista Apr 2, 2018

brcrista Apr 2, 2018

madhurig Apr 2, 2018

brcrista Apr 2, 2018

lkillgore Apr 2, 2018

lkillgore Apr 2, 2018

madhurig Apr 2, 2018

lkillgore Apr 2, 2018

Remove signing setup from Xamarin iOS task #6853

Remove signing setup from Xamarin iOS task #6853

Conversation

madhurig commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brcrista Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brcrista Apr 2, 2018 •

edited

Loading