-
Notifications
You must be signed in to change notification settings - Fork 112
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
Improve Visual Studio detection #38
Conversation
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.
Nice.
I didn't have the registry entry it looks for.
The entry is only in the Wow6432Node hive. cv2pdb is usually a 32-bit binary,
but a 64-bit execuable might fail due to this.
It even says it can detect future versions.
Probably until next version when they come up with just another installation scheme ;-)
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" /> | ||
<ImportGroup Label="ExtensionTargets"> | ||
<Import Project="$(VCTargetsPath)\BuildCustomizations\masm.targets" /> | ||
<Import Project="packages\Microsoft.VisualStudio.Setup.Configuration.Native.1.16.30\build\native\Microsoft.VisualStudio.Setup.Configuration.Native.targets" Condition="Exists('packages\Microsoft.VisualStudio.Setup.Configuration.Native.1.16.30\build\native\Microsoft.VisualStudio.Setup.Configuration.Native.targets')" /> |
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.
I think it would be much easier to just include Setup.Configuration.h into the src folder (the license seems to allow this), so building is independent from nuget packages.
Some people also prefer to compile cv2pdb with tools other than VS, e.g. #37
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.
It is still nice the header can automatically be retrieved from the source. What about including the retrieved package in the repository? Those who would like to know the source or update one can still check package.config
. Those who don't have NuGet just doesn't have to be bothered with it.
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.
What about including the retrieved package in the repository?
Ok, let's try that.
|
||
HKEY hkey; | ||
if (RegOpenKeyExA(HKEY_LOCAL_MACHINE, key, 0, KEY_OPEN_FLAGS, &hkey) != ERROR_SUCCESS) | ||
ISetupConfigurationPtr query; |
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.
I think the registry key version can stay as another option to find VS2017 (after the COM version), so better create a new function.
2e9c386 addresses #38 (comment). |
The new COM API is expected to work on Visual Studio 2017 and newer installations. It is also compatible with non-x86 runtime.
5e1632e also notes about non-x86 environment pointed out in #38 (review). |
Thank you for your contribution. I've added the package contents and made some minor adjustments: 58dab3c |
The new COM API is expected to work on Visual Studio 2017 and newer installations.
Actually the old detection code did not work for me; I didn't have the registry entry it looks for. The COM API works neatly. It is also used by vswhere, Microsoft's official detection tool, and reliable. It even says it can detect future versions.