Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Visual Studio detection and add VS2019 support #1762

Closed

Conversation

joaocgreis
Copy link
Member

Checklist
  • npm install && npm test passes
  • tests are included
  • commit message follows commit guidelines
Description of change

The message that is currently displayed by node-gyp when Visual Studio can't compile C++ mentions that VCBuild.exe was not found and recommends installing Visual Studio 2005. When no Visual Studio is installed at all, the message is even more confusing:

MSBUILD : error MSB4132: The tools version "2.0" is unrecognized. Available tools versions are "4.0".

This PR adds much more detailed information in case of failure and VS2019 support. The msvs_version option 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:

  • There is no good way to send information from GYP to node-gyp. Since node-gyp needs to know the location of MSBuild, a new way would have to be made.
  • GYP does not print any output to the console. One of the main points here is to provide good logging in case of failure, so that would have to change.
  • A way would have to be added for node-gyp to specify to GYP what VS versions are supported for the version of Node being used.
  • When no version of VS is found, GYP would have to fail instead of generating for the oldest version.
  • The VS components needed to compile C++ are very specific, so GYP would end up becoming too tied to node-gyp. If we are to make a version of GYP specifically for node-gyp, it makes more sense to remove the detection from there altogether.

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

image


function logWithPrefix (log, prefix) {
function setPrefix(logFunction) {
return (...args) => logFunction.apply(null, [prefix, ...args])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@rvagg
Copy link
Member

rvagg commented May 29, 2019

yeah, i have no idea, meaningless rubber stamp from me

@joaocgreis
Copy link
Member Author

joaocgreis commented May 29, 2019

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/129/
ARM64 Windows CI: https://ci.nodejs.org/view/All/job/nodegyp-test-commit-arm64/9/

@jkunkee
Copy link
Contributor

jkunkee commented May 29, 2019

Looks good to me.

@refack
Copy link
Contributor

refack commented May 29, 2019

I was thinking, why not just bundle a known vswhere binary? It's under MIT, and it can save us a bunch of code.
I plan on making it a feature of GYP3 anyway:
https://github.com/refack/GYP/blob/8806c633810730d0a78138a550f0ce3e5047abfb/gyp/MSVS/__init__.py#L451

P.S.
It has --legacy so we can eliminate all the Registry logic, and it has sort so we don't need the this.findVisualStudio* cascade.

lib/configure.js Outdated
}
if (vsInfo.versionMajor > 15 ||
(vsInfo.versionMajor === 15 && vsInfo.versionMajor >= 9)) {
defaults['msvs_enable_marmasm'] = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is only needed if variables.target_arch == 'arm64'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a fixup, thanks @refack!

@refack
Copy link
Contributor

refack commented May 29, 2019

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! 🥇

@joaocgreis
Copy link
Member Author

@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 was thinking, why not just bundle a known vswhere binary?

I also considered that. vswhere does not print a list of installed components, so we can't filter installations as we do here. The full C++ workload is not needed, and some people want a minimal installation. This can be achieved for example with: cinst -y visualstudio2019buildtools --params="--add Microsoft.VisualStudio.Component.Windows10SDK.17134 --add Microsoft.VisualStudio.Component.VC.Tools.x86.x64 --add Microsoft.VisualStudio.Component.VC.CoreIde" which takes about 5GB. Also, this PowerShell/C# code has been around for a while and working quite well, I don't think we're adding any risk there.

@joaocgreis
Copy link
Member Author

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/

@refack
Copy link
Contributor

refack commented Jun 3, 2019

I also considered that. vswhere does not print a list of installed components, so we can't filter installations as we do here.

For component filtering I use the PowerShell client DLL (it's also bundled in the MSVS installer)
https://github.com/refack/GYP/blob/master/tools/vssetup.powershell/VSQuery.ps1
Still it's very fragile code, I was happy to delegate to a MSVS product.

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.

💯 X 💯

I'll try to figure out a way to port stuff to GYP3... Maybe have python gyp.py --detect-msvs to wrap all the detection stuff, then we can print proper log here.

P.S. mostly because Python has builtin Registry sys-calls...

@joaocgreis
Copy link
Member Author

For component filtering I use the PowerShell client DLL

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'll try to figure out a way to port stuff to GYP3...

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.

@refack
Copy link
Contributor

refack commented Jun 3, 2019

this method with the system C# has been in use for a while without issues

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.
I agree that the bottom line is this works for now. Maybe if a new bug comes in the future we can decide to switch to the precompiled DLL.

I see this more like a way to free GYP3 of this burden.

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.

I would like to see this make it into a node-gyp release quickly...

Of course. I'll port stuff at a later date (if and when etc.).

@joaocgreis
Copy link
Member Author

I'll land this tomorrow if there are no objections.

It still scares me after all those years. And I wrote those bits 🤦‍♂.

I tested them quite a lot, you're not running alone here! It was a great idea to use the system C#.

We did experienced a few bugs when fields changed their format: #1516 and #1198.

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.

joaocgreis added a commit that referenced this pull request Jun 4, 2019
PR-URL: #1762
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
joaocgreis added a commit that referenced this pull request Jun 4, 2019
PR-URL: #1762
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
joaocgreis added a commit that referenced this pull request Jun 4, 2019
PR-URL: #1762
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
joaocgreis added a commit that referenced this pull request Jun 4, 2019
PR-URL: #1762
Reviewed-By: Refael Ackermann <refack@gmail.com>
joaocgreis added a commit that referenced this pull request Jun 4, 2019
PR-URL: #1762
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@joaocgreis
Copy link
Member Author

Thanks for the reviews!

Landed in 721eb69...a20faed

@joaocgreis joaocgreis closed this Jun 4, 2019
@joaocgreis joaocgreis mentioned this pull request Jun 4, 2019
@cclauss cclauss mentioned this pull request Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants