build,win: use latest installed VS by default and respect VS version for building addons #13911

joaocgreis · 2017-06-25T20:29:48Z

vcbuild.bat should detect what version of Visual Studio to use, it should simply work without any parameter if any supported version is installed. It should default to the latest version, to match the behavior of node-gyp.

The second commit makes node-gyp use the same Visual Studio version if it is explicitly specified with a vs2015/vs2017 parameter. If no parameter is specified, the detection mechanism built into node-gyp should be used, so that we can still test addons with VS2013 (which is still supported).

Fixes: #13641

I tested this locally on a VM with VS2015 and VS2017 installed. Also tested with an unusable installation of VS2017. @kfarnung can you also try this if you have a machine with both versions at hand?

cc @nodejs/platform-windows

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

Build, Windows.

vcbuild.bat should detect what version of Visual Studio to use, it should simply work without any parameter if any supported version is installed. It should default to the latest version, to match the behavior of `node-gyp`. Fixes: nodejs#13641

When building in machines with multiple versions of Visual Studio installed, node-gyp should respect the vs2015/vs2017 arguments passed to vcbuild.bat instead of relying on its own detection mechanism.

refack · 2017-06-25T21:52:03Z

vcbuild.bat

@@ -201,6 +203,8 @@ if defined msi (
 call "%VS140COMNTOOLS%\..\..\vc\vcvarsall.bat"
 SET VCVARS_VER=140
 if not defined VCINSTALLDIR goto msbuild-not-found
+@rem Visual C++ Build Tools 2015 does not define VisualStudioVersion


Are you sure? I test with "Build Tools 2015" and I get VisualStudioVersion? But anyway next line is good.

"Visual C++ Build Tools 2015" != "Build Tools 2015"... I'm sure VCBT doesn't set this.

I ment "Visual C++ Build Tools 2015", but I might have installed "VS 2015 community" on the machine I'm thinking of... Too many configuration 😵

refack · 2017-06-25T21:56:17Z

vcbuild.bat

@@ -122,6 +122,9 @@ if defined build_release (

 :: assign path to node_exe
 set "node_exe=%config%\node.exe"
+set "node_gyp_exe="%node_exe%" deps\npm\node_modules\node-gyp\bin\node-gyp"
+if "%target_env%"=="vs2015" set "node_gyp_exe=%node_gyp_exe% --msvs_version=2015"
+if "%target_env%"=="vs2017" set "node_gyp_exe=%node_gyp_exe% --msvs_version=2017"


Will this work? the GYP in node-gyp still doesn't know 2017 🤔

It works. node-gyp still uses the workaround to configure 2017 as 2015, but this line should be correct for the future. Right now it triggers this: https://github.com/nodejs/node-gyp/blob/c84a541/lib/configure.js#L93

@joaocgreis Doesn't this only cover the case where "vs2017" or "vs2015" is passed explicitly? Shouldn't we wait until the VS version detection is over before trying to add the argument?

I had the same thought, but if vs20XX was specified explicitly, only that version will be tried, so it's ok to set early.

That's true, I suppose node-gyp will also default to the highest installed version of VS so as long as they both have the same behavior then everything should work?

We set GYP_MSVS_VERSION, node-gyp should respect that as well

joaocgreis · 2017-06-25T22:08:57Z

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/8823/
Result: yellow. The Windows job is green.

refack · 2017-06-25T22:09:57Z

Result: yellow. The Windows job is green.

The yellowness is known.

kfarnung · 2017-06-26T14:49:39Z

@joaocgreis I'll give this a try today and report back.

kfarnung · 2017-06-27T01:00:13Z

@joaocgreis I'm not seeing any issues on my machine. I tried the default case with no arguments as well as explicitly passing vs2015.

refack · 2017-06-27T01:28:45Z

@kfarnung hide vswhere, vcbuild will fail to detect VS2017, but node-gyp will

refack · 2017-06-27T01:29:31Z

Also try vcbuild vs2015 nobuild noprojgen test

vcbuild.bat should detect what version of Visual Studio to use, it should simply work without any parameter if any supported version is installed. It should default to the latest version, to match the behavior of `node-gyp`. PR-URL: #13911 Fixes: #13641 Reviewed-By: Refael Ackermann <[email protected]>

When building in machines with multiple versions of Visual Studio installed, node-gyp should respect the vs2015/vs2017 arguments passed to vcbuild.bat instead of relying on its own detection mechanism. PR-URL: #13911 Reviewed-By: Refael Ackermann <[email protected]>

joaocgreis · 2017-06-30T18:29:55Z

Landed in e6b69b9...c9cf7c2

I confirmed that vcbuild (defaults to vs2017) then vcbuild vs2015 nobuild noprojgen test work as expected. @refack thanks for your review!

refack · 2017-06-30T18:29:55Z

@joaocgreis are release builds still done with vs2015?

joaocgreis · 2017-06-30T18:30:46Z

@refack yes, VS2013 is only for v4 now.

refack · 2017-06-30T18:32:49Z

@refack yes, VS2013 is only for v4 now.

Wanted to make sure v8.x won't get built with VS2017 suddenly.
Maybe nightlies could tough...

joaocgreis · 2017-06-30T18:40:46Z

We don't have release machines with VS2017 yet, and I think it's too early for that. The change will probably be driven by V8. We'll need to transition in a major version. For now, we should build the nightlies with the same compiler, to ensure they are a proper smoke test.

refack · 2017-06-30T18:42:47Z

The change will probably be driven by V8.

FWIW I'm keeping an eye on the chromium mailing list, they are still struggling with bugs in VS2017...

vcbuild.bat should detect what version of Visual Studio to use, it should simply work without any parameter if any supported version is installed. It should default to the latest version, to match the behavior of `node-gyp`. PR-URL: nodejs#13911 Fixes: nodejs#13641 Reviewed-By: Refael Ackermann <[email protected]>

When building in machines with multiple versions of Visual Studio installed, node-gyp should respect the vs2015/vs2017 arguments passed to vcbuild.bat instead of relying on its own detection mechanism. PR-URL: nodejs#13911 Reviewed-By: Refael Ackermann <[email protected]>

vcbuild.bat should detect what version of Visual Studio to use, it should simply work without any parameter if any supported version is installed. It should default to the latest version, to match the behavior of `node-gyp`. PR-URL: #13911 Fixes: #13641 Reviewed-By: Refael Ackermann <[email protected]>

When building in machines with multiple versions of Visual Studio installed, node-gyp should respect the vs2015/vs2017 arguments passed to vcbuild.bat instead of relying on its own detection mechanism. PR-URL: #13911 Reviewed-By: Refael Ackermann <[email protected]>

vcbuild.bat should detect what version of Visual Studio to use, it should simply work without any parameter if any supported version is installed. It should default to the latest version, to match the behavior of `node-gyp`. PR-URL: #13911 Fixes: #13641 Reviewed-By: Refael Ackermann <[email protected]>

When building in machines with multiple versions of Visual Studio installed, node-gyp should respect the vs2015/vs2017 arguments passed to vcbuild.bat instead of relying on its own detection mechanism. PR-URL: #13911 Reviewed-By: Refael Ackermann <[email protected]>

MylesBorins · 2017-08-14T23:48:46Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

saper · 2017-08-17T22:50:32Z

Question: can PlatformToolkit v140 be used with VS2017? if yes, maybe we could use that - produced builds should be compatible with VS2015.

refack · 2017-08-17T23:10:25Z

Question: can PlatformToolkit v140 be used with VS2017? if yes, maybe we could use that - produced builds should be compatible with VS2015.

@saper, currently the released binaries (v6.x, v8.x, and the v9.x nightlies) are explicitly built with VS2015.
The motivation for this patch is for cases when node is build together with the addons test suite, they should be build with the same toolset (and that should default to the highest version installed).

We do need to test compatibility between a node binary built with VS2017, and addons built with VS2015 (the vice-versa combination should work).

joaocgreis added 2 commits June 25, 2017 21:24

build,win: respect VS version for building addons

aa4a53c

When building in machines with multiple versions of Visual Studio installed, node-gyp should respect the vs2015/vs2017 arguments passed to vcbuild.bat instead of relying on its own detection mechanism.

joaocgreis added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Jun 25, 2017

nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Jun 25, 2017

joaocgreis mentioned this pull request Jun 25, 2017

Issues building test addons with both VS2015 and VS2017 installed #13641

Closed

refack approved these changes Jun 25, 2017

View reviewed changes

joaocgreis closed this Jun 30, 2017

addaleax mentioned this pull request Jul 3, 2017

v8.2.0 proposal #13744

Merged

MylesBorins added the backport-requested-v6.x label Aug 14, 2017

MylesBorins mentioned this pull request Aug 14, 2017

backport updates to windows build script to v6.x nodejs/Release#238

Closed

3 tasks

refack mentioned this pull request Sep 14, 2017

Tracking issue: Change 4 Windows 2008 machines to Visual Studio 2017 nodejs/build#883

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build,win: use latest installed VS by default and respect VS version for building addons #13911

build,win: use latest installed VS by default and respect VS version for building addons #13911

joaocgreis commented Jun 25, 2017

refack Jun 25, 2017

joaocgreis Jun 25, 2017

refack Jun 25, 2017

refack Jun 25, 2017

joaocgreis Jun 25, 2017

refack Jun 25, 2017

kfarnung Jun 26, 2017

refack Jun 26, 2017

kfarnung Jun 26, 2017

refack Jun 26, 2017

joaocgreis commented Jun 25, 2017

refack commented Jun 25, 2017

kfarnung commented Jun 26, 2017

kfarnung commented Jun 27, 2017

refack commented Jun 27, 2017

refack commented Jun 27, 2017

joaocgreis commented Jun 30, 2017

refack commented Jun 30, 2017

joaocgreis commented Jun 30, 2017

refack commented Jun 30, 2017

joaocgreis commented Jun 30, 2017

refack commented Jun 30, 2017

MylesBorins commented Aug 14, 2017

saper commented Aug 17, 2017

refack commented Aug 17, 2017

build,win: use latest installed VS by default and respect VS version for building addons #13911

build,win: use latest installed VS by default and respect VS version for building addons #13911

Conversation

joaocgreis commented Jun 25, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaocgreis commented Jun 25, 2017

refack commented Jun 25, 2017

kfarnung commented Jun 26, 2017

kfarnung commented Jun 27, 2017

refack commented Jun 27, 2017

refack commented Jun 27, 2017

joaocgreis commented Jun 30, 2017

refack commented Jun 30, 2017

joaocgreis commented Jun 30, 2017

refack commented Jun 30, 2017

joaocgreis commented Jun 30, 2017

refack commented Jun 30, 2017

MylesBorins commented Aug 14, 2017

saper commented Aug 17, 2017

refack commented Aug 17, 2017