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

nyc@15 crashes when spawning a different Node.js version #1246

Closed
ehmicky opened this issue Dec 22, 2019 · 8 comments
Closed

nyc@15 crashes when spawning a different Node.js version #1246

ehmicky opened this issue Dec 22, 2019 · 8 comments

Comments

@ehmicky
Copy link

ehmicky commented Dec 22, 2019

Link to bug demonstration repository

https://github.com/ehmicky/nyc-bug-repro

Expected Behavior

nyc should not crash when spawning a different Node.js version than the current one.
This happens for example when using nvm or similar tools.

Observed Behavior

nyc crashes.

Troubleshooting steps

index.js:

const { spawn } = require("child_process");
const { homedir } = require("os");

const nodePath = `${homedir()}/.nvm/versions/node/v11.15.0/bin/node`;
spawn(nodePath, ["--help"], { stdio: "inherit" });
$ nvm install 11.15.0
$ nvm install 12.0.0
$ nvm use 12.0.0
$ npm install
$ nyc node index.js
internal/modules/cjs/loader.js:670
    throw err;
    ^

Error: Cannot find module '"/home/user/nyc-bug-repro/node_modules/node-preload/preload-path/node-preload.js"'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:668:15)
    at Function.Module._load (internal/modules/cjs/loader.js:591:27)
    at Module.require (internal/modules/cjs/loader.js:723:19)
    at Module._preloadModules (internal/modules/cjs/loader.js:945:12)
    at loadPreloadModules (internal/bootstrap/pre_execution.js:363:5)
    at prepareMainThreadExecution (internal/bootstrap/pre_execution.js:61:3)
    at internal/main/print_help.js:180:1
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |
 index.js |     100 |      100 |     100 |     100 |
----------|---------|----------|---------|---------|-------------------

Notes

Please note the following bug only happens when:

  • the current Node.js version is >= 12
  • the spawned Node.js version is < 12

Also this only happens with nyc@15. I cannot use --use-spawn-wrap=true because spawn-wrap has its own sets of bugs that node-preload fixes.

The index.js file is spawning a node binary directly via its absolute path. This is only to make it easy to reproduce. In real life, this would be done when the PATH environment variable points to a different Node.js version, which happens when using nvm run and other tools.

Environment Information

  System:
    OS: Linux 5.3 Ubuntu 19.10 (Eoan Ermine)
    CPU: (16) x64 Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz
    Memory: 51.92 GB / 62.76 GB
  Binaries:
    Node: 12.0.0 - ~/.nvm/versions/node/v12.0.0/bin/node
    npm: 6.9.0 - ~/.nvm/versions/node/v12.0.0/bin/npm
  npmPackages:
    nyc: ^15.0.0 => 15.0.0 
coreyfarrell added a commit to cfware/node-preload that referenced this issue Dec 22, 2019
Use `NODE_PATH` and use bare filename for `--require` if it is needed in
node.js < 12 or if node.js 12+ would otherwise require quoting the
require path.  This should avoid a crash when `node-preload` is setup in
node.js 12 but then a test is run under node.js < 12.  It should also
ensure compatibility when setup is performed in node.js < 12 and tests
are later run under node.js 12+.

Ref istanbuljs/nyc#1246
coreyfarrell added a commit to cfware/node-preload that referenced this issue Dec 22, 2019
Use `NODE_PATH` and use bare filename for `--require` if it is needed in
node.js < 12 or if node.js 12+ would otherwise require quoting the
require path.  This should avoid a crash when `node-preload` is setup in
node.js 12 but then a test is run under node.js < 12.  It should also
ensure compatibility when setup is performed in node.js < 12 and tests
are later run under node.js 12+.

Ref istanbuljs/nyc#1246
@coreyfarrell
Copy link
Member

Thanks for this report. I've posted a PR to node-preload which should deal with this. Would you mind testing the patch to verify it actually fixes your issue in practice? You should be able to install git://github.com/cfware/node-preload#version-jump in a package with nyc 15.0.0. Please verify with npm ls node-preload that the git branch is the only copy of node-preload.

@ehmicky
Copy link
Author

ehmicky commented Dec 22, 2019

I just tried it and it works! Thanks a lot Corey 👍

@coreyfarrell
Copy link
Member

Thanks for the update. Be aware the branch I linked will be going away shortly and node-preload@0.2.1 will be released. nyc uses ^0.2.0 so it should pull the update as long as you don't have a lockfile blocking it. I'll close this issue once I've had a chance to release the linked PR.

coreyfarrell added a commit to cfware/node-preload that referenced this issue Dec 22, 2019
Use `NODE_PATH` and use bare filename for `--require` if it is needed in
node.js < 12 or if node.js 12+ would otherwise require quoting the
require path.  This should avoid a crash when `node-preload` is setup in
node.js 12 but then a test is run under node.js < 12.  It should also
ensure compatibility when setup is performed in node.js < 12 and tests
are later run under node.js 12+.

Ref istanbuljs/nyc#1246
@coreyfarrell
Copy link
Member

node-preload@0.2.1 is now published, I verified it gets pulled in by npm init -y && npm i -D nyc@next.

@ehmicky
Copy link
Author

ehmicky commented Dec 22, 2019

Thanks a lot Corey!
Thanks also for the v15 update, dropping spawn-wrap fixed many problems for me.

@coreyfarrell
Copy link
Member

I'm hoping dropping spawn-wrap by default fixes problems for many people (especially Windows users).

@XhmikosR
Copy link
Contributor

For what is worth the current tests don't pass on native Windows cmd. Travis is using msys2 or something. I'm unsure if this affects nyc Windows users though, I just noticed it while trying to do npm t on my Windows 10 VM.

@coreyfarrell
Copy link
Member

@XhmikosR would you mind opening a new issue including a link to a repo and any special instructions for how to reproduce the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants