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: enable build with vs2017 #11852

Closed
wants to merge 1 commit into from
Closed

win: enable build with vs2017 #11852

wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented Mar 14, 2017

Add support for building node with Visual Studio 2017
Dependent upon #12450

[Inspired by @bzoz and #11084]
With this version VS does not use Windows registry any more, instead it installs a COM server and a tools called vswhere that one can query to obtain list of installed modules and their locations - see this blogpost: https://blogs.msdn.microsoft.com/heaths/2017/04/21/vswhere-is-now-installed-with-visual-studio-2017/ by @heaths

I've written a script that wraps vswhere with all the needed arguments for finding a VC++ toolchain, transplanted from node4good/msvs-com-helper

Checklist

  • make vcbuild test (Windows) passes
  • commit message follows commit guidelines
  • update documentation (BUILDING.ms)

Affected core subsystem(s)
win, build, tools

Ref: nodejs/node-gyp#1130, nodejs/node-gyp#1103

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Mar 14, 2017
@refack
Copy link
Contributor Author

refack commented Mar 14, 2017

Ref: boostorg/build#170
Also Build status this CI runs the test suites of gyp.js node-gyp and boost/build on a matrix of VS versions

@refack
Copy link
Contributor Author

refack commented Mar 14, 2017

cc @indutny ;)

@jasnell
Copy link
Member

jasnell commented Mar 14, 2017

@nodejs/build

@mscdex
Copy link
Contributor

mscdex commented Mar 15, 2017

Are we separately maintaining our copy of gyp now or should the changes in tools/gyp be upstreamed somewhere first?

@refack
Copy link
Contributor Author

refack commented Mar 15, 2017

