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

Resign of IPA before submit. #41

Closed
janniklind opened this issue Oct 21, 2016 · 20 comments
Closed

Resign of IPA before submit. #41

janniklind opened this issue Oct 21, 2016 · 20 comments

Comments

@janniklind
Copy link
Contributor

Often we build the app using an enterprise profile to allow our testers to install it on their phone, it would be great if we could just resign that build instead of making a new one before submitting to AppStore.

Maybe it could be just a new build step, which could be useful in other scenarios to.

The XamariniOS task supports uploading and installing a provisioning profile, this works great, since it allows to build on hosted VSTS agents like the ones from MacinCloud.

https://github.com/Microsoft/vsts-tasks/tree/master/Tasks/XamariniOS

https://support.macincloud.com/support/solutions/articles/8000023177-versions-of-tools-and-applications-on-vsts-agent-plan-servers-

@madhurig
Copy link
Contributor

@janniklind: Thank you for the feedback. This functionality might be useful to include in the built in tasks instead of the extension. We will consider where to add it.

@janniklind
Copy link
Contributor Author

Hi again.

I have some time available now.

If I wanted to implemented this myself and submit a pull request, would you then prefer it to be in this extension or would you prefer it to be in the built in tasks?

If the choice falls on this extension, can you point me to a "How to build" guide? because I have troubles building these tasks. I however can build the built in tasks without any problems.

I guess I will be using fastlane to do the actual resign no matter where we end up placing this resign task.

@madhurig
Copy link
Contributor

@janniklind: To resign using fastlane, it will be better to do it as a task in the extension. In the built in tasks it makes sense to have an export task that will allow you to export using different identity than the one used to archive. To build the tasks in the extension run "node makevsix.js maketest"

Thanks,
Madhuri

@janniklind
Copy link
Contributor Author

@madhurig: I will use the fastlane sigh resign, since I am using it already in other setups, so I know that better. Also I do not want do maintain the resign part myself.

With regards to built these extension tasks, I get the same error as always.

I clone this repository and do a checkout on the master branch. I then perform the following:

Janniks-MacBook-Pro:app-store-vsts-extension Jannik$ npm install
app-store-vsts-extension@0.0.1 /Users/Jannik/Development/app-store-vsts-extension
├─┬ command-line-args@2.1.6
│ ├── array-back@1.0.3
│ ├─┬ command-line-usage@2.0.5
│ │ ├─┬ ansi-escape-sequences@2.2.2
│ │ │ └─┬ collect-all@0.2.1
│ │ │ ├── stream-connect@1.0.2
│ │ │ └── stream-via@0.1.1
│ │ ├─┬ column-layout@2.1.4
│ │ │ ├─┬ collect-json@1.0.8
│ │ │ │ ├── collect-all@1.0.2
│ │ │ │ └── stream-via@1.0.3
│ │ │ ├── deep-extend@0.4.1
│ │ │ └─┬ object-tools@2.0.6
│ │ │ ├── object-get@2.1.0
│ │ │ └── test-value@1.1.0
│ │ └── wordwrapjs@1.2.1
│ ├── core-js@2.4.1
│ ├── feature-detect-es6@1.3.1
│ ├─┬ find-replace@1.0.2
│ │ └── test-value@2.1.0
│ └── typical@2.6.0
├── q@1.4.1
└── shelljs@0.6.1

npm WARN app-store-vsts-extension@0.0.1 No repository field.
Janniks-MacBook-Pro:app-store-vsts-extension Jannik$
Janniks-MacBook-Pro:app-store-vsts-extension Jannik$ node makevsix.js --maketest
Installing task dependencies and compiling tasks...
2 tasks found.
Processing task app-store-promote
Installing npm dependencies for task...
npm WARN deprecated node-uuid@1.4.7: use uuid module instead
app-store-promote@0.0.1 /Users/Jannik/Development/app-store-vsts-extension/Tasks/app-store-promote
└─┬ vsts-task-lib@0.9.20
├─┬ glob@6.0.4
│ ├─┬ inflight@1.0.6
│ │ └── wrappy@1.0.2
│ ├── inherits@2.0.3
│ ├── once@1.4.0
│ └── path-is-absolute@1.0.1
├─┬ minimatch@3.0.3
│ └─┬ brace-expansion@1.1.6
│ ├── balanced-match@0.4.2
│ └── concat-map@0.0.1
├── mockery@1.7.0
├── node-uuid@1.4.7
├── q@1.4.1
├── semver@5.3.0
├── shelljs@0.3.0
└── vso-node-api@0.6.1

