Improve Visual Studio detection and add VS2019 support #1762
Conversation
|
|
||
| function logWithPrefix (log, prefix) { | ||
| function setPrefix(logFunction) { | ||
| return (...args) => logFunction.apply(null, [prefix, ...args]) |
There was a problem hiding this comment.
are we going to have complaints about the ES6+ here? this will probably get used with some ancient versions, if this is the first use of arrow functions and spread then maybe we should replace that and hold off.
There was a problem hiding this comment.
node-gyp requires Node 6 since #1570. This should not land in v3 (IIUC there won't be more of those anyway), I've added the semver-major label just in case.
This logWithPrefix function was moved from configure.js, and has been in master for a while (though not in a release).
|
yeah, i have no idea, meaningless rubber stamp from me |
joaocgreis
force-pushed
the
joaocgreis-J5O-generic-vs
branch
from
a951d24 to
5460da7
Compare
|
Looks good to me. |
|
I was thinking, why not just bundle a known P.S. |
lib/configure.js
Outdated
| } | ||
| if (vsInfo.versionMajor > 15 || | ||
| (vsInfo.versionMajor === 15 && vsInfo.versionMajor >= 9)) { | ||
| defaults['msvs_enable_marmasm'] = 1 |
There was a problem hiding this comment.
IMO this is only needed if variables.target_arch == 'arm64'
There was a problem hiding this comment.
Added a fixup, thanks @refack!
|
BTW: if this is not time sensitive, most of the code in this is already implemented by GYP3 (which actually should be ready for node-gyp, only known bug is with cross-compilation). The test fixtures are fantastic! 🥇 |
|
@refack I mentioned the reasons for not doing this in GYP3 in the opening comment. Actually, the first thing I considered was doing this there, but I think here is the right place. IMO the log output is very important, short in case of success and very detailed in case of error. It should help a lot with the issues we get about this.
I also considered that. |
|
Added a small commit to avoid possible errors when running from VS Command Prompt. @rvagg sorry for this late addition, please let me know if you have any objection. CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/130/ |
For component filtering I use the PowerShell client DLL (it's also bundled in the MSVS installer)
💯 X 💯 I'll try to figure out a way to port stuff to GYP3... Maybe have P.S. mostly because Python has builtin Registry sys-calls... |
That could also be an option. However, given that this method with the system C# has been in use for a while without issues (with the method itself - others I'm hoping to solve with this PR!), I'd rather not change it. The COM interface should not change, so the more layers we add the more we risk, I think.
I see this more like a way to free GYP3 of this burden. This is only needed for node-gyp AFAICT, so for Node core and other projects GYP3 could simply use VSWhere, require the C++ Workload, and pick the latest version. That would make it quite a lot simpler. I would like to see this make it into a node-gyp release quickly... if GYP3 gets a way to do all of this, we can always move. We also need GYP3 urgently for Python 3 compatibility, but these are independent concerns, so in my opinion the sooner we can get this out to start fixing part of the issues the better. |
It still scares me after all those years. And I wrote those bits 🤦♂. I think it's weakest point is it's not easy to attach a debugger to the JITed output. We did experienced a few bugs when fields changed their format: #1516 and #1198.
Good traceability is good traceability. GYP[3] has tests that require certain payloads installed, and having good logs in case of failure is IMO very valuable.
Of course. I'll port stuff at a later date (if and when etc.). |
|
I'll land this tomorrow if there are no objections.
I tested them quite a lot, you're not running alone here! It was a great idea to use the system C#.
Both of those would still have happened. The fields did not change the format, the name of the components themselves changed. This can always happen with any method that uses component names, but at least now it's much more robust that it was in the initial version. |
PR-URL: #1762 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: #1762 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: #1762 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: #1762 Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: #1762 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Refael Ackermann <refack@gmail.com>
|
Thanks for the reviews! Landed in 721eb69...a20faed |
Checklist
npm install && npm testpassesDescription of change
The message that is currently displayed by node-gyp when Visual Studio can't compile C++ mentions that
VCBuild.exewas not found and recommends installing Visual Studio 2005. When no Visual Studio is installed at all, the message is even more confusing:This PR adds much more detailed information in case of failure and VS2019 support. The
msvs_versionoption of node-gyp/npm can now specify either a version year or the full path of the installation to use. When a wrong value is passed, node-gyp presents a list of options that would have been valid.This adds all the logic to detect Visual Studio in node-gyp, thus always overriding the detection in GYP. I considered adding this in GYP3 instead, but there were several drawbacks:
Tests are included, using real output saved from the PowerShell script on my test machines. This should ensure the correct version is detected, but testing if it can be used correctly can only be done in real machines. I tested this in all supported versions (including 2013 Express), with 32 and 64 bit Node.js.
This also includes a fix for older versions of Visual Studio where the MARMASM items are not available. @jkunkee please let me know if it does not look good to you.
cc @refack @rvagg