some changes in GYP are long time coming (have landed in the last few months). VS2017 specific code is being reviewed upstream https://chromium-review.googlesource.com/c/453201/
(INHO if the code is in the repository it's our responsibility 🤷)

@refack
Copy link
Contributor Author

refack commented Mar 15, 2017

A better version of GYP lives in https://github.com/nodejs/node-gyp/tree/master/gyp.
🤞for gyp.js to mature

@rvagg
Copy link
Member

rvagg commented Mar 15, 2017

A better version of GYP lives in https://github.com/nodejs/node-gyp/tree/master/gyp.

Should we just copy that version then? Do your changes here diverge it much from the node-gyp version?

@joaocgreis can you comment on this? It looks a bit different to the approach you've taken in node-gyp and I wouldn't mind seeing a unified approach to both.

@refack
Copy link
Contributor Author

refack commented Mar 15, 2017

Rebased on top of GYP version from node-gyp

@joaocgreis
Copy link
Member

Took me a few more hours to finish my work on this, but I had it almost done so I completed testing and open a PR: #11867 . We can now compare and use the best or merge.

About this version, I'd like to see the gyp changes in a separate commit. If a user has multiple versions of VS installed and only one with a C++ compiler, does this filter to choose that one?

About the gyp update, I think it's easier to maintain if we update to the upstream version and float patches, as we've been doing. The gyp from node-gyp is missing some floating patches that we have here, at least those would have to be added (compare https://github.com/nodejs/node-gyp/commits/ae141e1/gyp with https://github.com/nodejs/node/commits/1a210c4/tools/gyp). This would be easier to review in a separate PR. (For reference, there's a good guide about floating patches for V8: https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md)

@refack
Copy link
Contributor Author

refack commented Mar 15, 2017

@joaocgreis In Hebrew we say "rivalry among scholars brings much wisdom" 😉

@refack
Copy link
Contributor Author

refack commented Mar 16, 2017

@joaocgreis so I've refactored the C# file, and now the scripts can pass filter criteria to the C# class (such as if if instance has all requirements for VC compilation) and it's easier to get values via powershell.
As for how to manage the transplanted GYP IMHO a pointer to the commit log in node-gyp is enough.

P.S. I'm working on a PR for node-gyp as I've did some debugging.

@refack
Copy link
Contributor Author

refack commented Mar 16, 2017

@joaocgreis as for the floating .patch files there's nodejs/node-gyp#1122
/cc @bnoordhuis

@indutny
Copy link
Member

indutny commented Mar 17, 2017

What are the major differences between approach here and #11867 ?

@refack
Copy link
Contributor Author

refack commented Mar 17, 2017

I have 3 major reservation

  1. I think that cherry picking changes from node-gyp/gyp is gonna be a PITA.
    (so I transplanted the whole directory from node-gyp, actualy I think we should use GYP:HEAD after CR456360 lands)
  2. Should at least use my new (v1.10.0) GetVS2017Configuration.cs it has gotten a bit of traction, has a simpler interface, and has a few bugs solved.
    (win,build: add Visual Studio 2017 support #11867 uses @joaocgreis patch of one of my older .cs file)
  3. I think you should run vcvarsall.bat in any case.
    (downside, takes a few seconds more in case of nobuild. upside; sets all needed ENV variables for the downstream process, needed anyway for the actual build action to work)

@indutny
Copy link
Member

indutny commented Mar 17, 2017

Alright, 👍 to this PR then.

@seishun
Copy link
Contributor

seishun commented Mar 17, 2017

I'd prefer to avoid shipping a hand-rolled JSON stringifier.

@indutny
Copy link
Member

indutny commented Mar 17, 2017

Should we consider using gyp.js for building node? 😉

If I'll have enough time, I will make gyp.js work without node.js so it will be usable for bootstraping.

@refack refack force-pushed the vs2017 branch 2 times, most recently from 993fe48 to 3d41612 Compare April 25, 2017 21:30
@refack
Copy link
Contributor Author

refack commented Apr 25, 2017

@joaocgreis PTAL. If you approve I'll land.

vcbuild.bat Outdated
@@ -51,7 +51,8 @@ if /i "%1"=="clean" set target=Clean&goto arg-ok
if /i "%1"=="ia32" set target_arch=x86&goto arg-ok
if /i "%1"=="x86" set target_arch=x86&goto arg-ok
if /i "%1"=="x64" set target_arch=x64&goto arg-ok
if /i "%1"=="vc2015" set target_env=vc2015&goto arg-ok
if /i "%1"=="vs2015" set target_env=vs2015&goto arg-ok
if /i "%1"=="vs2017" set target_env=vs2017&goto arg-ok
Copy link
Contributor

Choose a reason for hiding this comment

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

Those should be vc2015 and vc2017

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I won't bikeshed over this if you feel strongly.
But a lllooooonnngggg and boring discussion on the boost mailing list came to a conclusion it should be vs since vc2015 or vc2017 are misnomers, the VC++ versions are either 14.0 and 15.0 or v140 and v141.
Ref: https://lists.boost.org/Archives/boost/2017/03/233597.php

Copy link
Contributor

Choose a reason for hiding this comment

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

People right now can have setups that call vcbuild vc2015, no reason to break those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it as an "undocumented" switch, what do you think:

if /i "%1"=="vc2015"        set target_env=vs2015&goto arg-ok

@@ -144,7 +145,33 @@ if defined noprojgen if defined nobuild if not defined sign if not defined msi g

@rem Set environment for msbuild

set msvs_host_arch=x86
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this go inside :vs-set-2017? It looks like it is only used there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually should be added to the VS2015 case as well, but that's for a different PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also it's kinda consistent with the rem two lines above ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm testing the implication on calling the VS2016 with a host_target arch

vcbuild.bat Outdated
call tools\msvs\vswhere_usability_wrapper.cmd
if "_%VCINSTALLDIR%_" == "__" goto vs-set-2015
set vcvars_call="%VCINSTALLDIR%\Auxiliary\Build\vcvarsall.bat" %vcvarsall_arg%
echo %vcvars_call%
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@refack
Copy link
Contributor Author

refack commented Apr 26, 2017

/cc @nodejs/build @nodejs/platform-windows
Updated PR to use vswhere

@refack
Copy link
Contributor Author

refack commented May 3, 2017

ping @nodejs/build @nodejs/platform-windows @joaocgreis @bzoz @indutny
Comments have been addressed PTAL 🙏

@gibfahn
Copy link
Member

gibfahn commented May 3, 2017

LGTM as long as @seishun @bzoz and @joaocgreis are okay with it.

@joaocgreis
Copy link
Member

Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

a337f63163544df70d86cc318eaf4bda0bf5ae76 LGTM (CI pending)

Nit: casing on the commit title is wrong, make should be lowercase and VS uppercase.

This is missing at least the WiX warning and using the latest installed version that works, but we can improve on that afterwards.

@rem usualy vcvarsall takes an argument: host + '_' + target
set vcvarsall_arg=%msvs_host_arch%_%target_arch%
@rem unless both host and taget are x64
if %target_arch%==x64 if %msvs_host_arch%==amd64 set vcvarsall_arg=amd64
Copy link
Member

Choose a reason for hiding this comment

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

You could use VsDevCmd to avoid having to detect the host arch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda don't trust VsDevCmd 🤷‍♀️ I think it does too much.
Also both of them, for x86 it choose a native compiler over a 64bit host cross compiler:

c:\code$ "c:\bin\dev\VS\Microsoft Visual Studio 2017 Community\Common7\Tools\VsDevCmd.bat" -arch=x86
**********************************************************************
** Visual Studio 2017 Developer Command Prompt v15.0.26403.7
** Copyright (c) 2017 Microsoft Corporation
**********************************************************************

c:\code$ where cl
c:\bin\dev\VS\Microsoft Visual Studio 2017 Community\VC\Tools\MSVC\14.10.25017\bin\HostX86\x86\cl.exe

And apparently that matters: https://chromium-review.googlesource.com/c/486400/#message-04d641d86728a1b1832181033b692eb9a85e55de

Copy link
Contributor Author

Choose a reason for hiding this comment

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

*"does too much" == "runs longer" (probably does .NET setup)

@refack
Copy link
Contributor Author

refack commented May 4, 2017

@bzoz PTAL, vc2015 is retained as an "undocumented" compatibility switch

* Set default to `vs2015` since `vs2017` is not CI-green yet
* changes vcbuild.bat arg from `vc2015` to `vs2015`/`vs2017`
  `vc` as in Visual C++ is actually versions 14.0 or 14.10
  `vs` as in Visual Studio is 2015 or 2017
  Ref: http://lists.boost.org/Archives/boost/2017/03/233597.php 🤦
* keep `vc2015` for backward compatibility but "undocumented"
* tools: transplant vswhere wrapper from `msvs-com-helper`
  Ref: https://github.com/node4good/msvs-com-helper
@refack
Copy link
Contributor Author

refack commented May 4, 2017

@joaocgreis @gibfahn are we waiting for @bzoz? I've addressed his comments...
(this is kind of a PITA for me since I need to float over to all my other branches, and be careful not to leak it there...)

@refack
Copy link
Contributor Author

refack commented May 4, 2017

@gibfahn
Copy link
Member

gibfahn commented May 5, 2017

Landed in 1c93e8c

@gibfahn gibfahn closed this May 5, 2017
gibfahn pushed a commit that referenced this pull request May 5, 2017
* Set default to `vs2015` since `vs2017` is not CI-green yet
* changes vcbuild.bat arg from `vc2015` to `vs2015`/`vs2017`
  `vc` as in Visual C++ is actually versions 14.0 or 14.10
  `vs` as in Visual Studio is 2015 or 2017
  Ref: http://lists.boost.org/Archives/boost/2017/03/233597.php 🤦
* keep `vc2015` for backward compatibility but "undocumented"
* tools: transplant vswhere wrapper from `msvs-com-helper`
  Ref: https://github.com/node4good/msvs-com-helper

PR-URL: #11852
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@refack
Copy link
Contributor Author

refack commented May 5, 2017

Landed in 1c93e8c

Thanks I didn't even notice...

@refack refack deleted the vs2017 branch May 5, 2017 18:34
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
* Set default to `vs2015` since `vs2017` is not CI-green yet
* changes vcbuild.bat arg from `vc2015` to `vs2015`/`vs2017`
  `vc` as in Visual C++ is actually versions 14.0 or 14.10
  `vs` as in Visual Studio is 2015 or 2017
  Ref: http://lists.boost.org/Archives/boost/2017/03/233597.php 🤦
* keep `vc2015` for backward compatibility but "undocumented"
* tools: transplant vswhere wrapper from `msvs-com-helper`
  Ref: https://github.com/node4good/msvs-com-helper

PR-URL: nodejs#11852
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@refack refack removed their assignment Jun 12, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
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. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.