win,install: only download target_arch node.lib

[StefanStojanovic]

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

Instead of downloading node.lib for all architectures, just download the one that will be needed. install.js changed to enable downloading just node.lib for node versions that already have tarball downloaded and extracted. Not fetching lib now fails the installation. Increased installVersion because of the changes.

A note to reviewers - although it seems there are many changes, most of them are just indentation. Also @dsanders11 I'd like to ask you for a review, since your change for install.js landed a few days ago, and I worked on those parts of the code too.

Refs: #2847

[StefanStojanovic]

A question for people with more experience in node-gyp: Since a PR that changed installVersion from 9 to 10 landed yesterday in aaa117c do I need to increase it to 11 now, or not, because there was no release with installVersion: 10?

[dsanders11]

A question for people with more experience in node-gyp

I'm not more experienced, but my 2 cents: it's probably unnecessary for end users (since as you stated, no release went out), but AFAIK there's no harm in bumping it again, and it will prevent a situation where anyone who tested main (so folks working on node-gyp like you and I, or maybe even someone some where consuming node-gyp@main for whatever reason) and got their cache populated for installVersion: 10 won't end up in a broken situation.

So to me bumping it to 11 seems reasonable to prevent any problems.

[dsanders11]

Without going too deep on it, seems reasonable.

For more context, what's the main motivation for this change? Saving disk space, overall efficiency, fixing a corner case?

This does create a small behavior change - while before you could do node-gyp install to "pre-warm" the cache and then do subsequent builds using that cache, say offline, now a build with a specific --target value might require a download which would fail without network connection. Not sure that's necessarily a problem, just noting the change in behavior for end users.

[StefanStojanovic]

For more context, what's the main motivation for this change? Saving disk space, overall efficiency, fixing a corner case?

The main goal is to minimize the number of requests made to the download server, thus decreasing the network traffic. There are some random issues that occur (eg. #2847) that we'd like to get rid of.

This does create a small behavior change - while before you could do node-gyp install to "pre-warm" the cache and then do subsequent builds using that cache, say offline, now a build with a specific --target value might require a download which would fail without network connection. Not sure that's necessarily a problem, just noting the change in behavior for end users.

This is a good point. Thanks for bringing it up. I agree this changes the way node-gyp install works, but it is still possible to pass target_arch to node-gyp install and get node.lib you need (even all 3 of them, but you'll need to run it 3 times). Because this changes how the install command works by default, I've added a semver-major label to it.

[StefanStojanovic]

Can someone from @nodejs/node-gyp please take a look? Please note that the failed GitHub Actions are not caused by this change, it's a general problem that node-gyp started having after updating make-fetch-happen.

[StefanStojanovic]

After rebasing to the latest main (with the fix for GitHub Actions), all the tests are passing. @nodejs/node-gyp Is there something else I should do here?

[dsanders11]

Overall LGTM, although I have not tested it locally. Also AFAIK my check mark is not green here but happy to give it anyway. 😄

[dsanders11]

A question for people with more experience in node-gyp

I'm not more experienced, but my 2 cents: it's probably unnecessary for end users (since as you stated, no release went out), but AFAIK there's no harm in bumping it again, and it will prevent a situation where anyone who tested main (so folks working on node-gyp like you and I, or maybe even someone some where consuming node-gyp@main for whatever reason) and got their cache populated for installVersion: 10 won't end up in a broken situation.

So to me bumping it to 11 seems reasonable to prevent any problems.

[cclauss]

RSLGTM

[dsanders11]

@StefanStojanovic, just realized this could cause problems due to this installVersion check allowing newer versions without reinstalling:

node-gyp/lib/install.js

Line 90 in 33391db

if (installVersion < gyp.package.installVersion) {

If a user were to do something like npx node-gyp install, npx would use node-gyp@9.4.0 (current release) and as a result would not download node.lib for all architectures. If the user then does npm install with a different target_arch, their npm-bundled version of node-gyp will (likely) be older, it would see that the installVersion is newer than its version and consider it "good", but then fail when it can't find node.lib for the target_arch.

Seems like the check I've linked above should be !=, not <. Otherwise changes requiring bumping installVersion have to be backwards-compatible due to the aforementioned scenario, which doesn't seem reasonable. If the installVersion do not match it seems reasonable to force a reinstall.

Although that does mean that you could get fighting versions since npx node-gyp does not use the npm-bundled version. 🤔 That behavior already exists though if your npm-bundled version of node-gyp has a lower installVersion since npx node-gyp would reinstall it.