-
Notifications
You must be signed in to change notification settings - Fork 214
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
[BUG] msvc runtime library not setup at all. #24
Comments
Hello! First of all, thanks very much for trying out the template and for opening this issue, especially since I primarily use Linux, so I might've never found it otherwise. Now regarding the actual issue.
I would be happy to receive a PR from you, but there are some "issues" with your addition. While fixing the matter at hand, I think it introduces others. #
# Set Multithread Runtime library for MSVC ABI
#
set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>") First of all, neither on Linux, nor Mac will this work, without the proper setup. I also think, as far as my knowledge goes, that most *nix developers do not use VCPKG. Besides, this seems to only work for I have the following proposals for you, in order to get your input:
Now, based on your response and depending on the conclusions we reach, I think we can decide if you should open a PR (which I will be more than happy to receive 🎉) or if I should make a commit. |
Hey @filipdutescu, I investigate this issue more. For your specfic feedbacks, The below is the v2 changes from me:
Then I updated CMakeList.txt with
|
You are correct, I had a brain freeze apparently haha
I think this can be replaced by checking if the detected compiler is MSVC. Why do we need a DLL for this, are there cases where on some Windows versions it is a DLL and on others a static lib? Considering this comment on the exact CMake issue, I think we should not use this DLL linking. This is a template meant to work out of the box for the majority of people, with minor tweaks, so if someone needs DLL they should manually change it (if the static lib solution is the correct one). I think a comment mentioning this above the inserted snippet is enough in this case.
I fail to see how it covers more build types, such as release, maybe I am misunderstanding something? But, taking into consideration the comment mentioned above, it might not be an issue. The more I think about the problem, the more I am wondering if the template should include this by default. If this error occurs only when one is using threads in their code, than I would opt out of including the changes. If it is just a must for MSVC then it should indeed be included. Otherwise, I think it might be better to omit it, since it will mean larger results after compilation. |
I would also move the code near the section with project dependencies, since it is a MSVC dependency as far as I can tell. |
Why do we need a DLL for this, are there cases where on some Windows versions it is a DLL and on others a static lib? |
If it is just a must for MSVC then it should indeed be included |
I fail to see how it covers more build types, such as release |
I say keep the macro, but no option to dynamically link, just statically and based on each user's specific project and setup, they should modify it as they see fit (if your project needs to use DLL, then it is not an option, rather an obligation). In order to make it easy for others to realise this, a comment should suffice. What do you think? |
What do you think? |
used wrong default in CMakeLists.txt |
Describe the bug
cmake build stage failed with the error msg below:
The problem is casued by the missing of setup of msvc runtime library.
To Reproduce
vcpkg.exe install gtest:x64-windows-static
..vscode
in the root folder and create asettings.json
file in.vscode
folder with the below content:Change
D:/vcpkg/scripts/buildsystems/vcpkg.cmake
to your own path.7. config and build the project via vscode gui.
Expected behavior
config and build shoudl succeed. and tests shoudl all passed.
Desktop (please complete the following information):
Additional context
I fixed up the issue by adding this line in my fork
I am happy to create a PR when the issue is discussed and got alignment with the author.
The text was updated successfully, but these errors were encountered: