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

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

Closed
bingenito opened this issue Oct 28, 2020 · 2 comments · Fixed by #36009
Closed

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

bingenito opened this issue Oct 28, 2020 · 2 comments · Fixed by #36009

Comments

@bingenito
Copy link
Contributor

bingenito commented Oct 28, 2020

  • Version: Found when internally onboarding now active LTS 14.x
  • Platform: windows
  • Subsystem: build

What steps will reproduce the bug?

We have a build environment that does not have any Visual Studio installed. As part of bootstrapping vcbuild we explicitly set paths to build tooling as well as locally nuget install other required packages into the build directory. The change in PR #30119 causes VS version detection to now fail because VCINSTALLDIR which we are explicitly setting based on our environment is being cleared.

What is the expected behavior?

If you run "vcbuild vs2019 ..." it doesn't try and fall through to detect vs2017 since a specific version was requested so nothing needs to be cleared for that detection to work.

Additional information

Suggested fix is that if target_env is defined and is "vs2019" then do not clear VCINSTALLDIR. I can create the pull request if it seems like the right direction.

@richardlau
Copy link
Member

cc @nodejs/platform-windows
(FWIW I no longer have access to the Windows dev environment I had for #30119 so am not going to be able to test any changes.)

@bingenito
Copy link
Contributor Author

I tested locally with it wrapped as:

if not defined target_env set "VCINSTALLDIR="

If target_env is set it can only get to that line if it is vs2019 and if it isn't set I would assume it means you want it to auto detect and want it to do that fall through that #30119 was fixing. It seems low touch but I do worry about all the environment possibilities out there considering what we are doing ourselves. Another possible fix is adding a new env override to skip the clear and to keep simpler backcompat but it seems excessive.

@Trott Trott closed this as completed in 4b0308a Nov 13, 2020
codebytere pushed a commit that referenced this issue Nov 22, 2020
For scenario where target env is explicitly specified as vs2019, do
not clear VCINSTALLDIR which was being cleared to handle fallback to
vs2017 block when attempting to find a matching available VS.

Fixes: #35856

PR-URL: #36009
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this issue Dec 9, 2020
For scenario where target env is explicitly specified as vs2019, do
not clear VCINSTALLDIR which was being cleared to handle fallback to
vs2017 block when attempting to find a matching available VS.

Fixes: #35856

PR-URL: #36009
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this issue Dec 10, 2020
For scenario where target env is explicitly specified as vs2019, do
not clear VCINSTALLDIR which was being cleared to handle fallback to
vs2017 block when attempting to find a matching available VS.

Fixes: #35856

PR-URL: #36009
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this issue Dec 15, 2020
For scenario where target env is explicitly specified as vs2019, do
not clear VCINSTALLDIR which was being cleared to handle fallback to
vs2017 block when attempting to find a matching available VS.

Fixes: #35856

PR-URL: #36009
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants