Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

a couple tweaks to make things work on windows #5

Merged
merged 10 commits into from
Dec 27, 2015
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
node_modules
53 changes: 32 additions & 21 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,18 @@ var rimraf = require('rimraf')
var path = require('path')
var signalExit = require('signal-exit')
var homedir = require('os-homedir')() + '/.node-spawn-wrap-'
var winRebase = require('./lib/win-rebase')

var pieces = process.execPath.split(path.sep)
var cmdname = pieces[pieces.length - 1]

var shim = '#!' + process.execPath + '\n' +
fs.readFileSync(__dirname + '/shim.js')

var cmdShim = 'SETLOCAL\r\n' +
'SET PATHEXT=%PATHEXT:;.JS;=;%\r\n' +
process.execPath + ' "%~dp0\\.\\node" %*\r\n'
var isWindows = require('./lib/is-windows')()

var isWindows = false
var pathRe = /^PATH=/
if (process.platform === 'win32' ||
process.env.OSTYPE === 'cygwin' ||
process.env.OSTYPE === 'msys') {
pathRe = /^PATH=/i
isWindows = true
}
if (isWindows) pathRe = /^PATH=/i

var colon = isWindows ? ';' : ':'

Expand Down Expand Up @@ -80,19 +73,12 @@ function wrap (argv, env, workingDir) {
}
} else if (isWindows && (
file === path.basename(process.env.comspec) ||
file === 'cmd.exe')) {
file === 'cmd.exe' ||
file === 'cmd'
)) {
cmdi = options.args.indexOf('/c')
if (cmdi !== -1) {
c = options.args[cmdi + 1]
re = new RegExp('^\\s*"([^\\s]*(?:node|iojs)) ')
match = c.match(re)
if (match) {
exe = path.basename(match[1]).replace(/\.exe$/, '')
if (exe === 'node' || exe === 'iojs' || exe === cmdname) {
c = c.replace(re, exe + ' ')
options.args[cmdi + 1] = c
}
}
options.args[cmdi + 1] = winRebase(options.args[cmdi + 1], workingDir + '/node.cmd')
}
} else if (file === 'node' || file === 'iojs' || cmdname === file) {
// make sure it has a main script.
Expand Down Expand Up @@ -144,12 +130,31 @@ function wrap (argv, env, workingDir) {
options.envPairs.push((isWindows ? 'Path=' : 'PATH=') + workingDir)
}

if (isWindows) fixWindowsBins(workingDir, options)

return spawn.call(this, options)
}

return unwrap
}

// by default Windows will reference the full
// path to the node.exe or iojs.exe as the bin,
// we should instead point spawn() at our .cmd shim.
function fixWindowsBins (workingDir, options) {
var renode = new RegExp('.*node\\.exe$')
var reiojs = new RegExp('.*iojs\\.exe$')

options.file = options.file.replace(renode, workingDir + '/node.cmd')
options.file = options.file.replace(reiojs, workingDir + '/node.cmd')

options.args = options.args.map(function (a) {
a = a.replace(renode, workingDir + '/node.cmd')
a = a.replace(reiojs, workingDir + '/node.cmd')
return a
})
}

