-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Consolidate VS detection logic #49593
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue DetailsConsolidate and improve Visual Studio installation detection logic. Contributes to #1251. At present we use four almost identical code snippets in different script files for detection Visual Studio installation. They all run
|
The Mono failure is #49569. |
Cc @trylek regarding the plan he suggested in #38919 (comment). |
Yes, I hope we can close #1251 with this fix. I was going to check that with @danmoseley. |
Sounds like a fix to me. Thanks for the cleanup! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Just to double check, I don't see a reference to the consolidated script from the base build.ps1/build.sh scripts which makes me believe that managed builds don't fail early, right? I would expect to be able to i.e. build the libs.ref subset without a prereq check. |
OK just saw that your comment refers to the native check in the build-runtime.cmd and in the src/tests/build.cmd script. I think it's fine to unconditionally check for the VS installation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome cleanup, thank you!
Thank you for this cleanup! |
Consolidate and improve Visual Studio installation detection logic. Fixes #1251.
At present we use four almost identical code snippets in different script files for detection Visual Studio installation. They all run
vswhere.exe
, check Visual Studio version, launch Developer Command Prompt, and optionally set VC++ build environment, check DIA SDK's presence, and setCMakePath
environment variable. The goal of this PR is to consolidate that logic in a single script to improve maintainability.The current VS detection logic has the following flaw. If you have several VS installations on the machine, it may choose a VS installation with no VC Tools component present, which will fail any native build. Since DIA SDK is always installed with the VC Tools component, checking for DIA SDK's presence in our scripts is more or less equivalent to checking for the VC Tools component's presence. This PR fixes the described flaw by choosing only among VS installations with the VC Tools component present (still choosing the latest VS if there are many).
One consequence of this PR will be failing the build earlier if there is no VS installation with the VC Tools component present. If we think purely managed builds with no VC Tools installed on the computer is a scenario worth supporting, I can relax the
VSWhere
's filter to require the VC Tools component only when the native architecture is passed toinit-vs-env.cmd
.Below I summarized the relevant steps different scripts would execute before and after my changes.
This script did not exist
VSWhere
and callsVsDevCmd.bat -no_logo
• VC Tools component check guarantees DIA SDK's presence
Only if architecture is passed as an argument:
• Calls
vcvarsall.bat %__VCBuildArch%
• Sets
CMakePath
• Runs
VSWhere
and callsVsDevCmd.bat
(viasetup_vs_tools.cmd
)Only if native build is needed:
• Calls
vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
• Sets
CMakePath
init-vs-env.cmd
instead ofsetup_vs_tools.cmd
in the first step (checking for VC Tools unconditionally instead of
the conditional DIA SDK check)
• Runs
VSWhere
and callsVsDevCmd.bat
(viasetup_vs_tools.cmd
)• Calls
vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
init-vs-env.cmd %__BuildArch%
, which does all the steps on the left (checking for VC Tools instead of DIA SDK)• Runs
VSWhere
and callsVsDevCmd.bat
(viasetup_vs_tools.cmd
)init-vs-env.cmd
instead ofsetup_vs_tools.cmd
• Runs
VSWhere
and callsVsDevCmd.bat
init-vs-env.cmd
• Runs
VSWhere
and callsVsDevCmd.bat -no_logo
• Calls
vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
• Sets
CMakePath
init-vs-env.cmd %__BuildArch%
, which does all the steps on the left (checking for VC Tools instead of DIA SDK)• Calls
init-compiler-and-cmake.cmd $(Platform)
(see the next line)init-vs-env.cmd $(Platform)
• Runs
VSWhere
and callsVsDevCmd.bat -no_logo
• Calls
vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
• Sets
CMakePath
init-vs-env.cmd
• Runs
VSWhere
and callsVsDevCmd.bat
• Calls
vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
• Sets
CMakePath
init-vs-env.cmd %__BuildArch%
, which does all the steps on the left (checking for VC Tools instead of DIA SDK)• Runs
VSWhere
and callsVsDevCmd.bat
(viasetup_vs_tools.cmd
)Only if native build is needed:
• Calls
vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
• Sets
CMakePath
init-vs-env.cmd
instead ofsetup_vs_tools.cmd
in the first step (checking for VC Tools unconditionally instead of
the conditional DIA SDK check)