Fix toolkit selection for VS 2017/2019

[haxtibal]

Checklist

• npm install && npm test passes

Description of change

npm install --build-from-source sqlite3 failed for me in a fresh Visual Studio 2019 Build Tools environment, because node-gyp fell back to platform tools v140 which are not installed in VS 2019 by default, but v142 is. The error message was something like

The builds tools for v140 (Platform Toolset = 'v140') cannot be found.

This patch basically repeats some lines that @refack has already in GYP3 to default to toolset v141 for VS2017 and v142 for VS2019, see

• refack/GYP3@c40235d
• refack/GYP3@703706c
• refack/GYP3@eb296f6

In case you're already about to replace GYP with GYP3, sorry for the noise, just ignore this PR.

Disclaimer:

• Registry based detection in MSVSVersion._DetectVisualStudioVersions is still incomplete. Maybe it's even in GYP3, because registry handling has changed in VS2017/19 and keys are stored in a private location now. I didn't bother much, it works in node-gyp context anyway because node-gyps configure.js does it's own detection based on VCINSTALLDIR and sets GYP_MSVS_OVERRIDE_PATH and GYP_MSVS_VERSION accordingly. Btw., why is detection done in both the JS part and GYP, shouldn't it go to GYP solely for separation of concerns?

• I guessed project_version='16.0' for 2019, but it seems the Version element is not meaningful anymore at all in MSBuild project files?

[joaocgreis]

Visual Studio detection is completely done in node-gyp. Gyp is forced to use the detected version, completely bypassing the detection mechanism there. We do this to be able to provide good logging output, filter Visual Studio installations based on ability to compile C++ and support by the Node.js version being used.

I was surprised when I was able to reproduce your issue. sqlite3 indeed does not compile, but other modules (bcrypt, leveldown, ...) compile without issues. I investigated a bit and found that this is an issue in the sqlite3 module, it overwrites the variable node-gyp uses to specify the toolset.

PR in sqlite3 with a fix: TryGhost/node-sqlite3#1242

This PR should not land here, and I don't know how well this behaves when multiple versions of VS are installed. I don't have strong feelings about the Gyp changes, but managing Gyp has been a problem since Google stopped supporting it so it's probably better that we change it as little as possible. Modules affected by this problem should be fixed instead. @haxtibal thanks for opening a PR and actually working toward solving the issue!

[haxtibal]

PR in sqlite3 with a fix: TryGhost/node-sqlite3#1242

@joaocgreis That will solve my issue, thank's a lot for investigating.

Modules affected by this problem should be fixed instead.

Addon writers are officially instructed to create a binding.gyp, i.e. use the GYP language. They may well assume they have full GYP functionality available, including detection. Would it make sense to fix the documentation in first place (telling what works and what not), and only then the packages?

[joaocgreis]

I believe GYP works pretty much as expected. If you search for defaults and variables in https://github.com/nodejs/node-gyp/blob/master/lib/configure.js, there are a few more that node-gyp passes to GYP. So, node-gyp controlling some parts of the process is not unexpected.

@haxtibal I think it would be good to add a note in the documentation if you want to do that. Perhaps in https://github.com/nodejs/node-gyp/tree/68319a2c344c822d48bd6d5dd32f82dd41384e19#the-bindinggyp-file as it may be too specific for the core docs, but others may disagree so feel free to try a PR there.

[joaocgreis]

PR in node-sqlite3 landed, we can close this here but let me know if there are still issues. Thanks!