Node-gyp: Electron 4 is having ~/.node-gyp cache collisions

[pfrazee]

When rebuilding a module for Electron 4, the compiles began to fail by saying that node_api.h was not found.

It appears that the ~/.node-gyp folder had a 4.0.0 subfolder for node v4. Electron v4 was colliding with that node-4 folder. The solution was to delete the ~/.node-gyp/4.0.0` folder and install again.

Any ideas on how to handle the collision?

[rvagg]

Sorry @pfrazee for the lack of response here, I assume you've been able to move beyond this since December.
For anyone else searching for this, @MarshallOfSound do you have a suggestion about this? I assume it's not uncommon.

[MarshallOfSound]

Our docs have a recommendation to override the HOME var to redirect that folder to ~/.electron-gyp --> https://electronjs.org/docs/tutorial/using-native-node-modules#manually-building-for-electron

If you use electron-rebuild it will set that for you 👍

[rvagg]

HOME=~/.electron-gyp node-gyp is pretty nasty, perhaps we can fix that for you here and introduce an override argument or some way of detecting that it needs to go into ~/.electron-gyp? Surely there's something in process... that could be used to switch that?

[MarshallOfSound]

Using a hash of the distUrl or using the ~/${runtime}-gyp value would probably work. node-gyp when building for electron still runs in the users locally node so we have to figure out which dir to use based solely on arguments

[rvagg]

Reopened to see if we can come up with a solid solution for this to prevent the need for hacks like HOME modification. (Prompted by #1807).

[MarshallOfSound]

@rvagg How would you feel about changing devDir from node-gyp to node-gyp/${runtime}? Not sure if that's considered breaking as it'll just redownload the headers 🤔

[rvagg]

Well we just broke it with node-gyp@5 by switching to env-paths to do native "caches" dir, so /Users/user/Library/Caches/node-gyp on macOS, /home/user/.cache/node-gyp on Linux. So I guess we're not above breaking it! Doing a node-gyp@6 isn't out of the question to make this happen and maybe sooner than later would be a good idea.

[rvagg]

And, to make it less breaking, we could skip a runtime/ subdirectory if it === node, but I'm not even sure that's necessary. We've not had complaints yet about v5 breaking for anyone.

[todbot]

Looks like prebuild is having this issue too: prebuild/prebuild#249.

While the Electron docs suggest using HOME=~/.electron-gyp node-gyp rebuild to prevent collisions, it seems like the prebuild team would like node-gyp to handle this.

(aside: is doing HOME=<path> node-gyp rebuild ... the same as specifying node-gyp rebuild --devdir=<path> ...?)

[vweevers]

it seems like the prebuild team would like node-gyp to handle this

To clarify: I expressed that preference, I don't speak for other prebuild contributors.

[todbot]

Hi @vweevers, apologies for mis-speaking about prebuild team.

[mafintosh]

This is happening now for electron 6 if you ever built something for node 6 also.

I pushed a workaround in my prebuildify module that looks like this if anyone is running into the same issues https://github.com/prebuild/prebuildify/commit/c5ab0eb20a0a3f4857ea712be1570d8c8ecdfc65

[mafintosh]

@rvagg helped multiple people this week with this since people are trying out electron 6, so would be great to fix this in node-gyp itself :)

[cclauss]

Do we know if https://github.com/refack/GYP3 exhibits the same behavior?

[rvagg]

@cclauss it shouldn't impact it, this logic comes from the JS side. We just need someone to work up a proposal. I'll _try_ and get to it some time but would be appreciative if someone else had the time to throw together a basic proposal at least.

[richardlau]

@nodejs/node-gyp Probably semver-major, but what if we cached based on the NODE_MODULE_VERSION instead of Node.js version? There should be no clashes for current releases based on https://github.com/nodejs/node/blob/master/doc/abi_version_registry.json (there may still be clashes for versions prior to the introduction of the ABI version registry).

[rvagg]

@richardlau probably a good idea now that we've strongly pinned NODE_MODULE_VERSION to semver-major version; I don't think we really need to support < Node.js 10 at this stage so we might be safe now?

The only problem might be the --target flag, how do we manage the ability to compile against different versions? This is something that addon authors rely on to do pre-build binaries for different versions of Node.js.

[richardlau]

--target is currently defined as "Node.js version to build for (default is process.version)". That doesn't allow for Electron (putting the Electron version here is ambigious without some sort of qualifier) or any other runtime that ships a particular version of Node.js with an incompatible NODE_MODULE_VERSION.

We could:

• add some sort of qualifier
  
  
  
  • either as the option argument, e.g. electron:6.0,0
  
  
  • or a new option, e.g. --target-runtime electron (default to node). (I'd prefer this to changing the format of the --target option)
  
  
• make our users (or calling tooling) specify the NODE_MODULE_VERSION being targetted (not especially user friendly)
  
  
  
  • change the value of --target to be the target NODE_MODULE_VERSION
  
  
  • add a new option (--target-abi ?) to be the target NODE_MODULE_VERSION
  
  

Implementation wise we'd have to do a lookup in node-gyp based on https://github.com/nodejs/node/blob/master/doc/abi_version_registry.json -- maybe try to fetch the current version but ship a version in node-gyp as a backup for network restricted environments?

[MarshallOfSound]

process.versions.modules is the ABI version fwiw. There is also the node-abi package that can be used for determining the ABI version of node / electron

[rvagg]

yeah, having a recent version of the registry is going to be the biggest problem, and we're going to need special rules for figuring out what to do with certain numbers:

    { "modules": 79, "runtime": "node",     "variant": "v8_7.8",               "versions": "13" },
    { "modules": 78, "runtime": "node",     "variant": "v8_7.7",               "versions": "13.0.0-pre" },
    { "modules": 77, "runtime": "node",     "variant": "v8_7.6",               "versions": "13.0.0-pre" },

We probably exclude *-pre versions, even though it'd be theoretically possible to find some headers that worked (index.json in nightlies could give us this).

I think I'm in favour of offloading some of the responsibility to users of --target, we could deprecate --target and introduce --target-abi because it's more technically correct for what they're after. Having a central registry makes it (a) easier for users to grok and use and (b) much simpler for us to figure out what they're after.

@MarshallOfSound neither process.versions.modules or the node-abi package is helpful here. We're shipping a tool that'll get bundled into npm and used for _years_ in that single version without people updating it. We're still dealing with bug reports from versions 3.x and 4.x! So anything we do in this area needs to be at least a little future proof. "Sorry, I don't know what a modules version of 142 is, I only understand <= 80".