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

Replace ios-deploy with node-ios-device #488

Closed
wants to merge 2 commits into from

Conversation

reck1610
Copy link

@reck1610 reck1610 commented Jan 3, 2019

Closes #419, #420
Fixes #429

What does this PR do?

Replaces ios-deploy with node-ios-device.
Removes a global dependency.

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

* @param {String} appPath Path to application package
* @return {Promise} Resolves when deploy succeeds otherwise rejects
*/
function deployToDevice (appPath, target, extraArgs) {
Copy link
Member

Choose a reason for hiding this comment

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

What was extraArgs used for?

Copy link
Author

Choose a reason for hiding this comment

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

extraArgs contained the arguments not used by run.js.
It allowed to pass arguments to ios-deploy(ios-deploy usage).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so with your PR we loose this functionality. Which use case did it cover? What will not work any more for users that might be using this right now?

Copy link
Member

Choose a reason for hiding this comment

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

I think extraArgs is safe to ignore -- this would be any extra arguments to pass ios-deploy which we don't really officially support, but was "nice to have". If you do a cordova run --help you would see its the POPTS, which is not defined, thus we don't have any specific platform options.

We will have to remove any extraArgs code here as well:

var extraArgs = [];
if (runOptions.argv) {
// argv.slice(2) removes node and run.js, filterSupportedArgs removes the run.js args
extraArgs = module.exports.filterSupportedArgs(runOptions.argv.slice(2));
}
return module.exports.deployToDevice(appPath, runOptions.target, extraArgs);

@janpio

This comment has been minimized.

@codecov-io
Copy link

codecov-io commented Jan 3, 2019

Codecov Report

Merging #488 into master will decrease coverage by 0.38%.
The diff coverage is 12.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
- Coverage   75.66%   75.27%   -0.39%     
==========================================
  Files          11       11              
  Lines        1796     1792       -4     
==========================================
- Hits         1359     1349      -10     
- Misses        437      443       +6
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/build.js 58.04% <ø> (+0.33%) ⬆️
bin/templates/scripts/cordova/lib/check_reqs.js 51.42% <ø> (-1.28%) ⬇️
bin/templates/scripts/cordova/lib/versions.js 91.02% <100%> (+0.32%) ⬆️
bin/templates/scripts/cordova/lib/run.js 21.55% <6.66%> (-1.82%) ⬇️

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 fe01e5d...d8cd743. Read the comment docs.

@shazron
Copy link
Member

shazron commented Jan 9, 2019

Testing this right now.

@shazron
Copy link
Member

shazron commented Jan 9, 2019

Doesn't seem related, but I have an error with the below. Will debug:

[other build logs here]
** ARCHIVE SUCCEEDED **

Non-system Ruby in use. This may cause packaging to fail.
If you use RVM, please run `rvm use system`.
If you use chruby, please run `chruby system`.
Running command: xcodebuild -exportArchive -archivePath HelloCordova.xcarchive -exportOptionsPlist /Users/shaz/Desktop/foo/platforms/ios/exportOptions.plist -exportPath /Users/shaz/Desktop/foo/platforms/ios/build/device
2019-01-09 14:52:35.326 xcodebuild[97578:1313027] [MT] IDEDistribution: -[IDEDistributionLogging _createLoggingBundleAtPath:]: Created bundle at path '/var/folders/4h/x_0t05sd02x71168h34yh3hh0000gn/T/HelloCordova_2019-01-09_14-52-35.326.xcdistributionlogs'.
Exported HelloCordova to: /Users/shaz/Desktop/foo/platforms/ios/build/device
** EXPORT SUCCEEDED **

Running command: unzip -o -qq /Users/shaz/Desktop/foo/platforms/ios/build/device/HelloCordova.ipa
shell.js: internal error
Error: ENOTDIR: not a directory, rename '/Users/shaz/Desktop/foo/platforms/ios/build/device/Payload/HelloCordova.app' -> '/Users/shaz/Desktop/foo/platforms/ios/build/device/HelloCordova.app'
    at Object.fs.renameSync (fs.js:766:18)
    at /Users/shaz/Desktop/foo/platforms/ios/cordova/node_modules/shelljs/src/mv.js:77:8
    at Array.forEach (<anonymous>)
    at Object._mv (/Users/shaz/Desktop/foo/platforms/ios/cordova/node_modules/shelljs/src/mv.js:53:11)
    at Object.mv (/Users/shaz/Desktop/foo/platforms/ios/cordova/node_modules/shelljs/src/common.js:186:23)
    at /Users/shaz/Desktop/foo/platforms/ios/cordova/lib/run.js:91:31
    at _fulfilled (/Users/shaz/Desktop/foo/platforms/ios/cordova/node_modules/q/q.js:854:54)
    at self.promiseDispatch.done (/Users/shaz/Desktop/foo/platforms/ios/cordova/node_modules/q/q.js:883:30)
    at Promise.promise.promiseDispatch (/Users/shaz/Desktop/foo/platforms/ios/cordova/node_modules/q/q.js:816:13)
    at /Users/shaz/Desktop/foo/platforms/ios/cordova/node_modules/q/q.js:624:44

@shazron
Copy link
Member

shazron commented Jan 9, 2019

(UNRELATED) Ok, so it seems this line fails to delete a symlink:

shell.rm('-rf', appFile);
so I replaced it with require('fs').unlinkSync(appFile) and now the app can deploy.

However, even though the app deploys, it does not launch the app like ios-deploy. Thus, this will be a loss of a feature.

@shazron shazron mentioned this pull request Jan 9, 2019
@reck1610
Copy link
Author

reck1610 commented Jan 9, 2019

@shazron
I made a comment in the issue regarding this problem and a possible solution.
#429 (comment)

Copy link
Member

@shazron shazron left a comment

Choose a reason for hiding this comment

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

Even though it installs the app, it does not launch it -- so this is one loss of functionality. It was suggested ioslib could do this, but from what I can see it can only launch for simulator.

@erisu
Copy link
Member

erisu commented Oct 6, 2021

I will close out this ticket because:

  • It is outdated with conflicts
  • There has been no recent activity
  • The alternative solution had loss of functionality to launch the app, that was installed/deployed.

I still want to thank you for the effort you put into this.

@erisu erisu closed this Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use node-ios-device to deploy Why do we depend on a global ios-deploy?
5 participants