Upgrade foreground-child, remove unnecessary arg munging

[isaacs]

Foreground-child is now using cross-spawn-async in order to be able to
properly spawn shebangs and .cmd/.exe files. This avoids the extra
cmd.exe that win-spawn throws into the mix, since that causes problems
when there is no PATH environ and a binary is being called explicitly
via a full path name.

Cross-spawn-async is a lot more code than win-spawn, but the approach it
takes is more surgical and well-tested.

Because of this, it's no longer necessary to unshift the node/io.js
binary onto the argument list when spawning a shebanged javascript file.
So, doing 'nyc mocha ...' on Windows will now work, and it's no longer
necessary to do 'nyc ./node_modules/mocha/bin/mocha.js ...' or any other
extra manual path munging.

[jamestalmage]

LGTM!

[isaacs]

It looks like this might only be part of the solution.

I'm getting a NYC_REQUIRE=undefined in the environment on Windows. This diff seems to fix it, but idk why:

diff --git a/bin/nyc.js b/bin/nyc.js
index b63b5ee..0132a79 100755
--- a/bin/nyc.js
+++ b/bin/nyc.js
@@ -122,11 +122,14 @@ if (process.env.NYC_CWD) {
     if (argv.all) nyc.addAllFiles()
     if (!Array.isArray(argv.require)) argv.require = [argv.require]

-    sw([__filename], {
+    var env = {
       NYC_CWD: process.cwd(),
-      NYC_REQUIRE: argv.require.join(','),
       NYC_CACHE: argv.cache ? 'enable' : 'disable'
-    })
+    }
+    if (argv.require.length) {
+      env.NYC_REQUIRE = argv.require.join(',')
+    }
+    sw([__filename], env)

     foreground(nyc.mungeArgs(argv), function (done) {
       if (!argv.silent) report(argv)

But now, I get this:

Error: Cannot find module 'c:\tap\--silent'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:289:25)
    at Function.Module.runMain (module.js:467:10)
    at Function.runMain (c:\tap\node_modules\nyc\node_modules\spawn-wrap\index.js:248:10)
    at Object.<anonymous> (c:\tap\node_modules\nyc\bin\nyc.js:19:6)
    at Module._compile (module.js:435:26)
    at Object.Module._extensions..js (module.js:442:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:313:12)
    at Function.Module.runMain (module.js:467:10)

so it looks like it's not properly chopping off the arg when doing nyc --silent <some command>?

[jamestalmage]

@isaacs

Your branch works for me (without your diff or any other modification) on Windows 7, with Node 4.2.1 and 5.3.0.

// foo.js
var tap = require('tap');

tap.is(1, 1);

command:

node .\bin\nyc.js --silent tap foo.js

[jamestalmage]

also works when npm linked:

nyc --silent tap foo.js

(only tested that on Node 4.2.1).

[jamestalmage]

Your branch also seems to work just fine when I npm link into the node-tap test suite (I get the same errors using [email protected] and your branch).

One thing to note: you want to do npm run clean in the nyc directory before npm linking. Otherwise you end up using files instrumented for nyc self coverage, which can cause problems.

[isaacs]

That --silent thing was totally a legit bug in spawn-wrap, fixed on 1.1.1 (tagged next in npm for now).

[bcoe]

On Windows 7 I now get this slightly different error:

I can dig a bit more tomorrow, and see what the heck it attempts to spawn on Windows.

[jamestalmage]

Switching to just nyc --cache mocha --timeout=4000 --check-leaks fixed the yargs test for me.

(I do not have mocha installed globally, so it's not that it's picking up a global install. It is honoring the path environment set by npm).

I do get a single test failure, but it looks like it is related to newline separators, and occurs when I just run node_modules\.bin\mocha.

[bcoe]

@jamestalmage cool, confirmed myself having simply mocha rather than the full path does work -- makes me think this will be an easy fix.

[isaacs]

Yeah, the --silent thing was definitely spawn-wrap's fault. That's published on its @next tag.

I'm most of the way done with making tap's tests all pass (and generate coverage!) on Windows. The hardest one (and where I'm calling it a night) is tap's test/test.js that runs 43 output tests and verifies their output. Turns out that a LOT of tap's output has a lot of file names in it, so you get errors like this (note the path slashes):

not ok 8 - should match pattern provided
  ---
  found:
    at:
      file: "test\\test\\assert-todo-skip.js"
      line: 7
      column: 5
    source: |
      t.ok(false, 'expected', {todo: 'implement a thing'})
  pattern:
    at:
      column: 5
      file: test/test/assert-todo-skip.js
      line: 7
    source: |
      t.ok(false, 'expected', {todo: 'implement a thing'})

If I live for a thousand centuries, I might never forgive Microsoft for their unfathomable continued use of back slashes.

[jamestalmage]

OK,

The following hack to nyc/node_modules/spawn_wrap/index.js makes the yargs test suite work

- var spawn = ChildProcess.prototype.spawn 
+ var __spawn = ChildProcess.prototype.spawn
+
+  function spawn(options) {
+    console.log('spawn called: ', options.args[3]);
+    options.args[3] = options.args[3].replace('"/bin/sh" ', '')
+    __spawn.apply(this, arguments);
+  }

Somehow a "/bin/sh" is leaking into the arguments. Here is that log statement (before I did the options.args[3].replace line):

[isaacs]

If you're seeing tap+nyc fail on Windows, can you try this?

npm i isaacs/node-tap#wsup

That should pull in this nyc PR, plus the windows support I just landed in tap, @next versions of foreground-child and spawn-wrap.

[jamestalmage]

yargs uses mocha, and that fails with this PR. I haven't seen tap fail yet.

[isaacs]

I'd argue that the /bin/sh is actually correct behavior, since yargs is explicitly providing a path to a sh script.

c:\yargs>cat node_modules/.bin/_mocha
#!/bin/sh
basedir=`dirname "$0"`

case `uname` in
    *CYGWIN*) basedir=`cygpath -w "$basedir"`;;
esac

if [ -x "$basedir/node" ]; then
  "$basedir/node"  "$basedir/../mocha/bin/_mocha" "$@"
  ret=$?
else
  node  "$basedir/../mocha/bin/_mocha" "$@"
  ret=$?
fi
exit $ret

Then cross-spawn finds that the script does indeed exist, reads the shebang, and tries to execute it as intended.

It's only cmd.exe that uses PATHEXT rather than the explicitly provided script name. So, when win-spawn put an extra cmd in the process stack, then cmd munged node_modules/.bin/_mocha into ./node_modules/.bin/_mocha.cmd, even though that's not what we provided.

It smells to me like a hacky workaround to specify a test script of ./node_modules/.bin/_mocha rather than simply mocha. Yargs PR coming shortly.

[isaacs]

Also, spawn-wrap 1.1.1 fixes the failing-to-wrap bug that was the cause for doing _mocha instead of mocha in the first place.

[bcoe]

@isaacs my testing went quite well (I'm quite happy to stop referencing the absolute path to the bin in yargs). Mind rebasing this and we'll land?

[bcoe]

merged in #125

[jamestalmage]

I'd argue that the /bin/sh is actually correct behavior, since yargs is explicitly providing a path to a sh script.

Hmm. I didn't realize that. Still, this seems like a potential point of confusion. The error message was particularly unhelpful ("The system could not find the path specified"). My initial assumption on reading that was that it wasn't finding the nyc or _mocha files.

I'm assuming nyc isn't the place to improve the logged error. Would the right place be spawn-wrap or cross-spawn-async?