fix: re-add support for iojs prefixed headers to support Electron 3

[MarshallOfSound]

This reverts commit 4748f6a.

The original commit broke support for Electron 3 which is still a supported release line. For more details please see #1778

Please note I haven't yet tested if this revert functions, I just ran git revert at 2am and went to bed. Hopefully we can get this landed and fixed tomorrow 👍

[MarshallOfSound]

cc @bnoordhuis

[nicolasnoble]

#1777 prevents Electron builds on existing supported versions of Electron. I'd be great if this PR would get a bit more attention, please.

cc @richardlau and @rvagg who reviewed and merged #1670.

[rvagg]

"still a supported release line" using io.js? Seriously? That's a very very bad idea. I can think of just a few vulnerabilities off the top of my head in the last couple of years, let alone the many since we dropped io.js 4 years ago. We never had an LTS strategy in place for io.js, that came with the merge & Node 4.

I think this might be up to Electron or Electron 3 users to address in a separate space, not node-gyp. It's their call to make it "supported", not ours, we made that call long ago. We're even moving beyond Node 4 around here. If someone wants to support old technologies then that maybe that burden should be taken on elsewhere. A fork of node-gyp is not unreasonable. An even better option is to just use an older version, maybe with an older version of npm as well. You're stretching things that were never intended to be stretched that way.

This is not a final say of course, you're welcome to make more of a case here and maybe you can get others on board, but as it stands I don't think node-gyp should be burdened with supporting io.js.

[MarshallOfSound]

@rvagg Sorry I think that there is a misunderstanding here, Electron 3 does not use iojs. It does in fact use Node.JS 10.2.0 with V8 v6.6.346.32. The fundamental issue here is that node-gyp made the assumption that anything with a version number < 4 was iojs. This meant that when we released Electron 1, 2 and 3 we had to publish our assets with iojs in the name in order for node-gyp to function properly. They are still nodejs assets, but published under the iojs name to our S3 bucket so that node-gyp worked correctly.

For context, Electron v3 with Node.js v10.2.0 has somewhere in the neighborhood of ~500,000 total downloads that effectively equates to ~500,000 unique developer machines due to how we cache. This is a supported version of Electron using a still supported version of node (v10) and IMO the maintenance burden of supporting this code path in node-gyp is relatively small. Heck, I'll personally take ownership of this code path if that's what is required. But for now due to Legacy Assumptions of asset naming and version schemes we have a bunch of assets published with the "iojs" name in them that as of node-gyp@5 can't be used and are breaking builds. I'd really appreciate it if we can find a way forward on this one.

[bnb]

@rvagg since this seems to be a thing in Electron that's currently impacting usage of a technically supported LTS release line, perhaps this could be landed until a patch that addresses both the issues addressed in 4748f6a while also addressing the relatively impactful edge case of electron@3?

[rvagg]

Ahhh sorry for not grokking this @MarshallOfSound, I see you also explained that in #1778, my bad.
In that case, I'm fine with this but I would like to find a more minimal route if possible. The _ORG_MIRROR environment variables were slated for removal and the semver-major bump was our chance to do that. If we do a full revert then we're going to have to go through that cycle again for another semver-major bump (not end of the world, we're not time constrained on when we can do that, just annoying and can get confusing for users).

What set of _ORG_MIRROR environment variables are important for Electron? Which ones can we safely discard while maintaining Electron compatibility? I'm guessing that IOJS_ORG_MIRROR is the one that's used, so can we at least get rid of NVM_IOJS_ORG_MIRROR and NVM_NODEJS_ORG_MIRROR?

If you could add another commit that does some removal that would be great. A note underneath the isIojs = versionSemver.major >= 1 && versionSemver.major < 4 line would be helpful too, explaining that Electron 3 is dependent on this so that in the future we know when we can remove it, because there will be a desire to remove it still. If you have details on when that support ends then that might be good to include too.

[MarshallOfSound]

What set of _ORG_MIRROR environment variables are important for Electron? Which ones can we safely discard while maintaining Electron compatibility? I'm guessing that IOJS_ORG_MIRROR is the one that's used, so can we at least get rid of NVM_IOJS_ORG_MIRROR and NVM_NODEJS_ORG_MIRROR?

The mirror URLs afaik do not impact Electron in any way, I just did a simple git revert of the commit. I can do a more hand crafted one if we'ok with just adding the >=1 v < 4 logic back in 👍

If you have details on when that support ends then that might be good to include too.

I can add those details in the comments on the EOL of electron@3 👍

[rvagg]

OK, I get it now. That name is a bit invasive, mainly on Windows for the .lib files, and we don't infer name from process.release.headersUrl (which I assume Electron 3 is shipping a custom version of). I don't quite understand why the headers files need to be named iojs though because that just becomes the tarballUrl whatever it is, just the .lib files. But I guess if you have that baked in we're stuck with it.

[MarshallOfSound]

@rvagg Updated the PR to only do the name change and added a comment explaining why we need it and a general idea for the potential timeline for removal

[rvagg]

@MarshallOfSound you can confirm this works for Electron 3?

[MarshallOfSound]

@rvagg Yup, I npm link'ed to my local version of electron-rebuild and ran the test suite and it passed 👍 (the PR to bump directly to 5.0 failed).

[rvagg]

Landed in 1cfdb28, will get a release proposal up soon

[MarshallOfSound]

@rvagg Thanks 👍