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.

Sign up for GitHub

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

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c5b1aa
build,win: use latest installed VS by default
joaocgreis Jun 25, 2017
aa4a53c
build,win: respect VS version for building addons
joaocgreis Jun 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions vcbuild.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ if /i "%1"=="/?" goto help
set config=Release
set target=Build
set target_arch=x64
set target_env=vs2015
set target_env=
set noprojgen=
set nobuild=
set sign=
Expand Down Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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


if "%config%"=="Debug" set configure_flags=%configure_flags% --debug
if defined nosnapshot set configure_flags=%configure_flags% --without-snapshot
Expand Down Expand Up @@ -164,18 +167,18 @@ if %target_arch%==x64 if %msvs_host_arch%==amd64 set vcvarsall_arg=amd64

@rem Look for Visual Studio 2017
:vs-set-2017
if "%target_env%" NEQ "vs2017" goto vs-set-2015
if defined target_env if "%target_env%" NEQ "vs2017" goto vs-set-2015
echo Looking for Visual Studio 2017
@rem check if VS2017 is already setup, and for the requested arch
if "_%VisualStudioVersion%_" == "_15.0_" if "_%VSCMD_ARG_TGT_ARCH%_"=="_%target_arch%_" goto found_vs2017
@rem need to clear VSINSTALLDIR for vcvarsall to work as expected
set "VSINSTALLDIR="
call tools\msvs\vswhere_usability_wrapper.cmd
if "_%VCINSTALLDIR%_" == "__" goto vs-set-2015
@rem need to clear VSINSTALLDIR for vcvarsall to work as expected
set vcvars_call="%VCINSTALLDIR%\Auxiliary\Build\vcvarsall.bat" %vcvarsall_arg%
echo calling: %vcvars_call%
call %vcvars_call%

if errorlevel 1 goto vs-set-2015
:found_vs2017
echo Found MSVS version %VisualStudioVersion%
set GYP_MSVS_VERSION=2017
Expand All @@ -184,11 +187,10 @@ goto msbuild-found

@rem Look for Visual Studio 2015
:vs-set-2015
if "%target_env%" NEQ "vs2015" goto msbuild-not-found
if defined target_env if "%target_env%" NEQ "vs2015" goto msbuild-not-found
echo Looking for Visual Studio 2015
if not defined VS140COMNTOOLS goto msbuild-not-found
if not exist "%VS140COMNTOOLS%\..\..\vc\vcvarsall.bat" goto msbuild-not-found
echo Found Visual Studio 2015
if defined msi (
echo Looking for WiX installation for Visual Studio 2015...
if not exist "%WIX%\SDK\VS2015" (
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😵

echo Found MSVS version 14.0
set GYP_MSVS_VERSION=2015
set PLATFORM_TOOLSET=v140
goto msbuild-found
Expand Down Expand Up @@ -351,7 +355,7 @@ ssh -F %SSHCONFIG% %STAGINGSERVER% "touch nodejs/%DISTTYPEDIR%/v%FULLVERSION%/no

@rem Build test/gc add-on if required.
if "%build_testgc_addon%"=="" goto build-addons
"%config%\node" deps\npm\node_modules\node-gyp\bin\node-gyp rebuild --directory="%~dp0test\gc" --nodedir="%~dp0."
%node_gyp_exe% rebuild --directory="%~dp0test\gc" --nodedir="%~dp0."
if errorlevel 1 goto build-testgc-addon-failed
goto build-addons

Expand All @@ -376,7 +380,7 @@ if %errorlevel% neq 0 exit /b %errorlevel%
:: building addons
setlocal EnableDelayedExpansion
for /d %%F in (test\addons\*) do (
"%node_exe%" deps\npm\node_modules\node-gyp\bin\node-gyp rebuild ^
%node_gyp_exe% rebuild ^
--directory="%%F" ^
--nodedir="%cd%"
if !errorlevel! neq 0 exit /b !errorlevel!
Expand All @@ -395,7 +399,7 @@ for /d %%F in (test\addons-napi\??_*) do (
)
:: building addons-napi
for /d %%F in (test\addons-napi\*) do (
"%node_exe%" deps\npm\node_modules\node-gyp\bin\node-gyp rebuild ^
%node_gyp_exe% rebuild ^
--directory="%%F" ^
--nodedir="%cd%"
)
Expand Down