Compiling task...
app-store-promote.ts(1,21): error TS2307: Cannot find module 'os'.
app-store-promote.ts(2,23): error TS2307: Cannot find module 'path'.
app-store-promote.ts(49,24): error TS2304: Cannot find name 'process'.
app-store-promote.ts(49,52): error TS2304: Cannot find name 'process'.
app-store-promote.ts(49,92): error TS2304: Cannot find name 'process'.
app-store-promote.ts(49,141): error TS2304: Cannot find name 'process'.
app-store-promote.ts(50,9): error TS2304: Cannot find name 'process'.
app-store-promote.ts(51,9): error TS2304: Cannot find name 'process'.
app-store-promote.ts(52,9): error TS2304: Cannot find name 'process'.
app-store-promote.ts(55,9): error TS2304: Cannot find name 'process'.
app-store-promote.ts(55,31): error TS2304: Cannot find name 'process'.
node_modules/vsts-task-lib/task.d.ts(1,20): error TS2307: Cannot find module 'q'.
node_modules/vsts-task-lib/task.d.ts(2,21): error TS2307: Cannot find module 'fs'.
node_modules/vsts-task-lib/task.d.ts(8,32): error TS2503: Cannot find namespace 'NodeJS'.
node_modules/vsts-task-lib/task.d.ts(9,32): error TS2503: Cannot find namespace 'NodeJS'.
node_modules/vsts-task-lib/task.d.ts(208,64): error TS2304: Cannot find name 'Buffer'.
node_modules/vsts-task-lib/toolrunner.d.ts(1,20): error TS2307: Cannot find module 'q'.
node_modules/vsts-task-lib/toolrunner.d.ts(2,25): error TS2307: Cannot find module 'events'.
node_modules/vsts-task-lib/toolrunner.d.ts(3,25): error TS2307: Cannot find module 'stream'.
Compilation for task app-store-promote failed

I have then tried "npm install os", "npm install path" and so on, and it installs these dependencies, however I still get the same error when I try "node makevsix.js --maketest".

@jeffyoung
Copy link
Contributor

We will have to fix the script to do this for you, but you'll first need to install the 'typings' tool (npm install typings --global) and then in each task folder (e.g., Tasks\app-store-promote, Tasks\app-store-release) run 'typings -i' to install the typings files needed by the build.

