[BUG] msvc runtime library not setup at all. about modern-cpp-template HOT 10 CLOSED

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?

from modern-cpp-template.

What do you think?
looks good to me.

from modern-cpp-template.

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.

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.

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 Debug. I am not familiar with this syntax/configuration, but I think it should also be done for Release.

I have the following proposals for you, in order to get your input:

1. CMAKE_MSVC_RUNTIME_LIBRARY must only be set if ${PROJECT_NAME}_ENABLE_VCPKG is ON.
2. Similarly, for Release it should also be set (if it needs to be; here I would love to get your input as I'm kind of out of my league 😄).

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.

from modern-cpp-template.

Hey @filipdutescu, I investigate this issue more. For your specfic feedbacks,
CMAKE_MSVC_RUNTIME_LIBRARY must only be set if ${PROJECT_NAME}_ENABLE_VCPKG is ON.
The RUNTIME_LIBRARY is used to by developer to customize the way how the multithread library is linked. So I think maybe the better way is to only set it if MSVC is defined.
Similarly, for Release it should also be set (if it needs to be; here I would love to get your input as I'm kind of out of my league
I updated my changes to cover all build types morew than debug and release and tested them locally via vscode editor gui.
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.
You can do it. but please refer to my changes more or less.

The below is the v2 changes from me:
Firstly I added a new option ${PROJECT_NAME}_USE_MSVC_MultiThreadDLL in StandardSettings.cmake

#
# Compiler options
#
option(${PROJECT_NAME}_WARNINGS_AS_ERRORS "Treat compiler warnings as errors." OFF)
option(${PROJECT_NAME}_USE_MSVC_MultiThreadDLL "Link dynamically-link mutithread library. This flag only take effect when it is MSVC" OFF)

Then I updated CMakeList.txt with Setup CMAKE_MSVC_RUNTIME_LIBRARY:

#
# Set project options 
#
include(cmake/StandardSettings.cmake)
include(cmake/StaticAnalyzers.cmake)
include(cmake/Utils.cmake)
if(NOT CMAKE_BUILD_TYPE)
  set(CMAKE_BUILD_TYPE "Debug")
endif()
message(STATUS "Started CMake for ${PROJECT_NAME} v${PROJECT_VERSION}...\n")
#
# Setup CMAKE_MSVC_RUNTIME_LIBRARY
#
if(MSVC)
  if( ${PROJECT_NAME}_USE_MSVC_MultiThreadDLL)
    set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>DLL")
  else()
    set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
  endif()
  message(STATUS "Adjusted CMAKE_MSVC_RUNTIME_LIBRARY ${CMAKE_MSVC_RUNTIME_LIBRARY}")
endif()

from modern-cpp-template.

So I think maybe the better way is to only set it if MSVC is defined.

You are correct, I had a brain freeze apparently haha

${PROJECT_NAME}_USE_MSVC_MultiThreadDLL

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.

The below is the v2 changes from me:

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.

from modern-cpp-template.

I would also move the code near the section with project dependencies, since it is a MSVC dependency as far as I can tell.

from modern-cpp-template.

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?
you can refer to the cmake official doc. there are 4 combinations for CMAKE_MSVC_RUNTIME_LIBRARY .
The example only covers two static-version runtime libraries
The story is like this: I installed gtest via vcpkg and the by defaullt it uses MTd runtime library. However, the default runtime library used in my test excutable is MDd. so the build stage failed due to the inconsistency of the runtime libraries. So cmake after v3.15.0 has a new variable called CMAKE_MSVC_RUNTIME_LIBRARY to let developer have ability to specify whatever runtime libary he wants instead of the default MDd.

from modern-cpp-template.

If it is just a must for MSVC then it should indeed be included
Ny feeling is it is a must for msvc expecially when build stage fails. It does resolve an existing build problem for msvc abi and can save the developers a lots of time to get things work.

from modern-cpp-template.

I fail to see how it covers more build types, such as release
accroding to the cmake offical doc , there is postfix of Debug but there is no postfix of Release for runtime library. if it is release, then they are MultiThreaded and MultiThreadedDLL . otherwise, they are MultiThreadedDebug and MultiThreadedDebugDLL

from modern-cpp-template.

used wrong default in CMakeLists.txt

from modern-cpp-template.