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

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

Closed
wants to merge 2 commits into from

Conversation

bcomnes
Copy link

@bcomnes bcomnes commented May 16, 2017

This fixes issues during weird rebuild scenarios when installing with yarn and rebuilding for electron using electron-builder.

The fix is fairly general though, as it will search the users path for npm, then resolve the node-gyp that it ships. If npm doesn't exist, it will noop.

I'm not really sure how to test this, other than locally, since its such an environmental codepath.

Related to: yarnpkg/yarn#3240
and electron-userland/electron-builder#1542

This fixes issues during weird rebuild scenarios when installing with
yarn and rebuilding for electron using electron-builder.

The fix is fairly general though, as it will search the users path for
npm, then resolve the node-gyp that it ships.  If npm doesn't exist, it
will noop.
@@ -52,6 +52,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.

@Daniel15
Copy link

I just noticed that node-pre-gyp doesn't declare a dependency on node-gyp in its package.json file. Wouldn't it be safer to make that dependency explicit, rather than implicitly relying on a globally-installed node-gyp?

@bcomnes
Copy link
Author

bcomnes commented May 17, 2017

I think that would work too. I figured there was a reason for that but I guess I should have asked.

@bcomnes
Copy link
Author

bcomnes commented Apr 30, 2019

Im abandoning this PR.

@bcomnes bcomnes closed this Apr 30, 2019
@bcomnes bcomnes deleted the fix-node-gyp-yarn branch April 30, 2019 23:33
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

Successfully merging this pull request may close these issues.

3 participants