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

win,build: add Visual Studio 2017 support #11084

Closed
wants to merge 3 commits into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Jan 31, 2017

Add support for building node with Visual Studio 2017

With this version VS does not use Windows registry any more, instead it installs a COM server that one can query to obtain list of installed modules and their locations - see this blogpost: https://blogs.msdn.microsoft.com/heaths/2016/09/15/changes-to-visual-studio-15-setup/.

Python does not support communicating with COM servers directly. Chromium project when updating GN did not add extra library, instead assuming default installation path with environment variable override (see https://cs.chromium.org/chromium/src/build/vs_toolchain.py?q=vs_toolchain&sq=package:chromium&l=150). I've used the same method to update GYP - default installation path and default Windows SDK version with override. PR for GYP has been also opened upstream: https://chromium-review.googlesource.com/#/c/433540/

To ease usage for node users, a Python script that can obtain VS installation path and installed SDK version is provided. It depends on comtypes Python library (https://github.com/enthought/comtypes) which adds support for COM to Python. This script is in turn used by vcbuild to set proper environment variables overrides.

For easier review this PR is split into 3 commits:

  • First commit adds comtypes Python library to tools. It is needed to query VS COM server about installation paths.

  • Second commit adds support for VS2017 to gyp. Similar to the way it was implemented in gn, it assumes that VS2017 Professional was installed in the default location. User can use two environment variables to overwrite this: VS2017_INSTALL for installation path and VS2017_SDK for installed SDK version. This patch was also commited upstream: https://chromium-review.googlesource.com/#/c/433540/

  • Third commit adds support for VS2017 to vcbuild.bat. We use a Python script that queries VS COM server to determinate installed VS2017 modules and their location.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

win, build, tools

Ref: nodejs/node-gyp#1056

/cc @nodejs/build @nodejs/platform-windows

Add comtypes (http://starship.python.net/crew/theller/comtypes/) Python
library to tools. Library provides COM support for Python, which we use to
locate Visual Studio 2017 installation path.
Add support for creating VS2017 projects to gyp.
Add support for Visual Studio 2017. Python script is used to query
COM server about location of VS installation and installed SDK
version. Those are passed to GYP as environment variables.
@bzoz bzoz added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Jan 31, 2017
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jan 31, 2017
@bzoz
Copy link
Contributor Author

bzoz commented Jan 31, 2017

node-gyp PR: nodejs/node-gyp#1101

@jasnell
Copy link
Member

jasnell commented Feb 17, 2017

@nodejs/build @bnoordhuis

@joaocgreis
Copy link
Member

This is pending on the discussion in nodejs/node-gyp#1101 and nodejs/node-gyp#1103 . It would be good to use the same strategy here.

@joaocgreis joaocgreis added the wip Issues and PRs that are still a work in progress. label Feb 20, 2017
@rvagg
Copy link
Member

rvagg commented Mar 14, 2017

tested and this works as it should, but should we be opting for the C# solution as per nodejs/node-gyp#1130 instead? Much nicer than having to pull in the whole comtypes library if that's all we need. In fact, we could even reach down in to deps/npm/node_modules/node-gyp/lib for that, a little bit gross but zero duplication, we'd just have to fast-track the node-gyp upgrade in our bundled npm ourselves unless npm gets on it early.

@bzoz
Copy link
Contributor Author

bzoz commented Mar 14, 2017

@rvagg yes we should use nodejs/node-gyp#1130 method

@joaocgreis
Copy link
Member

I'm preparing a PR using that method

@refack
Copy link
Contributor

refack commented Mar 14, 2017

P.S. I'm pushing a very light weight VS2017 patch to gyp

@refack
Copy link
Contributor

refack commented Mar 14, 2017

PTAL #11852

@refack refack mentioned this pull request Mar 14, 2017
3 tasks
@rvagg
Copy link
Member

rvagg commented Mar 15, 2017

probably best close this one out @bzoz

@bzoz bzoz closed this Mar 15, 2017
@bzoz
Copy link
Contributor Author

bzoz commented Mar 15, 2017

Right. Closing.

@AndrewPardoe
Copy link

The officially supported vswhere tool is now available as part of the install starting today with Visual Studio 15.2 preview 2: https://blogs.msdn.microsoft.com/heaths/2017/04/21/vswhere-is-now-installed-with-visual-studio-2017/. This doesn't fix all the existing installs of VS 2017 but going forward you can now detect the VS install locations and whether C++ tools are available from a scripted environment. I hope this helps your scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants