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

CMakePresets should be able to specify a VC toolset by version number #2366

Merged
merged 12 commits into from
Feb 18, 2022

Conversation

sfreed141
Copy link
Contributor

@sfreed141 sfreed141 commented Feb 7, 2022

This change addresses item #1965

This changes VS installation detection logic for CMakePresets

The following changes are proposed:

  • Update CMakePresets to treat toolset.version to be interpreted as the msvc toolset version, not visual studio version. This is a breaking change but it is the correct interpretation. The latest VS version that matches the requested toolset version and host/target architecture will be used.
  • Respect CMAKE_GENERATOR_INSTANCE as of CMake 3.22 and use that when specified in "cacheVariables" (CMAKE_GENERATOR_INSTANCE only applies when using VS generators).
  • Remove the reliance on kits for sourcing development environment with presets (now presets will query vswhere.exe and run vcvars*.bat itself).

Notes

  • If CMAKE_GENERATOR_INSTANCE is set in a toolchain file we will not respect it. This may or may not cause problems but this is the existing behavior anyways. Simple workaround would be to launch vscode from the desired developer prompt.
  • If CMAKE_GENERATOR_INSTANCE isn't found we can still try to find a valid VS instance to use, however CMake will likely throw an error if it can't find the instance either. I considered setting this after we find our instance (and overriding any that the user supplies) but I don't think modifying the preset isn't very user friendly (and it wouldn't address the next caveat with toolchain files). If anything we could provide a toast asking the user for input here but I'll leave that for a future PR.
  • Might be worthwhile to cache the results from vswhere.exe and vcvars*.bat, but it didn't seem that bad on my machine (with several VS instances installed).
  • I'm not sure how cmake.exe is chosen but it doesn't come from the selected instance that presets will use. This is outside the scope of this PR but it might be nice to make that consistent.

@bobbrow bobbrow changed the title Dev/safreed/presetstoolset CMakePresets should be able to specify a VC toolset by version number Feb 8, 2022
@bobbrow
Copy link
Member

bobbrow commented Feb 8, 2022

  • Might be worthwhile to cache the results from vswhere.exe and vcvars*.bat, but it didn't seem that bad on my machine (with several VS instances installed).

We should already be caching the vswhere results. The cache expires after a certain amount of time, but it's there.

@bobbrow bobbrow added this to the 1.10.0 milestone Feb 8, 2022
@xisui-MSFT
Copy link
Collaborator

LGTM

Copy link
Member

@bobbrow bobbrow left a comment

Choose a reason for hiding this comment

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

We will test this and then merge. There was one comment about the version comparisons, but otherwise, this looks good.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMakePrests.json toolset requires the VS version instead of the toolset version
3 participants