8000 CMakePresets should be able to specify a VC toolset by version number by sfreed141 · Pull Request #2366 · microsoft/vscode-cmake-tools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Feb 18, 2022
Merged

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

merged 12 commits into from
Feb 18, 2022

Conversation

sfreed141
Copy link
Contributor
@sfreed141 sfreed141 commented Feb 7, 2022 •
edited
Loading

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

@bobbrow bobbrow linked an issue Feb 12, 2022 that may be closed by this pull request
CMakePrests.json toolset requires the VS version instead of the toolset version #1965
Closed
bobbrow
bobbrow approved these changes Feb 15, 2022
View reviewed changes
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.

@bobbrow bobbrow merged commit e7257e2 into main Feb 18, 2022
@bobbrow bobbrow deleted the dev/safreed/presetstoolset branch February 18, 2022 16:22
@Djoulihen Djoulihen mentioned this pull request Mar 12, 2022
Closed
@github-actions github-actions bot locked and limited conversation to collaborat 762F ors Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xisui-MSFT xisui-MSFT xisui-MSFT left review comments

@bobbrow bobbrow bobbrow approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

CMakePrests.json toolset requires the VS version instead of the toolset version
4 participants
@sfreed141 @bobbrow @xisui-MSFT @safreed-msft
0