fix node gyp from electron-spawn

[kewde]

No description provided.

[kewde]

builds with unparsed options

error MSB4057: The target "/p:FOO=bar" does not exist in the project

^works with Node 6 but fails with node 10, hmm.

The failing test is adding t:/ to the environment variable.

gyp info spawn msbuild
gyp info spawn args [ 'build/binding.sln',
gyp info spawn args   '/t:/p:FOO=bar',
gyp info spawn args   '/clp:Verbosity=minimal',
gyp info spawn args   '/nologo',
gyp info spawn args   '/p:Configuration=Release;Platform=x64' ]
gyp ERR! build error 
gyp ERR! stack Error: `msbuild` failed with exit code: 1

should've been:

gyp info spawn args   '/p:FOO=bar',

The builds work with older versions of node-gyp.
nodejs/node-gyp#1616

[kewde]

@springmeyer
I've had to remove two tests - the ability to pass unparsed options to node-gyp is broken across versions for windows.
A patch has been submitted to nodejs/node-gyp#1616 but it unlikely that this will be merged anytime soon.

I need this fix for node-sqlite3, it's not detecting the electron versions properly.

[springmeyer]

@kewde okay, thanks for this work.

I've had to remove two tests - the ability to pass unparsed options to node-gyp is broken across versions for windows.

How about, instead of removing, making them run only for non-windows platforms and adding a code comment that the test could be re-enabled if/after nodejs/node-gyp#1616 is merged?

Also @kewde @nicolasnoble is recently taking on publishing node-pre-gyp releases, so feel free to ping them for review here when this is ready to publish.

[nicolasnoble]

It leaves me fairly sad that this is the only solution we have, since electron is being fairly obtuse about its own versions here. I'm wondering how other similar projects are doing this.

I'd love to see a comment here that explains the situation of this fallback however. Something along the lines of

// If we don't have an electron version, this means we're in a fork.
// We are then forced to do some heuristics to determine electron's version.
// See https://github.com/electron/electron/issues/9058 for details.

Maybe also we could do a node-pre-gyp environment variable override as well, in case the heuristic fails, as a last resort for users? Something along the lines of process.env.NODE_PRE_GYP_OVERRIDE_ELECTRON_VERSION maybe?

[kewde]

I've briefly looked at the C++ source code of electron and it seems to me like environment variable ELECTRON_RUN_AS_NODE can be overloaded with the version number as it only checks whether it has been defined or not. I would have to verify this though.

It remains a strange and suboptimal solution though.

[nicolasnoble]

I was more talking about the case where something goes just wrong, and as a last resort, the end user could always use some mechanism to override the automatic detection. Not trying to inject the dependency through some kludge, but rather have an explicit, external, user-controlled override.

[murgatroid99]

If electron is installed as a global package, this will not work because require does not access global packages.

[murgatroid99]

One alternative possibility would be to look at process.execPath, and traverse from there to Electron's package.json.

[springmeyer]

this went stale. if others still see fixes needed please recreate a new PR against latest node-pre-gyp master branch.