You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by jsoref <gi...@git.apache.org> on 2014/04/10 19:28:02 UTC
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
GitHub user jsoref opened a pull request:
https://github.com/apache/cordova-cli/pull/161
CB-6377 superspawn: always wrap non .exe calls to cmd with /s /c
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/blackberry/cordova-cli cb_6377
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-cli/pull/161.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #161
----
commit 93d8e60f6cc95fc172fae125eac9720e9adab5d5
Author: Josh Soref <js...@blackberry.com>
Date: 2014-04-10T17:15:51Z
CB-6377 superspawn: always wrap non .exe calls to cmd with /s /c
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by jengee <gi...@git.apache.org>.
Github user jengee commented on the pull request:
https://github.com/apache/cordova-cli/pull/161#issuecomment-40144272
This version works in WinXP. (Haven't checked Win7 yet ... but will)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by jsoref <gi...@git.apache.org>.
Github user jsoref commented on the pull request:
https://github.com/apache/cordova-cli/pull/161#issuecomment-40128762
I want QA to confirm this works before it merges...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by sgrebnov <gi...@git.apache.org>.
Github user sgrebnov commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/161#discussion_r11501013
--- Diff: src/superspawn.js ---
@@ -67,11 +69,11 @@ exports.spawn = function(cmd, args, opts) {
var spawnOpts = {};
var d = Q.defer();
- if (process.platform == 'win32') {
+ if (iswin32) {
cmd = resolveWindowsExe(cmd);
// If we couldn't find the file, likely we'll end up failing,
// but for things like "del", cmd will do the trick.
- if (!fs.existsSync(cmd)) {
+ if (wrapcmd && path.extname(cmd) != '.exe' || !fs.existsSync(cmd)) {
--- End diff --
@jsoref, why do we need this for .exe only and not for .cmd and other?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by sgrebnov <gi...@git.apache.org>.
Github user sgrebnov commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/161#discussion_r11559369
--- Diff: src/superspawn.js ---
@@ -67,14 +68,18 @@ exports.spawn = function(cmd, args, opts) {
var spawnOpts = {};
var d = Q.defer();
- if (process.platform == 'win32') {
+ if (iswin32) {
cmd = resolveWindowsExe(cmd);
// If we couldn't find the file, likely we'll end up failing,
// but for things like "del", cmd will do the trick.
- if (!fs.existsSync(cmd)) {
+ if (path.extname(cmd) != '.exe' && path.indexOf(' ') != -1) {
--- End diff --
I think it must be cmd.indexOf
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-cli/pull/161
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by sgrebnov <gi...@git.apache.org>.
Github user sgrebnov commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/161#discussion_r11533344
--- Diff: src/superspawn.js ---
@@ -24,6 +24,8 @@ var _ = require('underscore');
var Q = require('q');
var shell = require('shelljs');
var events = require('./events');
+var iswin32 = process.platform == 'win32';
+var wrapcmd = iswin32 && require('os').release().split('.')[0] < 6;
--- End diff --
No, cli does not work; this patch is required (I think it is super useful). Win8.1 x64.
I have the following hook file so cordova-cli is trying to run it during build. The file is located at project with **whitespace** in the name.
c:\temp\sql2 app\hooks\before_build\cordova-plugin-websql3.bat
**cordova-cli from npm(3.4.1-0.1.0)**
c:\temp\sql2 app>cordova build -d
Running command: "c:\temp\sql2 app\hooks\before_build\cordova-plugin-websql3.bat" "c:\temp\sql2 app"
'c:\temp\sql2' is not recognized as an internal or external command, operable program or batch file.
The error above is due to the following reason: cli shows command in output as escaped, but actually it is not. So if I escape it, I receive the following error - I believe this is because spawn can't run bat file directly.
c:\temp\sql2 app>cordova build -d
Running command: "c:\temp\sql2 app\hooks\before_build\cordova-plugin-websql3.bat" "c:\temp\sql2 app"
Command finished with error code ENOENT: c:\temp\sql2 app\hooks\before_build\cordova-plugin-websql3.bat c:\temp\sql2 app
Hook failed with error code ENOENT: c:\temp\sql2 app\hooks\before_build\cordova-
plugin-websql3.bat
**w/ path** - works the same since patch is not applied due to wrapcmd = False (seee below)
c:\temp\sql2 app>cordova build -d
Running command: "c:\temp\sql2 app\hooks\before_build\cordova-plugin-websql3.bat" "c:\temp\sql2 app"
'c:\temp\sql2' is not recognized as an internal or external command, operable program or batch file.
**w/ path and w/o check for require('os').release().split('.')[0] < 6** (manually removed)
c:\temp\sql2 app>cordova build -d
Running command: cmd "/s /c ""c:\temp\sql2 app\hooks\before_build\cordova-plugin-websql3.bat" "c:\temp\sql2 app"""
"SUCCESS"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by sgrebnov <gi...@git.apache.org>.
Github user sgrebnov commented on the pull request:
https://github.com/apache/cordova-cli/pull/161#issuecomment-40355369
LGTM + Tested.
I think we should accept the changes and then look on the following improvements:
1. The line below is very complex to read/understand. Move to separate fn + simplify + comments?
[['/s', '/c', '"'+[cmd].concat(args).map(function(a){if (/^[^"].* .*[^"]/.test(a)) return '"'+a+'"'; return a;}).join(" ")+'"'].join(" ")];
2. Not sure we should use two different cases - if there is white space and not. Single case is preferred since we don't need to test both cases.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by sgrebnov <gi...@git.apache.org>.
Github user sgrebnov commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/161#discussion_r11523494
--- Diff: src/superspawn.js ---
@@ -24,6 +24,8 @@ var _ = require('underscore');
var Q = require('q');
var shell = require('shelljs');
var events = require('./events');
+var iswin32 = process.platform == 'win32';
+var wrapcmd = iswin32 && require('os').release().split('.')[0] < 6;
--- End diff --
PS. If I manually set wrapcmd to True, everything works correct.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by sgrebnov <gi...@git.apache.org>.
Github user sgrebnov commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/161#discussion_r11523476
--- Diff: src/superspawn.js ---
@@ -24,6 +24,8 @@ var _ = require('underscore');
var Q = require('q');
var shell = require('shelljs');
var events = require('./events');
+var iswin32 = process.platform == 'win32';
+var wrapcmd = iswin32 && require('os').release().split('.')[0] < 6;
--- End diff --
I'm running Win8.1 and have the following os.release value: OS: 6.2.9200.
So wrapcmd is False.
But I can't still run non-exe files: Command finished with error code ENOENT:
So I think this should be applied to all win platforms, isn't it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by jsoref <gi...@git.apache.org>.
Github user jsoref commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/161#discussion_r11539545
--- Diff: src/superspawn.js ---
@@ -24,6 +24,8 @@ var _ = require('underscore');
var Q = require('q');
var shell = require('shelljs');
var events = require('./events');
+var iswin32 = process.platform == 'win32';
+var wrapcmd = iswin32 && require('os').release().split('.')[0] < 6;
--- End diff --
@martincgg: I was adding it because we _thought_ that there was something specific to **XP**.
I'd *rather* **not** have a specific code path for **XP** v. **7+**.
I've pushed a refreshed patch which now provides something better.
If @sgrebnov , @martincgg , @jengee could all test, that'd be great.
I'm actually on vacation today and next week, but I'd still like to get this settled ASAP.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by bryanhiggins <gi...@git.apache.org>.
Github user bryanhiggins commented on the pull request:
https://github.com/apache/cordova-cli/pull/161#issuecomment-40364475
@sgrebnov Thanks for testing. I've merged this in. Feel free to issue a pull request for your points which seem valid to me. Josh is on holiday until next week. I think it can wait until then.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by jsoref <gi...@git.apache.org>.
Github user jsoref commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/161#discussion_r11531129
--- Diff: src/superspawn.js ---
@@ -24,6 +24,8 @@ var _ = require('underscore');
var Q = require('q');
var shell = require('shelljs');
var events = require('./events');
+var iswin32 = process.platform == 'win32';
+var wrapcmd = iswin32 && require('os').release().split('.')[0] < 6;
--- End diff --
@sgrebnov does cli work for you without this patch?
Can you provide sample cmd/args/cwd?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by jsoref <gi...@git.apache.org>.
Github user jsoref commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/161#discussion_r11502427
--- Diff: src/superspawn.js ---
@@ -67,11 +69,11 @@ exports.spawn = function(cmd, args, opts) {
var spawnOpts = {};
var d = Q.defer();
- if (process.platform == 'win32') {
+ if (iswin32) {
cmd = resolveWindowsExe(cmd);
// If we couldn't find the file, likely we'll end up failing,
// but for things like "del", cmd will do the trick.
- if (!fs.existsSync(cmd)) {
+ if (wrapcmd && path.extname(cmd) != '.exe' || !fs.existsSync(cmd)) {
--- End diff --
@sgrebnov you misread it, we need this for everything that is `!=` `.exe`.
For `.exe`, **Windows** passes arguments directly to the executable, and the executable (or its runtime, e.g. `libc`/`msvcrt`) is responsible for parsing the string into its arguments array (if it believes in arguments arrays at all).
For `.bat`, or anything else, **Windows** won't actually launch them if you use the thing that `child_process.spawn` see: https://github.com/joyent/node/issues/2318 so, `child_process.spawn` has to run `cmd.exe` with the arguments. But `cmd.exe` has its own argument parsing which treats `"`s differently. In order to get `cmd.exe` to handle `"`s reasonably, you need to pass it `/s` which is what this block does (see the line below).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by martincgg <gi...@git.apache.org>.
Github user martincgg commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/161#discussion_r11537493
--- Diff: src/superspawn.js ---
@@ -24,6 +24,8 @@ var _ = require('underscore');
var Q = require('q');
var shell = require('shelljs');
var events = require('./events');
+var iswin32 = process.platform == 'win32';
+var wrapcmd = iswin32 && require('os').release().split('.')[0] < 6;
--- End diff --
What do you need to determine with require('os').release().split('.')[0] < 6, I mean what's the reason?
If you need to get Windows OS info you can use: wmic, this command-line tool is available in every Windows OS since Win XP.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by sgrebnov <gi...@git.apache.org>.
Github user sgrebnov commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/161#discussion_r11502884
--- Diff: src/superspawn.js ---
@@ -67,11 +69,11 @@ exports.spawn = function(cmd, args, opts) {
var spawnOpts = {};
var d = Q.defer();
- if (process.platform == 'win32') {
+ if (iswin32) {
cmd = resolveWindowsExe(cmd);
// If we couldn't find the file, likely we'll end up failing,
// but for things like "del", cmd will do the trick.
- if (!fs.existsSync(cmd)) {
+ if (wrapcmd && path.extname(cmd) != '.exe' || !fs.existsSync(cmd)) {
--- End diff --
Opps, I see, thx @jsoref !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-cli pull request: CB-6377 superspawn: always wrap non .exe...
Posted by sgrebnov <gi...@git.apache.org>.
Github user sgrebnov commented on the pull request:
https://github.com/apache/cordova-cli/pull/161#issuecomment-40365473
@bryanhiggins thx!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---