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

[build] Fix toolchain detection when using Microsoft Build Tools #2220

Conversation

ix-dcourtois
Copy link
Contributor

@ix-dcourtois ix-dcourtois commented Jan 31, 2023

Description of Change(s)

The environment variable used by build_usd.py to detect the version of the MSVC compiler is in fact an environment variable storing the version of Visual Studio, which is the IDE and has a different versioning than the compiler. And when using Microsoft Build Tools, only the compiler and SDKs are installed, and that environment variable doesn't even exist.

This commit just uses the actual environment variable storing the MSVC compiler version (it's setup by both Visual Studio and the Build Tools) and fixes the IsVisualStudio20XXOrGreater helper functions to check against the compiler versions (see https://en.wikipedia.org/wiki/Microsoft_Visual_C%2B%2B the table MSVC++ versions)

Edit: It also skips adding the -A x64 platform option to the CMake command if the generator used isn't a Visual Studio Variant, because only those support that option (it gives an error when using Ninja for instance)

Fixes Issue(s)

  • Fixes build using Microsoft Build Tools instead of Visual Studio.

  • I have verified that all unit tests pass with the proposed changes (not applicable, build issue only)

  • I have submitted a signed Contributor License Agreement

@ix-dcourtois ix-dcourtois force-pushed the fix_microsoft_build_tools_build branch 2 times, most recently from b8c09a8 to 7af4278 Compare January 31, 2023 14:29
@ix-dcourtois ix-dcourtois force-pushed the fix_microsoft_build_tools_build branch from 7af4278 to bbe0059 Compare January 31, 2023 14:33
@tallytalwar
Copy link
Contributor

Filed as internal issue #USD-7950

@ix-dcourtois
Copy link
Contributor Author

Hi, sorry to bother. I'm not sure why the checks failed on Linux: the change only impacts Windows.
Is there anything I can do to fix things / help this get merged more easily?

@spiffmon
Copy link
Member

Hi @ix-dcourtois , we suspect this is because your PR conflicts with PR #2192 , which we pulled down last week, and which should be making its way out to github soon. Once it does, you can rebase your PR upon it, and that should take care of it!

@ix-dcourtois
Copy link
Contributor Author

Ah yes, the fix for Ninja, good to know, thanks!

@pixar-oss pixar-oss merged commit d8d5b73 into PixarAnimationStudios:dev Mar 14, 2024
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 this pull request may close these issues.

4 participants