Skip to content

Search path for npm to use npm's node-gyp #292

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Search path for npm to use npm's node-gyp #292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a2e160
Search path for npm to use npm's node-gyp
bcomnes May 16, 2017
5e117a4
require which
bcomnes May 16, 2017
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
9 changes: 9 additions & 0 deletions lib/util/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ module.exports = exports;

var fs = require('fs');
var path = require('path');
var which = require('which').sync;
var win = process.platform == 'win32';
var existsSync = fs.existsSync || path.existsSync;
var cp = require('child_process');
Expand Down Expand Up @@ -52,6 +53,14 @@ function which_node_gyp() {
if (existsSync(node_gyp_bin)) {
return node_gyp_bin;
}

try {
var maybe_elsewhere = path.join(path.dirname(fs.realpathSync(which('npm'))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you forgot to require which? Also, is it absolutely needed? Would be great to not add more deps.

Copy link
Author

Choose a reason for hiding this comment

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

Oops, given the bizarreness of toolchains and installers and nested dips that are needed to trigger the issue, I had to do work inside of an active node_modules and move the work over once I figured it out. Adding that requirement now.

An alternative to which would be baking in the common path that triggers this problem:
/usr/local/lib/node_modules/npm (the default place homebrew moves npm when you install node.). The problem is that users can and do change the basepath that homebrew installs too (don't ask me why 😭) or even introduce other npm installs in other parts of their environment. Fundamentally though, we are looking for node-gyp inside of npm's node_modules folder from the users configured environment. This is the most direct path to find that as far as I can think of. which is a fairly tried and true dependency by issacs that npm itself depends on, so its probably a fairly safe bet.

I also suggested that yarn not silent peer-depend on npm and just ship node-gyp like it used when everything 'just worked' to but I'm waiting to hear back about that idea.

Either way, this adds one other fairly robust system check for node-gyp before giving up.

'..', 'node_modules/node-gyp/bin/node-gyp.js');
if (existsSync(maybe_elsewhere)) {
return maybe_elsewhere;
}
} catch (err) { }
}

module.exports.run_gyp = function(args,opts,callback) {
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@
"rimraf": "^2.6.1",
"semver": "^5.3.0",
"tar": "^2.2.1",
"tar-pack": "^3.4.0"
"tar-pack": "^3.4.0",
"which": "^1.2.14"
},
"devDependencies": {
"aws-sdk": "^2.28.0",
Expand Down