You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cordova.apache.org by GitBox <gi...@apache.org> on 2020/01/08 12:16:48 UTC
[GitHub] [cordova-ios] erisu opened a new pull request #763: refactor:
replace superspawn & child_process with execa
erisu opened a new pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763
### Motivation and Context
apache/cordova#16
### Description
Replace superspawn & child_process usage with execa
### Testing
- `npm t`
### Checklist
- [x] I've run the tests to see all new and existing tests pass
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364460739
##########
File path: bin/templates/scripts/cordova/lib/versions.js
##########
@@ -19,73 +19,55 @@
under the License.
*/
-const child_process = require('child_process');
const Q = require('q');
+const execa = require('execa');
const semver = require('semver');
-exports.get_apple_ios_version = () => {
- const d = Q.defer();
- child_process.exec('xcodebuild -showsdks', (error, stdout, stderr) => {
- if (error) {
- d.reject(stderr);
- } else {
- d.resolve(stdout);
- }
- });
-
- return d.promise.then(output => {
- const regex = /[0-9]*\.[0-9]*/;
- const versions = [];
- const regexIOS = /^iOS \d+/;
- output = output.split('\n');
- for (let i = 0; i < output.length; i++) {
- if (output[i].trim().match(regexIOS)) {
- versions[versions.length] = parseFloat(output[i].match(regex)[0]);
+function fetchSdkVersionByType (sdkType) {
+ return execa('xcodebuild', ['-showsdks'])
+ .then(
+ ({ stdout }) => stdout,
+ ({ stderr }) => stderr
+ )
+ .then(output => {
+ output = output.split('\n');
+
+ const versions = [];
+ const regexSdk = new RegExp(`^${sdkType} \\d`);
+
+ for (const line of output) {
+ const matched = line.trim();
+
+ if (matched.match(regexSdk)) {
+ versions.push(parseFloat(matched.match(/[0-9]*\.[0-9]*/)[0]));
+ }
}
- }
- versions.sort();
- console.log(versions[0]);
- return Q();
- }, stderr => Q.reject(stderr));
+
+ versions.sort();
+ console.log(versions[0]);
+ });
+}
+
+exports.get_apple_ios_version = () => {
+ return fetchSdkVersionByType('iOS');
};
exports.get_apple_osx_version = () => {
- const d = Q.defer();
- child_process.exec('xcodebuild -showsdks', (error, stdout, stderr) => {
- if (error) {
- d.reject(stderr);
- } else {
- d.resolve(stdout);
- }
- });
-
- return d.promise.then(output => {
- const regex = /[0-9]*\.[0-9]*/;
- const versions = [];
- const regexOSX = /^macOS \d+/;
- output = output.split('\n');
- for (let i = 0; i < output.length; i++) {
- if (output[i].trim().match(regexOSX)) {
- versions[versions.length] = parseFloat(output[i].match(regex)[0]);
- }
- }
- versions.sort();
- console.log(versions[0]);
- return Q();
- }, stderr => Q.reject(stderr));
+ return fetchSdkVersionByType('macOS');
};
exports.get_apple_xcode_version = () => {
- const d = Q.defer();
- child_process.exec('xcodebuild -version', (error, stdout, stderr) => {
- const versionMatch = /Xcode (.*)/.exec(stdout);
- if (error || !versionMatch) {
- d.reject(stderr);
- } else {
- d.resolve(versionMatch[1]);
- }
- });
- return d.promise;
+ return execa('xcodebuild', ['-version'])
+ .then(
+ ({ stdout }) => {
+ const versionMatch = /Xcode (.*)/.exec(stdout);
+
+ if (!versionMatch) Promise.reject(new Error('Missing version'));
+
+ return versionMatch[1];
+ },
+ ({ stderr }) => stderr
Review comment:
```suggestion
({ stderr }) => Promise.reject(stderr)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io commented on issue #763: refactor: replace
superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-572330808
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=h1) Report
> Merging [#763](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-ios/commit/266d339ee80d381068f50065dfc132132ffd2e7f?src=pr&el=desc) will **decrease** coverage by `0.08%`.
> The diff coverage is `51.72%`.
[](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #763 +/- ##
==========================================
- Coverage 74.42% 74.34% -0.09%
==========================================
Files 11 11
Lines 1830 1797 -33
==========================================
- Hits 1362 1336 -26
+ Misses 468 461 -7
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bin/templates/scripts/cordova/lib/run.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3J1bi5qcw==) | `22.12% <14.28%> (+0.69%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/Podfile.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL1BvZGZpbGUuanM=) | `75.11% <18.18%> (+1.53%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/build.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2J1aWxkLmpz) | `50.76% <33.33%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/versions.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ZlcnNpb25zLmpz) | `90.19% <83.33%> (-0.05%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=footer). Last update [266d339...63ca5ae](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] erisu commented on a change in pull request #763:
refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
erisu commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364500027
##########
File path: bin/templates/scripts/cordova/lib/list-started-emulators
##########
@@ -19,21 +19,33 @@
under the License.
*/
-var Q = require('q');
-var exec = require('child_process').exec;
+const execa = require('execa');
/**
* Gets list of running iOS simulators
* @return {Promise} Promise fulfilled with list of running iOS simulators
+ *
+ * @todo In the next PR, I will refactor this entire method.
+ *
+ * The process no longer contains the pattern "[i]OS Simulator".
+ * The process is now called "Simulator.app"
+ *
+ * Additionaly, `defaults read com.apple.iphonesimulator "SimulateDevice"` is also not valid aymore.
+ *
+ * I will replace this entire method to locate the active simulators though `simctl`
+ *
+ * Alternativly, remove this file. It is not documented in Cordova and not used anywhere in our code base.
*/
function listStartedEmulators () {
- // wrap exec call into promise
- return Q.nfcall(exec, 'ps aux | grep -i "[i]OS Simulator"')
- .then(function () {
- return Q.nfcall(exec, 'defaults read com.apple.iphonesimulator "SimulateDevice"');
- }).then(function (stdio) {
- return stdio[0].trim().split('\n');
- });
+ return execa('ps', ['aux'])
+ .then(({ stdout }) => {
+ if (stdout.match(/[i]OS Simulator/)) {
+ return execa('defaults', ['read', 'com.apple.iphonesimulator', '"SimulateDevice"']);
+ }
+
+ return { stdout: '' };
+ })
+ .then(({ stdout }) => stdout.trim().split('\n'));
Review comment:
```suggestion
.then(({ stdout }) => stdout.split('\n'));
```
Removing trim as requested even though this code doesn't work at all.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364462704
##########
File path: bin/templates/scripts/cordova/lib/versions.js
##########
@@ -19,73 +19,55 @@
under the License.
*/
-const child_process = require('child_process');
const Q = require('q');
+const execa = require('execa');
const semver = require('semver');
-exports.get_apple_ios_version = () => {
- const d = Q.defer();
- child_process.exec('xcodebuild -showsdks', (error, stdout, stderr) => {
- if (error) {
- d.reject(stderr);
- } else {
- d.resolve(stdout);
- }
- });
-
- return d.promise.then(output => {
- const regex = /[0-9]*\.[0-9]*/;
- const versions = [];
- const regexIOS = /^iOS \d+/;
- output = output.split('\n');
- for (let i = 0; i < output.length; i++) {
- if (output[i].trim().match(regexIOS)) {
- versions[versions.length] = parseFloat(output[i].match(regex)[0]);
+function fetchSdkVersionByType (sdkType) {
+ return execa('xcodebuild', ['-showsdks'])
+ .then(
+ ({ stdout }) => stdout,
+ ({ stderr }) => stderr
Review comment:
```suggestion
({ stderr }) => Promise.reject(stderr)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io edited a comment on issue #763: WIP:
refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #763: WIP: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-572330808
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=h1) Report
> Merging [#763](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-ios/commit/266d339ee80d381068f50065dfc132132ffd2e7f?src=pr&el=desc) will **decrease** coverage by `0.16%`.
> The diff coverage is `55.05%`.
[](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #763 +/- ##
==========================================
- Coverage 74.42% 74.26% -0.17%
==========================================
Files 11 11
Lines 1830 1791 -39
==========================================
- Hits 1362 1330 -32
+ Misses 468 461 -7
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bin/templates/scripts/cordova/Api.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvQXBpLmpz) | `71.22% <100%> (+0.1%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/run.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3J1bi5qcw==) | `21.42% <17.64%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/Podfile.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL1BvZGZpbGUuanM=) | `75.36% <27.27%> (+1.77%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/check\_reqs.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2NoZWNrX3JlcXMuanM=) | `51.94% <37.5%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/build.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2J1aWxkLmpz) | `50.51% <66.66%> (-0.26%)` | :arrow_down: |
| [bin/templates/scripts/cordova/lib/prepare.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ByZXBhcmUuanM=) | `84.06% <66.66%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/versions.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ZlcnNpb25zLmpz) | `87.5% <78.57%> (-2.75%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=footer). Last update [266d339...770e133](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io edited a comment on issue #763: refactor:
replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-572330808
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=h1) Report
> Merging [#763](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-ios/commit/266d339ee80d381068f50065dfc132132ffd2e7f?src=pr&el=desc) will **decrease** coverage by `0.08%`.
> The diff coverage is `51.72%`.
[](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #763 +/- ##
==========================================
- Coverage 74.42% 74.34% -0.09%
==========================================
Files 11 11
Lines 1830 1797 -33
==========================================
- Hits 1362 1336 -26
+ Misses 468 461 -7
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bin/templates/scripts/cordova/lib/run.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3J1bi5qcw==) | `22.12% <14.28%> (+0.69%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/Podfile.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL1BvZGZpbGUuanM=) | `75.11% <18.18%> (+1.53%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/build.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2J1aWxkLmpz) | `50.76% <33.33%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/versions.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ZlcnNpb25zLmpz) | `90.19% <83.33%> (-0.05%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=footer). Last update [266d339...63ca5ae](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364466859
##########
File path: bin/templates/scripts/cordova/lib/Podfile.js
##########
@@ -391,25 +389,14 @@ Podfile.prototype.install = function (requirementsCheckerFunction) {
return requirementsCheckerFunction()
.then(toolOptions => this.before_install(toolOptions))
.then(toolOptions => {
- if (toolOptions.ignore) {
- events.emit('verbose', '==== pod install start ====\n');
- events.emit('verbose', toolOptions.ignoreMessage);
- return Q.resolve();
- } else {
- return superspawn.spawn('pod', ['install', '--verbose'], opts)
- .progress(stdio => {
- if (stdio.stderr) { console.error(stdio.stderr); }
- if (stdio.stdout) {
- if (first) {
- events.emit('verbose', '==== pod install start ====\n');
- first = false;
- }
- events.emit('verbose', stdio.stdout);
- }
- });
- }
+ events.emit('verbose', '==== pod install start ====\n');
+
+ return toolOptions.ignore
+ ? Q.resolve(toolOptions.ignoreMessage)
Review comment:
For consistency we should remove non-Q in both cases I guess
```suggestion
? Promise.resolve(toolOptions.ignoreMessage)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] erisu commented on issue #763: WIP: refactor: replace
superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
erisu commented on issue #763: WIP: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-572799060
I will review everything again before answering.
It seems other changes will be needed anyways and more tests. I noticed that with the lack of tests it didn't catch something that would have become an issues after dependency changes.
Depending on timing, I am expecting maybe by late-Jan or early-Feb before I can answer and have all the fixes and new tests completed.
IF others need to push on with the major release, I recommend ignoring this PR for now and will target next next major.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364472177
##########
File path: bin/templates/scripts/cordova/lib/list-started-emulators
##########
@@ -19,21 +19,33 @@
under the License.
*/
-var Q = require('q');
-var exec = require('child_process').exec;
+const execa = require('execa');
/**
* Gets list of running iOS simulators
* @return {Promise} Promise fulfilled with list of running iOS simulators
+ *
+ * @todo In the next PR, I will refactor this entire method.
+ *
+ * The process no longer contains the pattern "[i]OS Simulator".
+ * The process is now called "Simulator.app"
+ *
+ * Additionaly, `defaults read com.apple.iphonesimulator "SimulateDevice"` is also not valid aymore.
+ *
+ * I will replace this entire method to locate the active simulators though `simctl`
+ *
+ * Alternativly, remove this file. It is not documented in Cordova and not used anywhere in our code base.
*/
function listStartedEmulators () {
- // wrap exec call into promise
- return Q.nfcall(exec, 'ps aux | grep -i "[i]OS Simulator"')
- .then(function () {
- return Q.nfcall(exec, 'defaults read com.apple.iphonesimulator "SimulateDevice"');
- }).then(function (stdio) {
- return stdio[0].trim().split('\n');
- });
+ return execa('ps', ['aux'])
Review comment:
you could use `execa.command` to keep using grep
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] erisu commented on a change in pull request #763:
refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
erisu commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364500973
##########
File path: bin/templates/scripts/cordova/lib/list-started-emulators
##########
@@ -19,21 +19,33 @@
under the License.
*/
-var Q = require('q');
-var exec = require('child_process').exec;
+const execa = require('execa');
/**
* Gets list of running iOS simulators
* @return {Promise} Promise fulfilled with list of running iOS simulators
+ *
+ * @todo In the next PR, I will refactor this entire method.
+ *
+ * The process no longer contains the pattern "[i]OS Simulator".
+ * The process is now called "Simulator.app"
+ *
+ * Additionaly, `defaults read com.apple.iphonesimulator "SimulateDevice"` is also not valid aymore.
+ *
+ * I will replace this entire method to locate the active simulators though `simctl`
+ *
+ * Alternativly, remove this file. It is not documented in Cordova and not used anywhere in our code base.
*/
function listStartedEmulators () {
- // wrap exec call into promise
- return Q.nfcall(exec, 'ps aux | grep -i "[i]OS Simulator"')
- .then(function () {
- return Q.nfcall(exec, 'defaults read com.apple.iphonesimulator "SimulateDevice"');
- }).then(function (stdio) {
- return stdio[0].trim().split('\n');
- });
+ return execa('ps', ['aux'])
Review comment:
I am OK leaving this as it is. This file should be deleted anyways IMO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364460234
##########
File path: bin/templates/scripts/cordova/lib/versions.js
##########
@@ -94,15 +76,11 @@ exports.get_apple_xcode_version = () => {
* or rejected in case of error
*/
exports.get_ios_deploy_version = () => {
- const d = Q.defer();
- child_process.exec('ios-deploy --version', (error, stdout, stderr) => {
- if (error) {
- d.reject(stderr);
- } else {
- d.resolve(stdout);
- }
- });
- return d.promise;
+ return execa('ios-deploy', ['--version'])
+ .then(
+ ({ stdout }) => stdout,
+ ({ stderr }) => stderr
Review comment:
```suggestion
({ stderr }) => Promise.reject(stderr)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io edited a comment on issue #763: refactor:
replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-572330808
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=h1) Report
> Merging [#763](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-ios/commit/266d339ee80d381068f50065dfc132132ffd2e7f?src=pr&el=desc) will **decrease** coverage by `0.09%`.
> The diff coverage is `49.01%`.
[](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #763 +/- ##
=========================================
- Coverage 74.42% 74.33% -0.1%
=========================================
Files 11 11
Lines 1830 1792 -38
=========================================
- Hits 1362 1332 -30
+ Misses 468 460 -8
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bin/templates/scripts/cordova/lib/run.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3J1bi5qcw==) | `22.12% <14.28%> (+0.69%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/Podfile.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL1BvZGZpbGUuanM=) | `75.48% <22.22%> (+1.89%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/build.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2J1aWxkLmpz) | `50.76% <33.33%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/versions.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ZlcnNpb25zLmpz) | `89.36% <80%> (-0.89%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=footer). Last update [266d339...20479f7](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364471791
##########
File path: bin/templates/scripts/cordova/lib/list-started-emulators
##########
@@ -19,21 +19,33 @@
under the License.
*/
-var Q = require('q');
-var exec = require('child_process').exec;
+const execa = require('execa');
/**
* Gets list of running iOS simulators
* @return {Promise} Promise fulfilled with list of running iOS simulators
+ *
+ * @todo In the next PR, I will refactor this entire method.
+ *
+ * The process no longer contains the pattern "[i]OS Simulator".
+ * The process is now called "Simulator.app"
+ *
+ * Additionaly, `defaults read com.apple.iphonesimulator "SimulateDevice"` is also not valid aymore.
+ *
+ * I will replace this entire method to locate the active simulators though `simctl`
+ *
+ * Alternativly, remove this file. It is not documented in Cordova and not used anywhere in our code base.
*/
function listStartedEmulators () {
- // wrap exec call into promise
- return Q.nfcall(exec, 'ps aux | grep -i "[i]OS Simulator"')
- .then(function () {
- return Q.nfcall(exec, 'defaults read com.apple.iphonesimulator "SimulateDevice"');
- }).then(function (stdio) {
- return stdio[0].trim().split('\n');
- });
+ return execa('ps', ['aux'])
+ .then(({ stdout }) => {
+ if (stdout.match(/[i]OS Simulator/)) {
+ return execa('defaults', ['read', 'com.apple.iphonesimulator', '"SimulateDevice"']);
+ }
+
+ return { stdout: '' };
+ })
+ .then(({ stdout }) => stdout.trim().split('\n'));
Review comment:
execa already trims
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364458436
##########
File path: tests/spec/unit/lib/list-devices.spec.js
##########
@@ -17,30 +17,24 @@
under the License.
*/
+const fs = require('fs-extra');
+const path = require('path');
+const execa = require('execa');
const list_devices = require('../../../../bin/templates/scripts/cordova/lib/list-devices');
-const Q = require('q');
+
+const sampleData = fs.readFileSync(path.resolve('tests/spec/unit/fixtures/sample-ioreg-output.txt'), 'utf-8');
describe('cordova/lib/list-devices', () => {
describe('run method', () => {
beforeEach(() => {
- spyOn(Q, 'all').and.returnValue(Q.resolve([]));
- spyOn(Q, 'nfcall');
- });
- it('should invoke proper system calls to retrieve connected devices', () => {
- return list_devices.run()
- .then(() => {
- expect(Q.nfcall).toHaveBeenCalledWith(jasmine.any(Function), jasmine.stringMatching(/ioreg.*iPad/g));
- expect(Q.nfcall).toHaveBeenCalledWith(jasmine.any(Function), jasmine.stringMatching(/ioreg.*iPod/g));
- expect(Q.nfcall).toHaveBeenCalledWith(jasmine.any(Function), jasmine.stringMatching(/ioreg.*iPhone/g));
Review comment:
Maybe keep one of those expectations?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] erisu commented on a change in pull request #763:
refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
erisu commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364500265
##########
File path: bin/create
##########
@@ -68,4 +68,4 @@ var options = {
require('./templates/scripts/cordova/loggingHelper').adjustLoggerLevel(argv);
-Api.createPlatform(projectPath, config, options).done();
+Api.createPlatform(projectPath, config, options);
Review comment:
```suggestion
Api.createPlatform(projectPath, config, options).catch(err => {
console.log(err);
process.exit(2);
});
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io edited a comment on issue #763: refactor:
replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-572330808
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=h1) Report
> Merging [#763](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-ios/commit/266d339ee80d381068f50065dfc132132ffd2e7f?src=pr&el=desc) will **decrease** coverage by `0.08%`.
> The diff coverage is `51.72%`.
[](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #763 +/- ##
==========================================
- Coverage 74.42% 74.34% -0.09%
==========================================
Files 11 11
Lines 1830 1797 -33
==========================================
- Hits 1362 1336 -26
+ Misses 468 461 -7
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bin/templates/scripts/cordova/lib/run.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3J1bi5qcw==) | `22.12% <14.28%> (+0.69%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/Podfile.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL1BvZGZpbGUuanM=) | `75.11% <18.18%> (+1.53%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/build.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2J1aWxkLmpz) | `50.76% <33.33%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/versions.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ZlcnNpb25zLmpz) | `90.19% <83.33%> (-0.05%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=footer). Last update [266d339...63ca5ae](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io edited a comment on issue #763: refactor:
superspawn usage
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #763: refactor: superspawn usage
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-572330808
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=h1) Report
> Merging [#763](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-ios/commit/3c418eb4e9bb5803dd5bc03facf92dccf75f779d?src=pr&el=desc) will **decrease** coverage by `0.21%`.
> The diff coverage is `57.64%`.
[](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #763 +/- ##
==========================================
- Coverage 74.55% 74.33% -0.22%
==========================================
Files 13 13
Lines 1835 1777 -58
==========================================
- Hits 1368 1321 -47
+ Misses 467 456 -11
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bin/templates/scripts/cordova/lib/versions.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ZlcnNpb25zLmpz) | `100% <100%> (+9.75%)` | :arrow_up: |
| [...emplates/scripts/cordova/lib/listEmulatorImages.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2xpc3RFbXVsYXRvckltYWdlcy5qcw==) | `100% <100%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/listDevices.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2xpc3REZXZpY2VzLmpz) | `100% <100%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/Api.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvQXBpLmpz) | `71.32% <100%> (-0.11%)` | :arrow_down: |
| [bin/templates/scripts/cordova/lib/Podfile.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL1BvZGZpbGUuanM=) | `73.8% <14.28%> (+0.22%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/run.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3J1bi5qcw==) | `21.1% <14.28%> (-0.33%)` | :arrow_down: |
| [bin/templates/scripts/cordova/lib/check\_reqs.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2NoZWNrX3JlcXMuanM=) | `51.94% <37.5%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/build.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2J1aWxkLmpz) | `50.51% <66.66%> (-0.26%)` | :arrow_down: |
| [bin/templates/scripts/cordova/lib/prepare.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ByZXBhcmUuanM=) | `83.74% <66.66%> (ø)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=footer). Last update [3c418eb...c4e5cd3](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] erisu edited a comment on issue #763: refactor:
superspawn usage
Posted by GitBox <gi...@apache.org>.
erisu edited a comment on issue #763: refactor: superspawn usage
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-586190108
@raphinesse PR has been finalized and ready to review.
* Everything will use `superspawn`.
* `cordova-ios` direct requirement for `Q` dependency has been removed.
* `Q.progress` will remain to be used and accessible by `superspawn`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on issue #763: WIP: refactor:
replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on issue #763: WIP: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-573432456
This PR should probably renamed to "drop Q" or something like it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364486538
##########
File path: bin/templates/scripts/cordova/lib/run.js
##########
@@ -199,21 +200,30 @@ function startSim (appPath, target) {
}
function iossimLaunch (appPath, devicetypeid, log, exit) {
- const f = path.resolve(path.dirname(require.resolve('ios-sim')), 'bin', 'ios-sim');
- const params = ['launch', appPath, '--devicetypeid', devicetypeid, '--log', log, exit];
+ const subprocess = execa(
+ require.resolve('ios-sim/bin/ios-sim'),
+ ['launch', appPath, '--devicetypeid', devicetypeid, '--log', log, exit],
+ { cwd: projectPath }
+ );
- return superspawn.spawn(f, params, { cwd: projectPath, printCommand: true })
- .progress(stdio => {
- if (stdio.stderr) {
- events.emit('error', `[ios-sim] ${stdio.stderr}`);
- }
- if (stdio.stdout) {
- events.emit('log', `[ios-sim] ${stdio.stdout.trim()}`);
- }
- })
- .then(result => {
- events.emit('log', 'Simulator successfully started via `ios-sim`.');
- });
+ const transformOutput = (channel, line) => {
+ events.emit(channel, `[ios-sim] ${line}`);
+ };
+
+ subprocess.stdout
+ .pipe(split2())
+ .on('data', transformOutput.bind(this, 'log'));
+
+ subprocess.stderr
+ .pipe(split2())
+ .on('data', transformOutput.bind(this, 'error'));
+
+ return (async () => {
+ const { stderr } = await subprocess;
+ const hasError = !!stderr;
+
+ events.emit('log', `Simulator ${hasError ? 'had an error while starting' : 'successfully started'} via "ios-sim".`);
+ })();
Review comment:
Why not leave it at the equivalent of before?
```js
subprocess.then(() => {
events.emit('log', 'Simulator successfully started via `ios-sim`.');
});
```
Can we really be sure we encountered an error when `stderr` is non-empty?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364472711
##########
File path: bin/templates/scripts/cordova/lib/list-started-emulators
##########
@@ -19,21 +19,33 @@
under the License.
*/
-var Q = require('q');
-var exec = require('child_process').exec;
+const execa = require('execa');
/**
* Gets list of running iOS simulators
* @return {Promise} Promise fulfilled with list of running iOS simulators
+ *
+ * @todo In the next PR, I will refactor this entire method.
+ *
+ * The process no longer contains the pattern "[i]OS Simulator".
+ * The process is now called "Simulator.app"
+ *
+ * Additionaly, `defaults read com.apple.iphonesimulator "SimulateDevice"` is also not valid aymore.
+ *
+ * I will replace this entire method to locate the active simulators though `simctl`
+ *
+ * Alternativly, remove this file. It is not documented in Cordova and not used anywhere in our code base.
Review comment:
I think we should definitely evaluate the bins we do not use ourselves and get rid of anything non-essential
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364470372
##########
File path: bin/create
##########
@@ -68,4 +68,4 @@ var options = {
require('./templates/scripts/cordova/loggingHelper').adjustLoggerLevel(argv);
-Api.createPlatform(projectPath, config, options).done();
+Api.createPlatform(projectPath, config, options);
Review comment:
On error, the old code causes an uncaught exception, the new one an unhandled rejection.
I would suggest to fix the code, maybe even in a preceding PR. Adding this should suffice and would be in line with the other binaries in this repo:
```js
.catch(err => {
console.log(err);
process.exit(2);
});
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364460018
##########
File path: bin/templates/scripts/cordova/lib/versions.js
##########
@@ -111,15 +89,11 @@ exports.get_ios_deploy_version = () => {
* or rejected in case of error
*/
exports.get_cocoapods_version = () => {
- const d = Q.defer();
- child_process.exec('pod --version', (error, stdout, stderr) => {
- if (error) {
- d.reject(stderr);
- } else {
- d.resolve(stdout);
- }
- });
- return d.promise;
+ return execa('pod', ['--version'])
+ .then(
+ ({ stdout }) => stdout,
+ ({ stderr }) => stderr
Review comment:
```suggestion
({ stderr }) => Promise.reject(stderr)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] codecov-io edited a comment on issue #763: WIP:
refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #763: WIP: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-572330808
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=h1) Report
> Merging [#763](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-ios/commit/266d339ee80d381068f50065dfc132132ffd2e7f?src=pr&el=desc) will **decrease** coverage by `0.16%`.
> The diff coverage is `55.05%`.
[](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #763 +/- ##
==========================================
- Coverage 74.42% 74.26% -0.17%
==========================================
Files 11 11
Lines 1830 1791 -39
==========================================
- Hits 1362 1330 -32
+ Misses 468 461 -7
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bin/templates/scripts/cordova/Api.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvQXBpLmpz) | `71.22% <100%> (+0.1%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/run.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3J1bi5qcw==) | `21.42% <17.64%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/Podfile.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL1BvZGZpbGUuanM=) | `75.36% <27.27%> (+1.77%)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/check\_reqs.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2NoZWNrX3JlcXMuanM=) | `51.94% <37.5%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/build.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2J1aWxkLmpz) | `50.51% <66.66%> (-0.26%)` | :arrow_down: |
| [bin/templates/scripts/cordova/lib/prepare.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ByZXBhcmUuanM=) | `84.06% <66.66%> (ø)` | :arrow_up: |
| [bin/templates/scripts/cordova/lib/versions.js](https://codecov.io/gh/apache/cordova-ios/pull/763/diff?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ZlcnNpb25zLmpz) | `87.5% <78.57%> (-2.75%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=footer). Last update [266d339...770e133](https://codecov.io/gh/apache/cordova-ios/pull/763?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364457612
##########
File path: tests/spec/unit/lib/list-devices.spec.js
##########
@@ -17,30 +17,24 @@
under the License.
*/
+const fs = require('fs-extra');
+const path = require('path');
+const execa = require('execa');
const list_devices = require('../../../../bin/templates/scripts/cordova/lib/list-devices');
-const Q = require('q');
+
+const sampleData = fs.readFileSync(path.resolve('tests/spec/unit/fixtures/sample-ioreg-output.txt'), 'utf-8');
Review comment:
I think the path to the fixtures should be resolved relative to `__dirname` to work correctly independent from the current working dir.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] erisu commented on a change in pull request #763:
refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
erisu commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364499259
##########
File path: bin/templates/scripts/cordova/lib/versions.js
##########
@@ -19,73 +19,55 @@
under the License.
*/
-const child_process = require('child_process');
const Q = require('q');
+const execa = require('execa');
const semver = require('semver');
-exports.get_apple_ios_version = () => {
- const d = Q.defer();
- child_process.exec('xcodebuild -showsdks', (error, stdout, stderr) => {
- if (error) {
- d.reject(stderr);
- } else {
- d.resolve(stdout);
- }
- });
-
- return d.promise.then(output => {
- const regex = /[0-9]*\.[0-9]*/;
- const versions = [];
- const regexIOS = /^iOS \d+/;
- output = output.split('\n');
- for (let i = 0; i < output.length; i++) {
- if (output[i].trim().match(regexIOS)) {
- versions[versions.length] = parseFloat(output[i].match(regex)[0]);
+function fetchSdkVersionByType (sdkType) {
+ return execa('xcodebuild', ['-showsdks'])
+ .then(
+ ({ stdout }) => stdout,
+ ({ stderr }) => stderr
+ )
+ .then(output => {
+ output = output.split('\n');
+
+ const versions = [];
+ const regexSdk = new RegExp(`^${sdkType} \\d`);
+
+ for (const line of output) {
+ const matched = line.trim();
+
+ if (matched.match(regexSdk)) {
+ versions.push(parseFloat(matched.match(/[0-9]*\.[0-9]*/)[0]));
+ }
}
- }
- versions.sort();
- console.log(versions[0]);
- return Q();
- }, stderr => Q.reject(stderr));
+
+ versions.sort();
+ console.log(versions[0]);
+ });
+}
+
+exports.get_apple_ios_version = () => {
+ return fetchSdkVersionByType('iOS');
};
exports.get_apple_osx_version = () => {
- const d = Q.defer();
- child_process.exec('xcodebuild -showsdks', (error, stdout, stderr) => {
- if (error) {
- d.reject(stderr);
- } else {
- d.resolve(stdout);
- }
- });
-
- return d.promise.then(output => {
- const regex = /[0-9]*\.[0-9]*/;
- const versions = [];
- const regexOSX = /^macOS \d+/;
- output = output.split('\n');
- for (let i = 0; i < output.length; i++) {
- if (output[i].trim().match(regexOSX)) {
- versions[versions.length] = parseFloat(output[i].match(regex)[0]);
- }
- }
- versions.sort();
- console.log(versions[0]);
- return Q();
- }, stderr => Q.reject(stderr));
+ return fetchSdkVersionByType('macOS');
};
exports.get_apple_xcode_version = () => {
- const d = Q.defer();
- child_process.exec('xcodebuild -version', (error, stdout, stderr) => {
- const versionMatch = /Xcode (.*)/.exec(stdout);
- if (error || !versionMatch) {
- d.reject(stderr);
- } else {
- d.resolve(versionMatch[1]);
- }
- });
- return d.promise;
+ return execa('xcodebuild', ['-version'])
+ .then(
+ ({ stdout }) => {
Review comment:
```suggestion
({ stdout, stderr }) => {
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] erisu commented on issue #763: refactor: superspawn
usage
Posted by GitBox <gi...@apache.org>.
erisu commented on issue #763: refactor: superspawn usage
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-586190108
@raphinesse PR has been finalized and ready to review.
* Everything will use superspawn.
* Cordova iOS direct `Q` dependency has been removed.
* `Q.progress` will remain with `superspawn`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364468480
##########
File path: bin/templates/scripts/cordova/lib/Podfile.js
##########
@@ -391,25 +389,14 @@ Podfile.prototype.install = function (requirementsCheckerFunction) {
return requirementsCheckerFunction()
.then(toolOptions => this.before_install(toolOptions))
.then(toolOptions => {
- if (toolOptions.ignore) {
- events.emit('verbose', '==== pod install start ====\n');
- events.emit('verbose', toolOptions.ignoreMessage);
- return Q.resolve();
- } else {
- return superspawn.spawn('pod', ['install', '--verbose'], opts)
- .progress(stdio => {
- if (stdio.stderr) { console.error(stdio.stderr); }
- if (stdio.stdout) {
- if (first) {
- events.emit('verbose', '==== pod install start ====\n');
- first = false;
- }
- events.emit('verbose', stdio.stdout);
- }
- });
- }
+ events.emit('verbose', '==== pod install start ====\n');
+
+ return toolOptions.ignore
+ ? Q.resolve(toolOptions.ignoreMessage)
+ : execa('pod', ['install', '--verbose'], opts).then(({ stdout }) => stdout);
Review comment:
The previous implementation streamed the `pod install` output. The new one buffers `stdout` and drops `stderr`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on issue #763: refactor: replace
superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on issue #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#issuecomment-572566185
With the commits I just pushed we have resolved everything except for these two remarks I left before:
> Command printing was dropped without replacement and without a note in the PR description. I think we need either one.
>
> Do we need to transform `execa` Errors to a format compatible to the old one (`child_process` or `superspawn`)? Did you check if there is any error handling that relies on special error properties or anything like that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364488623
##########
File path: bin/templates/scripts/cordova/lib/list-devices
##########
@@ -19,38 +19,39 @@
under the License.
*/
-var Q = require('q');
-var exec = require('child_process').exec;
+const execa = require('execa');
/**
* Gets list of connected iOS devices
* @return {Promise} Promise fulfilled with list of available iOS devices
*/
function listDevices () {
- var commands = [
- Q.nfcall(exec, "ioreg -p IOUSB -l | sed -n -e '/iPad/,/USB Serial Number/p' | grep 'Serial Number' | awk -F\\\" '{print $4 \" iPad\"}'"),
- Q.nfcall(exec, "ioreg -p IOUSB -l | sed -n -e '/iPhone/,/USB Serial Number/p' | grep 'Serial Number' | awk -F\\\" '{print $4 \" iPhone\"}'"),
- Q.nfcall(exec, "ioreg -p IOUSB -l | sed -n -e '/iPod/,/USB Serial Number/p' | grep 'Serial Number' | awk -F\\\" '{print $4 \" iPod\"}'")
- ];
+ return execa.command('ioreg -p IOUSB -l')
+ .then(({ stdout }) => {
+ const deviceTypes = ['iPhone', 'iPad', 'iPod'];
+ const detectedDevices = [];
+ let targetDeviceType = null;
- // wrap al lexec calls into promises and wait until they're fullfilled
- return Q.all(commands).then(function (results) {
- var accumulator = [];
- results.forEach(function (result) {
- var devicefound;
- // Each command promise resolves with array [stout, stderr], and we need stdout only
- // Append stdout lines to accumulator
- devicefound = result[0].trim().split('\n');
- if (devicefound && devicefound.length) {
- devicefound.forEach(function (device) {
- if (device) {
- accumulator.push(device);
+ stdout.split('\n').forEach(line => {
+ if (!targetDeviceType) {
+ const detectedDevice = deviceTypes.filter(deviceType => line.includes(`-o ${deviceType}`));
+
+ if (detectedDevice.length) {
+ targetDeviceType = detectedDevice[0];
+ }
+ } else if (targetDeviceType && line.includes('USB Serial Number')) {
+ const reuslt = line.match(/"USB Serial Number" = "(.*)"/);
+
+ if (reuslt && !detectedDevices.includes(reuslt[1])) {
Review comment:
The second part of the condition will always be false, right? Because `detectedDevices` includes entries of the form `${reuslt[1]} ${targetDeviceType}`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364470458
##########
File path: bin/update
##########
@@ -34,4 +34,4 @@ if (args.help || args.argv.remain.length === 0) {
require('./templates/scripts/cordova/loggingHelper').adjustLoggerLevel(args);
-Api.updatePlatform(args.argv.remain[0], { link: (args.link || args.shared) }).done();
+Api.updatePlatform(args.argv.remain[0], { link: (args.link || args.shared) });
Review comment:
On error, the old code causes an uncaught exception, the new one an unhandled rejection.
I would suggest to fix the code, maybe even in a preceding PR. Adding this should suffice and would be in line with the other binaries in this repo:
```js
.catch(err => {
console.log(err);
process.exit(2);
});
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364461513
##########
File path: bin/templates/scripts/cordova/lib/versions.js
##########
@@ -19,73 +19,55 @@
under the License.
*/
-const child_process = require('child_process');
const Q = require('q');
+const execa = require('execa');
const semver = require('semver');
-exports.get_apple_ios_version = () => {
- const d = Q.defer();
- child_process.exec('xcodebuild -showsdks', (error, stdout, stderr) => {
- if (error) {
- d.reject(stderr);
- } else {
- d.resolve(stdout);
- }
- });
-
- return d.promise.then(output => {
- const regex = /[0-9]*\.[0-9]*/;
- const versions = [];
- const regexIOS = /^iOS \d+/;
- output = output.split('\n');
- for (let i = 0; i < output.length; i++) {
- if (output[i].trim().match(regexIOS)) {
- versions[versions.length] = parseFloat(output[i].match(regex)[0]);
+function fetchSdkVersionByType (sdkType) {
+ return execa('xcodebuild', ['-showsdks'])
+ .then(
+ ({ stdout }) => stdout,
+ ({ stderr }) => stderr
+ )
+ .then(output => {
+ output = output.split('\n');
+
+ const versions = [];
+ const regexSdk = new RegExp(`^${sdkType} \\d`);
+
+ for (const line of output) {
+ const matched = line.trim();
+
+ if (matched.match(regexSdk)) {
+ versions.push(parseFloat(matched.match(/[0-9]*\.[0-9]*/)[0]));
+ }
}
- }
- versions.sort();
- console.log(versions[0]);
- return Q();
- }, stderr => Q.reject(stderr));
+
+ versions.sort();
+ console.log(versions[0]);
+ });
+}
+
+exports.get_apple_ios_version = () => {
+ return fetchSdkVersionByType('iOS');
};
exports.get_apple_osx_version = () => {
- const d = Q.defer();
- child_process.exec('xcodebuild -showsdks', (error, stdout, stderr) => {
- if (error) {
- d.reject(stderr);
- } else {
- d.resolve(stdout);
- }
- });
-
- return d.promise.then(output => {
- const regex = /[0-9]*\.[0-9]*/;
- const versions = [];
- const regexOSX = /^macOS \d+/;
- output = output.split('\n');
- for (let i = 0; i < output.length; i++) {
- if (output[i].trim().match(regexOSX)) {
- versions[versions.length] = parseFloat(output[i].match(regex)[0]);
- }
- }
- versions.sort();
- console.log(versions[0]);
- return Q();
- }, stderr => Q.reject(stderr));
+ return fetchSdkVersionByType('macOS');
};
exports.get_apple_xcode_version = () => {
- const d = Q.defer();
- child_process.exec('xcodebuild -version', (error, stdout, stderr) => {
- const versionMatch = /Xcode (.*)/.exec(stdout);
- if (error || !versionMatch) {
- d.reject(stderr);
- } else {
- d.resolve(versionMatch[1]);
- }
- });
- return d.promise;
+ return execa('xcodebuild', ['-version'])
+ .then(
+ ({ stdout }) => {
+ const versionMatch = /Xcode (.*)/.exec(stdout);
+
+ if (!versionMatch) Promise.reject(new Error('Missing version'));
Review comment:
The equivalent of the old code would be:
```suggestion
if (!versionMatch) return Promise.reject(stderr);
```
Any reason to change the behavior here? Your code seems to be missing a `return` as well
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364460086
##########
File path: bin/templates/scripts/cordova/lib/versions.js
##########
@@ -128,15 +102,11 @@ exports.get_cocoapods_version = () => {
* or rejected in case of error
*/
exports.get_ios_sim_version = () => {
- const d = Q.defer();
- child_process.exec('ios-sim --version', (error, stdout, stderr) => {
- if (error) {
- d.reject(stderr);
- } else {
- d.resolve(stdout);
- }
- });
- return d.promise;
+ return execa('ios-sim', ['--version'])
+ .then(
+ ({ stdout }) => stdout,
+ ({ stderr }) => stderr
Review comment:
```suggestion
({ stderr }) => Promise.reject(stderr)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] erisu commented on a change in pull request #763:
refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
erisu commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364499432
##########
File path: bin/update
##########
@@ -34,4 +34,4 @@ if (args.help || args.argv.remain.length === 0) {
require('./templates/scripts/cordova/loggingHelper').adjustLoggerLevel(args);
-Api.updatePlatform(args.argv.remain[0], { link: (args.link || args.shared) }).done();
+Api.updatePlatform(args.argv.remain[0], { link: (args.link || args.shared) });
Review comment:
```suggestion
Api.updatePlatform(args.argv.remain[0], { link: (args.link || args.shared) }).catch(err => {
console.log(err);
process.exit(2);
});
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-ios] raphinesse commented on a change in pull request
#763: refactor: replace superspawn & child_process with execa
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #763: refactor: replace superspawn & child_process with execa
URL: https://github.com/apache/cordova-ios/pull/763#discussion_r364487087
##########
File path: bin/templates/scripts/cordova/lib/list-devices
##########
@@ -19,38 +19,39 @@
under the License.
*/
-var Q = require('q');
-var exec = require('child_process').exec;
+const execa = require('execa');
/**
* Gets list of connected iOS devices
* @return {Promise} Promise fulfilled with list of available iOS devices
*/
function listDevices () {
- var commands = [
- Q.nfcall(exec, "ioreg -p IOUSB -l | sed -n -e '/iPad/,/USB Serial Number/p' | grep 'Serial Number' | awk -F\\\" '{print $4 \" iPad\"}'"),
- Q.nfcall(exec, "ioreg -p IOUSB -l | sed -n -e '/iPhone/,/USB Serial Number/p' | grep 'Serial Number' | awk -F\\\" '{print $4 \" iPhone\"}'"),
- Q.nfcall(exec, "ioreg -p IOUSB -l | sed -n -e '/iPod/,/USB Serial Number/p' | grep 'Serial Number' | awk -F\\\" '{print $4 \" iPod\"}'")
- ];
+ return execa.command('ioreg -p IOUSB -l')
+ .then(({ stdout }) => {
+ const deviceTypes = ['iPhone', 'iPad', 'iPod'];
+ const detectedDevices = [];
+ let targetDeviceType = null;
- // wrap al lexec calls into promises and wait until they're fullfilled
- return Q.all(commands).then(function (results) {
- var accumulator = [];
- results.forEach(function (result) {
- var devicefound;
- // Each command promise resolves with array [stout, stderr], and we need stdout only
- // Append stdout lines to accumulator
- devicefound = result[0].trim().split('\n');
- if (devicefound && devicefound.length) {
- devicefound.forEach(function (device) {
- if (device) {
- accumulator.push(device);
+ stdout.split('\n').forEach(line => {
+ if (!targetDeviceType) {
+ const detectedDevice = deviceTypes.filter(deviceType => line.includes(`-o ${deviceType}`));
+
+ if (detectedDevice.length) {
+ targetDeviceType = detectedDevice[0];
+ }
+ } else if (targetDeviceType && line.includes('USB Serial Number')) {
+ const reuslt = line.match(/"USB Serial Number" = "(.*)"/);
Review comment:
Typo `reuslt`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org