a couple tweaks to make things work on windows

[bcoe]

• disabled a couple tests that do not currently work on Windows.
• pulled the windows-rebase logic into a module and put some tests around it:
  • c:\Program Files\node foo -> c:\node.cmd foo
  • c:\Program Files\node -> c:\node.cmd.
• we now also support cmd, along with cmd.exe, this is the bin used by win-spawn.
• a couple tweaks to the cmd-shim:
  • turn off echo.
  • quote the exec path.
• switched tests to win-spawn.

[bcoe]

I have tested this branch on Windows and confirmed that the tweaks do the trick:

[rmg]

@bcoe is this still working? Is it done?

[bcoe]

@rmg @isaacs finally took the time to get tests passing on Linux, Windows, and OSX. cp.exec seemed to be behaving a bit differently on *nix vs. OSX machines, solved by switching tests to spawn.

This should put us in a position to have nyc stop running a forked version of spawn-wrap \o/

[rmg]

🎉

[bcoe]

@isaacs we're not running the test-suite on AppVeyor yet, so I just removed win-spawn and the Windows checks in test.

It would be nice to have some tests for exec(), but having tested on master I believe these tests never passed on *nix systems:

https://gist.github.com/bcoe/574f20564e423375d0a6

I propose pulling this in, and then writing some targeted tests for exec() and digging into why Ubuntu doesn't like exec().

[rmg]

@bcoe is Ubuntu the only Linux distribution you test with? Can't tell from your description if you mean the failure is distro specific or OS specific.

[bcoe]

@rmg I've only tested on Ubuntu 14.04, Travis, and OSX. master fails on both Travis and Ubuntu, works on OSX.

my guess would be that exec has never worked on debian OSes -- potentially an underlying problem with signal-exit.

I'd like to advocate getting the Windows fixes in, and then getting to the bottom of exec on Ubuntu.

[isaacs]

@bcoe This smells like a race condition to me. The process could be exiting before the signal can get to it.

[isaacs]

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)

[isaacs]

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.

[isaacs]

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

[isaacs]

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.

[bcoe]

@isaacs @rmg boom \o/ exec() is back in the mix, the problem was simply:

• exec() on OSX returns the child process of the actual node process we want to kill.
• exec() on Ubuntu returns the child process of the shell executing the node process.

[isaacs]

I was able to simplify some of this, and make things a bit more stable, with fewer opportunities for false positives on some of the path tests, and one less .cmd in the process stack most of the time.

Thanks for continuing to research and work on this! Tests all passing now on Windows, Darwin, and Ubuntu.

[rmg]

A small PR in a small module, but the ripples this could have for testing node modules on Windows are very far reaching.