Use npm's node-gyp if available, otherwise automatically install node-gyp

[Daniel15]

Summary
Some Node.js packages depend on node-gyp but do not declare the dependency in their package.json, instead relying on node-gyp to already be globally installed. For packages like this, we used to bundle node-gyp with Yarn itself. However, this does not work for the standalone JS build (which is used by the tarball now too)

The ideal fix is for packages to explicitly specify that they require node-gyp, as relying on the global version is dangerous (eg. if there's breaking changes between releases).

The changes I've made are as follows:

1. Include node-gyp version from npm in the PATH for lifecycle scripts. This means that Yarn should behave the same way as npm, as long as the user has npm installed (not every Node.js user uses npm - Many distributions of Node.js require you to install npm separately).
2. If no node-gyp is available (not even from a local npm installation), attempt to automatically install it by running yarn global add node-gyp.

Test plan

Tested manually on Windows and on Ubuntu 16.04 by using yarn add nodetunes

I'm not sure how I could unit test this... I guess I could mock out the fs methods, but that might mess up other tests 😕

Fixes #2266
Fixes #3114

cc @bestander

[bestander]

Love the solution, thanks for considering so many edge cases

[SimenB]

Can this get a patch release asap zulu? 😄

EDIT: I realize that expression only works in my timezone, where zulu is one hour behind (CET)... Meh

[SimenB]

this stuff can use path.resolve which gets rid of the .. from the resulting string, and you can pass a single arg with /. Yes, that works on windows.

$ node -pe "require('path').win32.resolve('/root', 'some/path/../slashes')"
\root\some\slashes

[SimenB]

so this can be path.resolve(path.dirname(process.execPath), '../lib/node_modules/npm/bin/node-gyp-bin')

[Daniel15]

path.join gets rid of the .. too 😃

I used path.join with all the pieces rather than hard-coding a string with / since it seemed cleaner. I guess either way is fine.

[SimenB]

path.join gets rid of the .. too

Didn't know! Nice

$ node -pe "require('path').win32.join('/root', 'cool', 'some/path/../slashes', '..')"
\root\cool\some

Would you look at that, it also fixes the slashes! You learn something every day

[bcomnes]

I think this might have complicated rebuilding native dependencies for electron: electron-userland/electron-builder#1542

Still trying to track down the source of the problem, but I can't rebuild sqlite3 for electron now when installing with 0.24.0 and greater.

[Daniel15]

@bcomnes - I think there's some issue specific to node-pre-gyp. Unfortunately I only tested with regular node-gyp.

[bcomnes]

@Daniel15 Do you think the fix should live there? I'm not terribly familiar with the internals of pre-gyp.

[bcomnes]

@springmeyer do you have any idea why these changes might confuse or break the way node-pre-gyp works?

[springmeyer]

@bcomnes the error you refer to in the electron ticket: Failed to execute 'node-gyp clean' (Error: spawn node-gyp ENOENT) indicates that node-gyp is not being found by node-pre-gyp (not on PATH). So it must be that node-gyp is not being detected by any of the checks at https://github.com/mapbox/node-pre-gyp/blob/aecb0dad52e6c185f4380664cd8019293fbf0322/lib/util/compile.js#L20-L54. So, can you figure out where node-gyp is on your system and figure out why none of the checks in node-pre-gyp are finding it? Perhaps the way that this PR searches for node-gyp might need to be added to node-pre-gyp?

[bcomnes]

I'll dig into it. From the sound of the changes in yarn, the only location of node-gyp would be in npm's node_modules. I don't have a global node-gyp install. Thank you for the hint!

[springmeyer]

@bcomnes sounds good. A quick glance at this PR shows path.dirname(process.execPath), 'node_modules', 'npm', 'bin', 'node-gyp-bin' which might need to be added to node-pre-gyp since I see that https://github.com/mapbox/node-pre-gyp/blob/aecb0dad52e6c185f4380664cd8019293fbf0322/lib/util/compile.js#L48-L51 is finding node-gyp at a slightly different (less likely?) location inside the npm install.

[bcomnes]

Sounds right. Want a PR? I might be able to get one out today or tomorrow.

[springmeyer]

Another idea: maybe someone here would be interested in writing a separate module that:

• Is specifically focused on finding the node-gyp binary in likely locations
• Has great tests
• Both yarn and node-pre-gyp could depend on

[springmeyer]

Sounds right. Want a PR? I might be able to get one out today or tomorrow.

Happy to review a PR over at node-pre-gyp. Thanks again for the ping here.

[Daniel15]

The paths that Yarn uses to look for node-gyp are based on my experience with it:

• node_modules/npm/bin/node-gyp-bin on Windows (eg. Node is at C:\Program Files\nodejs\node.exe, node-gyp is in C:\Program Files\nodejs\node_modules\npm\bin\node-gyp-bin\
• ../lib/node_modules/npm/bin/node-gyp-bin on other platforms
  • On Debian: /usr/bin/nodejs for Node, /usr/lib/node_modules/npm/bin/node-gyp-bin/ for node-gyp
  • In the Linux binary tarball from nodejs.org: bin/node for Node, lib/node_modules/npm/bin/node-gyp-bin/ for node-gyp

Yarn adds these paths to the PATH, so any command being executed should find it. @springmeyer - Does node-pre-gyp not look on the PATH for node-gyp?

The only special case is when you do not have npm installed, in which case Yarn needs to install its own version of node-gyp. The logic in Yarn only checks if the command contains node-gyp when running that logic. It probably needs to check for node-pre-gyp too. However, that logic only applies when you do not have npm installed.

Looking at the code that @springmeyer linked to, it sounds like you could fix this by setting the npm_config_node_gyp environment variable.

[bcomnes]

I have to head home now and I haven't finished testing a fix yet. Perhaps tomorrow. If anyone else wants to land a fix first, don't let me stop you.

[bcomnes]

I just tried searching the path for node-gyp when called from electron-builder's install-app-deps launched from yarn, and it doesn't work. Its not getting the path generated by yarn for some reason.

The next solution I will try is resolving the node-gyp path from npm in the exact same way as yarn, as @springmeyer originally suggested. I'm confident this will fix the current problem.

But...

This brings me to a question for @Daniel15: Can you help me understand what problem this change solves by not depending on node-gyp like npm does? It seems like this change causes some problems not mentioned yet:

• You are correct that packages that need node-gyp should depend directly on node-gyp, in theory.
• Many packages do not though because npm includes it. I believe this was one of npm's original sins that they just decided to live with due to breakage it would cause to try to remove it.
• Yarn adding an optional and silent peer dependency on npm is more of a sin than just shipping node-gyp with yarn.
• Promoting additional global dependencies is more of a sin than yarn shipping node-gyp as a dependency.
• Shipping node-gyp is not an expensive dependency for yarn in any way that I can see.

I'm cool either way, but those are just my thoughts now that I understand the situation a little deeper.

[bcomnes]

I just PR'd a possible fix for node-pre-gyp: https://github.com/mapbox/node-pre-gyp/pull/292/files

That should fix the issue, or if you like the argument I made above, reverting back to yarn shipping node-gyp would also put us back into a working state.

[Daniel15]

This brings me to a question for @Daniel15: Can you help me understand what problem this change solves by not depending on node-gyp like npm does?

The standalone .js build of Yarn nas never bundled node-gyp, due to the fact that it's a single file. This is the build of Yarn we use in some places at Facebook, and is also used in various other situations (for example, in cases where people want to commit all their build dependencies). This meant that node-gyp stuff never worked with it. The primary purpose of this pull request was to make node-gyp work the same way on all versions of Yarn - Both the bundled .js build as well as the tarball/installer version.

Additionally, the Yarn tarball now contains the bundled .js file rather than all the raw Yarn JS + node_modules. This significantly decreased the installation footprint of Yarn, allowing Yarn to be used in places where the developers are space conscious (this is how we were able to have Yarn added to the official Node.js Docker image, for example):

However, it means we can't easily bundle node-gyp. It would need to be a separate JS file in the bundle, including all its dependencies, defeating one of the main gains of the combined .js file.

It's worth noting that Yarn will automatically install node-gyp if it's not available on your system. Essentially it runs yarn global add node-gyp if node-gyp is required and not on the path.

Many packages do not though because npm includes it.

This is a fixable problem 😃

[bcomnes]

Thanks for the info, I think I understand now. Looks like there are more concerns at play than I had originally considered.

[AquiGorka]

This solves it: npm i -g node-gyp