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

Update the visual studio version checking logic #12586

Closed
wants to merge 2 commits into from

Conversation

mai93
Copy link
Contributor

@mai93 mai93 commented Nov 30, 2020

This PR updates the _is_vs_2017_or_2019 function to specify the visual studio version based on the contents of the VC folder instead of depending on the parent directories of VC.

This is helpful in cases where BAZEL_VC points to a custom location of VC.

Fixes: #11259

@mai93
Copy link
Contributor Author

mai93 commented Nov 30, 2020

I am not sure if this is the best way to distinguish between the different VS versions based on VC directory. Other solutions that I considered:

  • Since the contents of the VC folder are completely different between VS 2017/2019 and older versions, we can determine the VS version based on the existence of only one of VC subdirectories. For example, if VC contains 'Auxiliary' directory then it is VS 2017 or 2019 and if not then it is an older version.

  • Check for VCVARSALL.BAT script location and determine the VS version based on it, i.e. if {vc_path}\Auxiliary\Build\VCVARSALL.BAT exists this means it is VS 2017 or 2019 while {vc_path}\VCVARSALL.BAT means it is VS 2015 or older.

But I thought it is better to check that the VC subdirectories match the valid layout in VS 2017/2019.

@@ -254,14 +254,17 @@ def find_vc_path(repository_ctx):
auto_configure_warning_maybe(repository_ctx, "Visual C++ build tools found at %s" % vc_dir)
return vc_dir

def _is_vs_2017_or_2019(vc_path):
"""Check if the installed VS version is Visual Studio 2017."""
def _is_vs_2017_or_2019(repository_ctx, vc_path):
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

This check looks much better, according to https://devblogs.microsoft.com/cppblog/compiler-tools-layout-in-visual-studio-15/#where-the-compiler-toolset-moved-to-in-vs-2017, those three directory should always exist for 2017 and later version of Visual C++ Build Tools.

However, since file path is case-insensitive, can you convert all strings to lower case and then do the comparison? Just in case somehow the directory was created with different cases somehow.

Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thank you!

@bazel-io bazel-io closed this in d155376 Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BAZEL_VS / BAZEL_VC don't account for custom install locations well.
2 participants