build: fix detection of Visual Studio 2017 #30119

richardlau · 2019-10-25T16:17:54Z

When run in a Visual Studio 2017 command prompt the VCINSTALLDIR
environment variable will be already set and is not cleared by the
tools/msvs/vswhere_usability_wrapper.cmd utility when it fails to
find Visual Studio 2019. This causes vcbuild.bat to incorrectly
assume Visual Studio 2019 and generate an incompatible configuration.

Clearing the value of VCINSTALLDIR before calling the utility fixes
the detection logic.

Fixes: #30118
Refs: #30022

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

Checklist

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

targos · 2019-10-25T18:09:24Z

do you know why it's not found in the Look for Visual Studio 2017 block?

targos · 2019-10-25T18:10:38Z

Maybe we need to change the argument to call tools\msvs\vswhere_usability_wrapper.cmd "[16.0,17.0)"

richardlau · 2019-10-25T18:12:29Z

do you know why it's not found in the Look for Visual Studio 2017 block?

Because it doesn't get there. See the debug in #30118 (comment).

richardlau · 2019-10-25T18:26:20Z

Or to be more specific in this block:

node/vcbuild.bat

Lines 239 to 272 in 11275dc

    
           @rem Look for Visual Studio 2019 
        
           :vs-set-2019 
        
           if defined target_env if "%target_env%" NEQ "vs2019" goto vs-set-2017 
        
           echo Looking for Visual Studio 2019 
        
           call tools\msvs\vswhere_usability_wrapper.cmd "[16.0,17.0)" 
        
           if "_%VCINSTALLDIR%_" == "__" goto vs-set-2017 
        
           if defined msi ( 
        
             echo Looking for WiX installation for Visual Studio 2019... 
        
             if not exist "%WIX%\SDK\VS2017" ( 
        
               echo Failed to find WiX install for Visual Studio 2019 
        
               echo VS2019 support for WiX is only present starting at version 3.11 
        
               goto vs-set-2017 
        
             ) 
        
             if not exist "%VCINSTALLDIR%\..\MSBuild\Microsoft\WiX" ( 
        
               echo Failed to find the WiX Toolset Visual Studio 2019 Extension 
        
               goto vs-set-2017 
        
             ) 
        
           ) 
        
           @rem check if VS2019 is already setup, and for the requested arch 
        
           if "_%VisualStudioVersion%_" == "_16.0_" if "_%VSCMD_ARG_TGT_ARCH%_"=="_%target_arch%_" goto found_vs2019 
        
           @rem need to clear VSINSTALLDIR for vcvarsall to work as expected 
        
           set "VSINSTALLDIR=" 
        
           @rem prevent VsDevCmd.bat from changing the current working directory 
        
           set "VSCMD_START_DIR=%CD%" 
        
           set vcvars_call="%VCINSTALLDIR%\Auxiliary\Build\vcvarsall.bat" %vcvarsall_arg% 
        
           echo calling: %vcvars_call% 
        
           call %vcvars_call% 
        
           if errorlevel 1 goto vs-set-2017 
        
           if defined DEBUG_HELPER @ECHO ON 
        
           :found_vs2019 
        
           echo Found MSVS version %VisualStudioVersion% 
        
           set GYP_MSVS_VERSION=2019 
        
           set PLATFORM_TOOLSET=v142 
        
           goto msbuild-found

On my system:

%VCINSTALLDIR% comes back as C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\ so the if test on L244 doesn't pass and it continues into the Looking for Visual Studio 2019 block.
The WiX test passes because apparently WiX uses the 2017 directory for 2019 too.
The test on L258 doesn't pass because %VisualStudioVersion% is 15.0.
The call %vcvars_call% on L265 succeeds so the if errorlevel test on the next line doesn't match and we drop into found_vs2019.

It looks like on the CI (which uses VS2017) %VCINSTALLDIR% is coming back empty. Perhaps this is a difference between the Professional (which I have) and Community (on the CI) editions of Visual Studio? Or is it something with tools\msvs\vswhere_usability_wrapper.cmd?

richardlau · 2019-10-25T18:43:02Z

It looks like on the CI (which uses VS2017) %VCINSTALLDIR% is coming back empty. Perhaps this is a difference between the Professional (which I have) and Community (on the CI) editions of Visual Studio? Or is it something with tools\msvs\vswhere_usability_wrapper.cmd?

Ah, it's probably because I build Node.js inside a "Developer Command Prompt for VS 2017" command prompt which has set up that environment variable.

richardlau · 2019-10-25T18:51:40Z

I've got a build going at the moment but when it completes I'll try clearing VCINSTALLDIR before we call tools\msvs\vswhere_usability_wrapper.cmd.

richardlau · 2019-10-25T19:38:33Z

@targos I've updated this PR. Turns out all we need to do is clear the value of the VCINSTALLDIR environment variable before calling tools/msvs/vswhere_usability_wrapper.cmd so that the subsequent logic works properly.

nodejs-github-bot · 2019-10-26T03:49:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26202/

targos · 2019-10-27T09:32:07Z

@joaocgreis

joaocgreis

Thanks @richardlau!

joaocgreis · 2019-10-29T00:15:18Z

vcbuild.bat

@@ -240,6 +240,7 @@ if %target_arch%==x86 if %msvs_host_arch%==x86 set vcvarsall_arg=x86
 :vs-set-2019
 if defined target_env if "%target_env%" NEQ "vs2019" goto vs-set-2017
 echo Looking for Visual Studio 2019
+set "VCINSTALLDIR="


Suggested change

set "VCINSTALLDIR="

@rem VS2017 is detected here when VCINSTALLDIR is set (as in VS Command Prompt)

set "VCINSTALLDIR="

To be clear, this is just a suggestion. Feel free to take it, change it as you see fit or leave it.

Added a (different) comment. PTAL.

nodejs-github-bot · 2019-10-31T06:17:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26253/

nodejs-github-bot · 2019-10-31T07:09:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26254/ (✔️)

nodejs-github-bot · 2019-11-05T02:17:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26337/

nodejs-github-bot · 2019-11-05T03:03:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26338/

nodejs-github-bot · 2019-11-05T03:13:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26339/

When run in a Visual Studio 2017 command prompt the `VCINSTALLDIR` environment variable will be already set and is not cleared by the `tools/msvs/vswhere_usability_wrapper.cmd` utility when it fails to find Visual Studio 2019. This causes `vcbuild.bat` to incorrectly assume Visual Studio 2019 and generate an incompatible configuration. Clearing the value of `VCINSTALLDIR` before calling the utility fixes the detection logic. PR-URL: nodejs#30119 Fixes: nodejs#30118 Refs: nodejs#30022 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: João Reis <[email protected]>

richardlau · 2019-11-05T03:59:05Z

Landed in 1f2fdc9

When run in a Visual Studio 2017 command prompt the `VCINSTALLDIR` environment variable will be already set and is not cleared by the `tools/msvs/vswhere_usability_wrapper.cmd` utility when it fails to find Visual Studio 2019. This causes `vcbuild.bat` to incorrectly assume Visual Studio 2019 and generate an incompatible configuration. Clearing the value of `VCINSTALLDIR` before calling the utility fixes the detection logic. PR-URL: #30119 Fixes: #30118 Refs: #30022 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: João Reis <[email protected]>

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 Oct 25, 2019

richardlau mentioned this pull request Oct 25, 2019

vcbuild.bat fails with Visual Studio 2017 #30118

Closed

This comment has been minimized.

Sign in to view

richardlau changed the title ~~build: fix detection of VS 2019 on Windows~~ build: fix detection of VS 2017 on Windows Oct 25, 2019

richardlau force-pushed the vs2017 branch from 906cb2e to 89d2593 Compare October 25, 2019 16:39

richardlau changed the title ~~build: fix detection of VS 2017 on Windows~~ build: fix detection of Visual Studio 2017 Oct 25, 2019

richardlau added the wip Issues and PRs that are still a work in progress. label Oct 25, 2019

richardlau force-pushed the vs2017 branch from 89d2593 to f129125 Compare October 25, 2019 19:36

richardlau removed the wip Issues and PRs that are still a work in progress. label Oct 25, 2019

richardlau force-pushed the vs2017 branch from f129125 to 15e8421 Compare October 25, 2019 19:39

jasnell approved these changes Oct 26, 2019

View reviewed changes

richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 26, 2019

targos approved these changes Oct 27, 2019

View reviewed changes

joaocgreis approved these changes Oct 29, 2019

View reviewed changes

richardlau force-pushed the vs2017 branch 2 times, most recently from dcdd328 to 07c4965 Compare October 31, 2019 05:39

joaocgreis approved these changes Nov 5, 2019

View reviewed changes

richardlau force-pushed the vs2017 branch from 07c4965 to 1f2fdc9 Compare November 5, 2019 03:43

richardlau merged commit 1f2fdc9 into nodejs:master Nov 5, 2019

richardlau deleted the vs2017 branch November 5, 2019 04:03

targos mentioned this pull request Nov 5, 2019

v13.1.0 release proposal #30262

Merged

targos added the dont-land-on-v12.x label Nov 8, 2019

richardlau mentioned this pull request Jan 17, 2020

VS 2019 libuv/libuv#2560

Closed

bingenito mentioned this pull request Oct 28, 2020

vcbuild.bat fails for Visual Studio 2019 if VCINSTALLDIR intentionally pre-defined #35856

Closed

bingenito mentioned this pull request Nov 13, 2020

build: conditionally clear vcinstalldir #36009

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: fix detection of Visual Studio 2017 #30119

build: fix detection of Visual Studio 2017 #30119

richardlau commented Oct 25, 2019 •

edited

Loading

This comment has been minimized.

targos commented Oct 25, 2019

targos commented Oct 25, 2019

richardlau commented Oct 25, 2019

richardlau commented Oct 25, 2019 •

edited

Loading

richardlau commented Oct 25, 2019

richardlau commented Oct 25, 2019

richardlau commented Oct 25, 2019

nodejs-github-bot commented Oct 26, 2019

targos commented Oct 27, 2019

joaocgreis left a comment

joaocgreis Oct 29, 2019

joaocgreis Oct 29, 2019

richardlau Oct 31, 2019

nodejs-github-bot commented Oct 31, 2019

nodejs-github-bot commented Oct 31, 2019 •

edited by richardlau

Loading

nodejs-github-bot commented Nov 5, 2019

nodejs-github-bot commented Nov 5, 2019

nodejs-github-bot commented Nov 5, 2019

richardlau commented Nov 5, 2019

	set "VCINSTALLDIR="
	@rem VS2017 is detected here when VCINSTALLDIR is set (as in VS Command Prompt)
	set "VCINSTALLDIR="

build: fix detection of Visual Studio 2017 #30119

build: fix detection of Visual Studio 2017 #30119

Conversation

richardlau commented Oct 25, 2019 • edited Loading

Checklist

This comment has been minimized.

targos commented Oct 25, 2019

targos commented Oct 25, 2019

richardlau commented Oct 25, 2019

richardlau commented Oct 25, 2019 • edited Loading

richardlau commented Oct 25, 2019

richardlau commented Oct 25, 2019

richardlau commented Oct 25, 2019

nodejs-github-bot commented Oct 26, 2019

targos commented Oct 27, 2019

joaocgreis left a comment

Choose a reason for hiding this comment

joaocgreis Oct 29, 2019

Choose a reason for hiding this comment

joaocgreis Oct 29, 2019

Choose a reason for hiding this comment

richardlau Oct 31, 2019

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 31, 2019

nodejs-github-bot commented Oct 31, 2019 • edited by richardlau Loading

nodejs-github-bot commented Nov 5, 2019

nodejs-github-bot commented Nov 5, 2019

nodejs-github-bot commented Nov 5, 2019

richardlau commented Nov 5, 2019

richardlau commented Oct 25, 2019 •

edited

Loading

richardlau commented Oct 25, 2019 •

edited

Loading

nodejs-github-bot commented Oct 31, 2019 •

edited by richardlau

Loading