function setup (argv, env) {
if (argv && typeof argv === 'object' && !env && !Array.isArray(argv)) {
env = argv
Expand Down Expand Up @@ -215,6 +220,12 @@ function setup (argv, env) {
mkdirp.sync(workingDir)
workingDir = fs.realpathSync(workingDir)
if (isWindows) {
var cmdShim =
'@echo off\r\n' +
'SETLOCAL\r\n' +
'SET PATHEXT=%PATHEXT:;.JS;=;%\r\n' +
'"' + process.execPath + '"' + ' "%~dp0\\.\\node" %*\r\n'

fs.writeFileSync(workingDir + '/node.cmd', cmdShim)
fs.chmodSync(workingDir + '/node.cmd', '0755')
fs.writeFileSync(workingDir + '/iojs.cmd', cmdShim)
Expand Down
5 changes: 5 additions & 0 deletions lib/is-windows.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = function () {
return process.platform === 'win32' ||
process.env.OSTYPE === 'cygwin' ||
Copy link

Choose a reason for hiding this comment

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

Does cygwin actually change process.platform to not be 'win32'?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rmg I believe I cargo culted this from somwhere, it might have been that I pulled the logic originally sitting in index.js to is-windows.js -- my guess would be there's a good reason for the cygwin check, it could also be that someone accidentally created a cult and I've been indoctrinated into it.

process.env.OSTYPE === 'msys'
}
7 changes: 7 additions & 0 deletions lib/win-rebase.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
var re = new RegExp(/(.*((node\.exe($| ))|(node($| ))|(iojs($| ))|(iojs\.exe($| )))) ?(.*)$/)

module.exports = function (path, rebase) {
var m = path.match(re)
Copy link

Choose a reason for hiding this comment

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

I generally would use re.exec(path) here instead, because it is safer if path is not a string - though I admit that "safer" may not actually be safer in all cases.

I'm curious, is there a specific reason for this form? Or is it just what happened to flow from your fingers while typing it up? No wrong answer, I'm honestly curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rmg this is just what fell out of my fingers, no good reason -- I find I flip flop a lot on how I build up regexes.

if (!m) return path
return path.replace(m[1].trim(), rebase)
}
14 changes: 7 additions & 7 deletions test/basic.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
var sw = require('../')
var onExit = require('signal-exit')

var cp = require('child_process')
var spawn = require('child_process').spawn
var fixture = require.resolve('./fixtures/script.js')
var fs = require('fs')
var path = require('path')
Expand Down Expand Up @@ -38,7 +38,7 @@ var expect = 'WRAP ["{{FIXTURE}}","xyz"]\n' +
'EXIT [0,null]\n'

t.test('spawn execPath', function (t) {
var child = cp.spawn(process.execPath, [fixture, 'xyz'])
var child = spawn(process.execPath, [fixture, 'xyz'])

var out = ''
child.stdout.on('data', function (c) {
Expand All @@ -53,7 +53,7 @@ t.test('spawn execPath', function (t) {
})

t.test('exec shebang', function (t) {
var child = cp.exec(fixture + ' xyz')
var child = spawn(fixture, ['xyz'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should still be cp.exec.

The failures on Linux can probably be fixed with this diff:

diff --git a/test/fixtures/script.js b/test/fixtures/script.js
index 51f518a..ebc08f9 100755
--- a/test/fixtures/script.js
+++ b/test/fixtures/script.js
@@ -1,3 +1,5 @@
 #!/usr/bin/env node
 console.log('%j', process.execArgv)
 console.log('%j', process.argv.slice(2))
+// keep the process open long enough to catch a signal
+setTimeout(function() {}, 500)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, that's not it.

Definitely something weird happening with signal-exit, it's sending the signal, but the exit happens with status 0, not the signal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, apparently signal-exit just straight up does not work on Linux. This is strange.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See tapjs/signal-exit#15. It works fine, but exec() uses a real /bin/sh on Linux, so a bunch of the tests gets weird results.


var out = ''
child.stdout.on('data', function (c) {
Expand All @@ -68,7 +68,7 @@ t.test('exec shebang', function (t) {
})

t.test('SIGHUP', function (t) {
var child = cp.exec(fixture + ' xyz')
var child = spawn(fixture, ['xyz'])

var out = ''
child.stdout.on('data', function (c) {
Expand All @@ -88,7 +88,7 @@ t.test('SIGHUP', function (t) {
})

t.test('SIGINT', function (t) {
var child = cp.exec(fixture + ' xyz')
var child = spawn(fixture, ['xyz'])

var out = ''
child.stdout.on('data', function (c) {
Expand All @@ -114,7 +114,7 @@ t.test('SIGINT', function (t) {

t.test('--harmony', function (t) {
var node = process.execPath
var child = cp.spawn(node, ['--harmony', fixture, 'xyz'])
var child = spawn(node, ['--harmony', fixture, 'xyz'])
var out = ''
child.stdout.on('data', function (c) {
out += c
Expand All @@ -135,7 +135,7 @@ t.test('node exe with different name', function(t) {
var data = fs.readFileSync(process.execPath)
fs.writeFileSync(fp, data)
fs.chmodSync(fp, '0775')
var child = cp.spawn(process.execPath, [fixture, 'xyz'])
var child = spawn(process.execPath, [fixture, 'xyz'])

var out = ''
child.stdout.on('data', function (c) {
Expand Down
20 changes: 20 additions & 0 deletions test/win-rebase.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
var t = require('tap')
var winRebase = require('../lib/win-rebase')

t.test('it replaces path to node bin', function (t) {
var result = winRebase('C:\\Program Files\\nodejs\\node.exe', 'C:\\foo')
t.equal(result, 'C:\\foo')
t.done()
})

t.test('it does not replace path if it references an unknown bin', function (t) {
var result = winRebase('C:\\Program Files\\nodejs\\banana', 'C:\\foo')
t.equal(result, 'C:\\Program Files\\nodejs\\banana')
t.done()
})

t.test('replaces node bin and leaves the script being executed', function (t) {
var result = winRebase('C:\\Program Files\\nodejs\\node.exe foo.js', 'C:\\foo')
t.equal(result, 'C:\\foo foo.js')
t.done()
})