Once you do that (I've confirmed it), the build will succeed.

@madhurig
Copy link
Contributor

madhurig commented Nov 29, 2016

@janniklind: As Jeff pointed out the typings are missing from the repo. You will need to run the following in each task folder and then run "node makevsix.js --maketest":

$ typings install dt~node --global --save
$ typings install dt~q --global --save

I will fix the script/repo to not require this.

Thanks,
Madhuri

@janniklind
Copy link
Contributor Author

janniklind commented Dec 7, 2016

@jeffyoung @madhurig Thanks, I can built it now.

I have created most of the logic and can resign using only Signing Identities already installed on the build agent. However this doesn't work on hosted agents, where you need a p12 certificate. Hence I am missing the part that enables to install a p12 certificate into keychain and use that keychain when resigning.

XamariniOS has the logic for that part already and I will reuse some of it. However it uses a module named ios-signing-common. It is included using the line: import sign = require('ios-signing-common/ios-signing-common');

I have some problems when trying to use the functionality given in the module ios-signing-common - Can any of you provide some assistance on how to include that module within my task?

If you could provide an example on how to include the ios-signing-common module in the existing app-store-promote task, then it would be great:

Thanks,
Jannik

@janniklind
Copy link
Contributor Author

janniklind commented Dec 7, 2016

Hi again.

I manage to solve it myself. I haven't worked that much with typescript yet.

I have added vsts-tasks as a github dependency in package.json, since it isn't distributed as an npm module. I then added the following require path instead.

import sign = require('Agent.Tasks/Tasks/Common/ios-signing-common/ios-signing-common');

EDIT: This doesn't seems to work on a hosted MacinCloud build agent. It only works when building the vsix file. So I guess I need to find another way.

Is it posible that you can distribute vsts-tasks as an npm module in the npm registry, so we can use it as a normal dependency like the vsts-task-lib?

@janniklind
Copy link
Contributor Author

I have now pushed some code with the logic.
janniklind@20cb82e

Could you take a look and see what we can do to import the module properly?

Also it seems that the node makevsix.js --publishtest doesn't work. It fails on the token. I have to use the following instead in order to push the task to vsts:

tfx build tasks upload --task-path ./Tasks/ipa-resign --service-url https://<myproject>.visualstudio.com/DefaultCollection --auth-type pat --token qa2...cg

I have correctly set PUBLISH_ACCESSTOKEN to qa2...cg using:
export PUBLISH_ACCESSTOKEN=qa2...cg

@janniklind
Copy link
Contributor Author

janniklind commented Dec 9, 2016

Hi.

The task works now.

https://github.com/janniklind/app-store-vsts-extension/blob/0ad5748da482a742f7edaffee6bd33d183669c66/Tasks/ipa-resign/ipa-resign.ts

Vsix fails using node makevsix.js --maketest, it compiles the task correctly and I can publish the task and run it in vsts using tfx build tasks upload --task-path ./Tasks/ipa-resign --service-url https://<myproject>.visualstudio.com/DefaultCollection --auth-type pat --token qa2...cg but it gives me an error while packaging it all into the vsix extension.

error: Error: spawn /bin/sh EAGAIN

Also I am not a TypeScript expert so before I submit a pull request, it would be nice if you could have a quick look and tell if anything needs to be refactored.

Some of the things I consider to change:

  1. I reference the ios-signing-common and findfiles.legacy modules using a relative path into node_modules, since I couldn't make it work otherwise.

  2. findfiles.legacy is, as the name indicates, legacy code. I first tried using nuget-task-common/Utility, but it gave me an unreachable code compile error.

  3. Reference the vsts-tasks dependency in package.json with a specific git commit.

  4. I am also thinking of providing a way to specify a path to new Entitlements that should be used in the newly signed app. However, that is possible through the additional "Sigh resign arguments" already.

EDIT The error: Error: spawn /bin/sh EAGAIN error was do to the dependency to vsts-tasks, once I removed that, this error also disappeared.

@madhurig
Copy link
Contributor

madhurig commented Dec 9, 2016

@janniklind: The task implementation itself looks good. But I have concerns with taking vsts-tasks as a dependency. The repo has a lot of tasks and it was not really designed to be used as such. I understand you want to reuse the iOS signing logic from the common module but it is not setup to be used outside the tasks repo. I would suggest you copy the code into this repo even though it is not ideal.

  1. Don't add dependency on vsts-tasks, copy the ios signing logic that you need from the tasks. I will think about how we might want to make the ios signing logic reusable outside the vsts-tasks repo. We can switch over when it is ready.
  2. findFiles is legacy, you should use findMatch instead: https://github.com/Microsoft/vsts-task-lib/blob/master/node/task.ts#L1643
  3. Additional arguments is good but if you feel entitlements would be a common input, you can have it in the "Advanced section".

We appreciate your contributions.

Thanks,
Madhuri

@janniklind
Copy link
Contributor Author

@madhurig: Thanks for your feedback.

I have made the following and created a pull request:

  1. The dependency to vsts-tasks is now history.
  2. findMatch is used instead of findFiles.
    2.1 findMatch is used on every filePath input.
  3. It is now easier to specify entitlements, as it now has its own filePath input.

@janniklind
Copy link
Contributor Author

Bump

@jeffyoung
Copy link
Contributor

Hi Janni. Thanks for your submission.

Since you started working on this task, we updated the upstream repository with the build tools/framework from the Microsoft/vsts-task repository (the main repository for our build tasks). As a result, there are a few things that will happen once the code is merged:

  • building the extension VSIX has changed (no more makevsix.js)
  • tslint is run on the code
  • localization is required
  • a framework for adding tests now exists

That said, you started working on this before those changes went in. What I'd like to propose is that we merge your changes into the upstream repository, and we (not you) handle getting that code updated so that it builds with the new build system, tslint runs clean and localization is set up. Once that occurs, I'd like to know if you'd be interested in submitting some tests for the new task? If so, you could just sync your fork and add some tests. Otherwise, we would take a backlog item to add those tests ourselves.

What do you think?

@janniklind
Copy link
Contributor Author

@jeffyoung - It sounds fine to me. I have seen how the tests are applied in the vsts-tasks repository and I do like that approach.

When you are ready let me know and I will provide the tests.

@jeffyoung
Copy link
Contributor

Submitted #53 for review. Once it's merged, you can start adding the tests. (I did create an initial test that you can follow as a pattern.) You should also update the README.md file with instructions on how to use the IPA Resign task (just follow the pattern we have set in the current README.md file).

@jeffyoung
Copy link
Contributor

I made some final changes and merged this PR. @janniklind Feel free to review all of the code and update it as necessary (as the author of the task). Holiday vacations are starting so fewer and fewer folks will be around to review. Therefore you should be able to take your time (likely until the new year). Once the changes are reviewed and in, we should be able to get the extension updated relatively quickly.

@jeffyoung
Copy link
Contributor

The Ipa Resign task was just published to production in the 1.112.0 release of the extension. Any issues with the task should be opened in the Issues list.

@john1452
Copy link

Hi @jeffyoung I cannot see the ipa-resign task in lastest version of the extension -1.147.0 has this been removed?

@madhurig
Copy link
Contributor

@john1452 - The task is still available in the latest version of the extension. If you are still not seeing it, please open new issue and provide screenshots of what you notice and your Azure DevOps organization name.

Thanks,
Madhuri

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants