-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
build,windows: check for VS version and arch #13485
Conversation
20d804d
to
eb26d19
Compare
@seishun can you verify this fix, I don't have a machine with both VS2015 & VS2017 installed. |
LGTM - cc @joaocgreis @kunalspathak |
@refack This is the error I get with your PR after running
Again, reopening cmd fixes that. |
@seishun thanks, could you paste the first few lines on output from P.S. even better if you remove the |
@refack Here you go:
(Is there a way to collapse text on GitHub?) [refack edited formating] |
10x
Wrap in |
I am able to repro this problem. For me the |
Don't know where they hide the pointer now. |
I added another guard, but I can't test it right now... |
@refack Still the same error.
|
@seishun looks like something is wonky with your VS2017 version:
If you run |
It works just fine if I reopen cmd. Were you unable to reproduce this in your environment? |
No, but I get |
@refack this seems to fix it for me, feel free to add it if it lgty: --- a/vcbuild.bat
+++ b/vcbuild.bat
@@ -167,6 +167,7 @@ echo Looking for Visual Studio 2017
if "_%VSCMD_ARG_TGT_ARCH%_"=="_%target_arch%_" goto found_vs2017
call tools\msvs\vswhere_usability_wrapper.cmd
if "_%VCINSTALLDIR%_" == "__" goto vs-set-2015
+set "VSINSTALLDIR="
set vcvars_call="%VCINSTALLDIR%\Auxiliary\Build\vcvarsall.bat" %vcvarsall_arg%
echo calling: %vcvars_call%
call %vcvars_call% |
|
PR-URL: nodejs#13485 Fixes: nodejs#13398 Reviewed-By: James M Snell <[email protected]>
dcc1fcd
to
780acc2
Compare
Landed in 780acc2 |
PR-URL: #13485 Fixes: #13398 Reviewed-By: James M Snell <[email protected]>
Should this land on v6.x? |
ping @refack |
ping @refack |
@MylesBorins v6.x doesn't have any support for VS2017 yet, so this change alone is a noop. AFAIK there were no issues opened for VS2017 support for v6.x, and the CI/release infra is setup for VS2015 so my estimation is it's not needed. |
Fixes: #13398
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
build,windows