Making CUDA test work with CMAKE_MSVC_RUNTIME_LIBRARY #1317

risa2000 · 2019-09-19T13:36:19Z

Below is the original commit message. Please note that I have tried to preserve the old behavior so with the older CMake (<3.15) or on non MSVC platforms it should work the same.

However for the new CMake I can only test it on my setup (Windows+MSVC+CUDA). Since I have spent quite some time on this, I decided to put it here as a PR even if only for the opportunity to discuss it, or as an example, how the things about the CUDA handling could evolve.

On the other hand, I am not compiling anything with CUDA and {fmt}, it was just that I pulled the new version of {fmt} and it automatically started using CUDA which I also had, and it triggered my interest, but apart from that I have no experience with building CUDA apps. So I may be wrong in my observations.

CMake 3.15 introduced a new way of handling MSVC CRT type definition for
the build: CMAKE_MSVC_RUNTIME_LIBRARY variable.
(https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html)

This is supposed to be the way to go with MSVC CRT selection in new
projects. Using this method however breaks the current CMake script for
CUDA test. The reason is the CUDA test uses FindCUDA CMake module to
detect and set up CUDA support in CMake, which is deprecated since CMake
version 3.10, and which does not support CMAKE_MSVC_RUNTIME_LIBRARY
selector correctly (i.e. it does not propagate the compiler option
related to the CRT).

I did not find a way to "patch" in the correct compiler options, so
(while knowing this feature is only available from CMake 3.15 on) I
decided to change also the way CUDA is handled and instead of using
FindCUDA, used enable_language. Apart from having some nice additional
side-effects, it also fixed the problem with CRT selection.

However, the propagation of the compiler options (and in particular the
options related to C++ standard selection) is still a bit flaky on
Windows+MSVC platform, so it had to be done manually.

The patch makes two things in parallel:

Introduces MSVC_BUILD_STATIC, which, together with CMake version >=
3.15, allows building static version of the 'fmt' lib (and all the
tests).
At the same time, for CMake >= 3.15 it switches handling of CUDA
support from (old) FindCUDA to (new) enable_language, to fix the
problems which the old method has with the new CRT selector for MSVC in
a new CMake.

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

CMake 3.15 introduced a new way of handling MSVC CRT type definition for the build: CMAKE_MSVC_RUNTIME_LIBRARY variable. (https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html) This is supposed to be the way to go with MSVC CRT selection in new projects. Using this method however breaks the current CMake script for CUDA test. The reason is the CUDA test uses "FindCUDA" CMake module to detect and set up CUDA support in CMake, which is deprecated since CMake version 3.10, and which does not support CMAKE_MSVC_RUNTIME_LIBRARY selector correctly (i.e. it does not propagate the compiler option related to the CRT). I did not find a way to "patch" in the correct compiler options, so (while knowing this feature is only available from CMake 3.15 on) I decided to change also the way CUDA is handled and instead of using FindCUDA, used enable_language. Apart from having some nice additional side-effects, it also fixed the problem with CRT selection. However, the propagation of the compiler options (and in particular the options related to C++ standard selection) is still a bit flaky on Windows+MSVC platform, so it had to be done manually. The patch makes two things in parallel: 1) Introduces MSVC_BUILD_STATIC, which, together with CMake version >= 3.15, allows building static version of the 'fmt' lib (and all the tests). 2) At the same time, for CMake >= 3.15 it switches handling of CUDA support from (old) FindCUDA to (new) enable_language, to fix the problems which the old method has with the new CRT selector for MSVC in a new CMake.

Since apparently VERSION_GREATER_EQUAL exists only from CMake 3.7, while Android is using CMake 3.6.

luncliff · 2019-09-20T13:55:09Z

I think the update is much nice than I thought at the moment!

Just like the comment in test/cuda-test/CMakeLists.txt, I did know about the deprecation.

fmt/test/cuda-test/CMakeLists.txt

Lines 5 to 9 in f6afb5e

    
           # See: https://cmake.org/cmake/help/latest/module/FindCUDA.html#replacement 
        
           # 
        
           # However, this requires CMake version 3.10 or higher and we can't be sure most 
        
           # of the CUDA projects are using those. 
        
           #

But decided to be conservative because there is no information about the CUDA users' import pattern and their CMake version management.

I'm sure the changed part will be helpful for the CMake early adapters.

Related Issues

Import test for CUDA project #1285 Compile error under visual c++ 2017 and CUDA 10.1 #1149

In the meantime, I made a mistype in the file. My god... 😱

risa2000 · 2019-09-22T16:43:48Z

Just like the comment in test/cuda-test/CMakeLists.txt, I did know about the deprecation.

Actually I found the comments in the CMake file quite helpful, both to figure out what is going on and to realize the author did it for a reason.

When testing my patch I noticed two differences from the original solution, one positive, one negative.

The negative one was that detecting the CUDA framework by using check_language and enable_language took much more time than FindCUDA package. Fortunately, it has to be run only once.

The positive side effect was that the actual command for nvcc compiler was handled in the exactly same way as the command for the host compiler (i.e. MSVC). So when using ninja generator, the file build.ninja contains nvcc invocation flags (as it does for the host compiler) and when enabling compiler commands, the nvcc command is emitted into compile_commands.json as well. Which makes debugging the nvcc command line quirks much easier.

I have realized there is one thing about my patch which possibly needs to be addressed, but I do not know how. At the moment it enables the new policy for CMAKE_MSVC_RUNTIME_LIBRARY unconditionally regardless the compiler or the platform (and even prints the messages about it). It will be confusing on anything different from Windows+MSVC.

The problem is that this policy needs to be enabled and the configuration set in the top level CMakeLists.txt and before any target is defined. But for example MSVC variable is only defined after the compiler is detected and it happens only after some C/CPP files are defined as source (typically in some target). So technically I need to enable the policy before defining the targets, but I only know which compiler I will run only after defining the targets.

So far the only way to make it only active on a relevant platform I figured out was to use MSVC_BUILD_STATIC as a user flag to activate it:

if ((NOT ${CMAKE_VERSION} VERSION_LESS 3.15) AND MSVC_BUILD_STATIC)
  # Set policy to accept MSVC runtime selector (requires CMake >= 3.15)
  cmake_policy (SET CMP0091 NEW)
  option(MSVC_BUILD_STATIC "Enable building with static CRT." ON)
  set (CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
  message (STATUS "Building with static CRT.")
endif()

But it still seems quite hackish, but at least it won't execute on any other platform, unless the user asks for it. Should I add it to the patch?

luncliff · 2019-09-23T15:37:39Z

@risa2000 I think the PR and patch is good enough.
For the part of CMake, @vitaut might have a plan for it.

vitaut · 2019-09-24T18:56:07Z

Thanks for the PR. I'm fine with the changes to the CUDA test if you both think it's an improvement but I don't think we should set CMAKE_MSVC_RUNTIME_LIBRARY in fmt. This should be controlled by a user.

risa2000 · 2019-09-24T20:10:54Z

@vitaut Ok, I have just verified that it is possible to set the policy and the variable on the command line:

cmake -G "Ninja" -DCMAKE_BUILD_TYPE=Debug -DCMAKE_POLICY_DEFAULT_CMP0091=NEW -DMSVC_BUILD_STATIC=ON -DCMAKE_MSVC_RUNTIME_LIBRARY="MultiThreaded$<$<CONFIG:Debug>:Debug>" ..

Will set the linked runtime to static (debug). So I can drop the logic from the patch.

There is however one test posix-mock-test which does not build with the static runtime. Right now, it is circumvented by setting MSVC_BUILD_STATIC variable (different from CMAKE_MSVC_RUNTIME_LIBRARY). I guess I should keep it there for cases when someone devises a way to build the lib with static CRT without using the new CMAKE_MSVC_RUNTIME_LIBRARY. Would you agree?

The static build can be set on the command line with CMake >= 3.15 by defining the policy and the CMAKE_MSVC_RUNTIME_LIBARY this way: cmake -G <gen> <options> -DCMAKE_POLICY_DEFAULT_CMP0091=NEW -DMSVC_BUILD_STATIC=ON -DCMAKE_MSVC_RUNTIME_LIBRARY="MultiThreaded$<$<CONFIG:Debug>:Debug>" When MSVC_BUILD_DEBUG is set the test 'posix-mock-test' is skipped as it does not build with the static runtime.

vitaut · 2019-09-24T21:34:57Z

Could you disable posix-mock-test when building with static runtime?

risa2000 · 2019-09-24T22:04:28Z

Could you disable posix-mock-test when building with static runtime?

It is difficult to check (if you mean check directly the CRT build configuration and not the additional variable). Someone can hack compiler flags on CMake < 3.15 and add the compiler options for static runtime explicitly. Other (as I) can use the new feature and set CMAKE_MSVC_RUNTIME_LIBRARY instead. I think writing a reliable check for that will be much more complex (and potentially also confusing for anyone reading the CMake file).

There is already a condition if (HAVE_OPEN) which controls whether posix-mock-test is built, but if I override it on command line, something else does not build.

vitaut · 2019-09-25T13:13:49Z

test/CMakeLists.txt

@@ -99,6 +99,9 @@ add_fmt_test(grisu-test)
 target_compile_definitions(grisu-test PRIVATE FMT_USE_GRISU=1)
 add_fmt_test(gtest-extra-test)
 add_fmt_test(format-test mock-allocator.h)
+if (MSVC)
+  target_compile_options(format-test PRIVATE /bigobj)


What is this for?

This one is actually quite strange, I had to rerun all the tests to remember why I added it:
When building the lib in the default config (i.e. with DLL CRT and debug mode), by this:

cmake -G "Ninja" -DCMAKE_BUILD_TYPE=Debug

and with CMake 3.14 (included in Visual Studio 2019), the build fails with an error:

[50/54] Building CXX object test\CMakeFiles\format-test.dir\format-test.cc.obj
FAILED: test/CMakeFiles/format-test.dir/format-test.cc.obj
C:\PROGRA~~2\MICROS~~2\2019\COMMUN~~1\VC\Tools\MSVC\1422~~1.279\bin\Hostx64\x64\cl.exe /nologo /TP -DFMT_LOCALE -DFMT_USE_FILE_DESCRIPTORS=1 -DGTEST_HAS_STD_WSTRING=1 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING=1 -D_VARIADIC_MAX=10 -I..\include -I..\test\gtest -I..\test\gmock -I..\test. /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MDd /ZI /Ob0 /Od /RTC1 /JMC /showIncludes /Fotest\CMakeFiles\format-test.dir\format-test.cc.obj /Fdtest\CMakeFiles\format-test.dir\ /FS -c ..\test\format-test.cc
D:\Work_OSS_risa2000\fmt\test\format-test.cc : fatal error C1128: number of sections exceeded object file format limit: compile with /bigobj

I was surprised you did not catch it on your tests, but added the compiler option to fix this anyway (and forgot about it).

Now when I removed the option again (to figure out why I added it in the first place), to my surprise I noticed that all builds with CMake 3.15 were fine without it (regardless whether I used `CMAKE_MSVC_RUNTIME_LIBRARY setting or not). Then I realized that in your tests you also use CMake 3.15. Apparently the default compiler flags for MSVC Debug build has changed between CMake 3.14 and CMake 3.15.

CMake 3.14 compile flags for Debug

"C:\PROGRA~~2\MICROS~~2\2019\COMMUN~~1\VC\Tools\MSVC\1422~~1.279\bin\Hostx64\x64\cl.exe /nologo /TP -DFMT_LOCALE -DFMT_USE_FILE_DESCRIPTORS=1 -DGTEST_HAS_STD_WSTRING=1 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING=1 -D_VARIADIC_MAX=10 -I..\include -I..\test\gtest -I..\test\gmock -I..\test\. /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MDd /ZI /Ob0 /Od /RTC1 /JMC /Fotest\CMakeFiles\format-test.dir\format-test.cc.obj /FdTARGET_COMPILE_PDB /FS -c D:\Work_OSS\_risa2000\fmt\test\format-test.cc"

CMake 3.15 flags

C:\PROGRA~~2\MICROS~~2\2019\COMMUN~~1\VC\Tools\MSVC\1422~~1.279\bin\Hostx64\x64\cl.exe /nologo /TP -DFMT_LOCALE -DFMT_USE_FILE_DESCRIPTORS=1 -DGTEST_HAS_STD_WSTRING=1 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING=1 -D_VARIADIC_MAX=10 -I..\include -I..\test\gtest -I..\test\gmock -I..\test\. /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MDd /Zi /Ob0 /Od /RTC1 /Fotest\CMakeFiles\format-test.dir\format-test.cc.obj /FdTARGET_COMPILE_PDB /FS -c D:\Work_OSS\_risa2000\fmt\test\format-test.cc

The newer ones misses /JMC flag which should be off by default, but "enabled in all Visual Studio templates" (https://docs.microsoft.com/en-us/cpp/build/reference/jmc?view=vs-2019). I guess CMake wanted to go by VS defaults and then possibly realized that without VS this flag is not very useful and removed it - or it caused the problems described above - or CMake version included in VS is doing and CMake vanilla version does not (CMake 3.15 I am using I built from the source as it is not yet available in VS).

Makes sense. Thanks for the detailed explanation.

vitaut · 2019-09-25T13:40:08Z

Let's leave it as is for now. Merged in ac59d9f. Thanks!

Richard Musil added 3 commits September 19, 2019 14:58

Added a check for CUDA before enabling it.

dfcd43b

Using VERSION_LESS instead of VERSION_GREATER_EQUAL

f6afb5e

Since apparently VERSION_GREATER_EQUAL exists only from CMake 3.7, while Android is using CMake 3.6.

vitaut reviewed Sep 25, 2019

View reviewed changes

vitaut closed this Sep 25, 2019

risa2000 deleted the patch_cuda_msvc_static_crt branch January 10, 2020 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making CUDA test work with CMAKE_MSVC_RUNTIME_LIBRARY #1317

Making CUDA test work with CMAKE_MSVC_RUNTIME_LIBRARY #1317

risa2000 commented Sep 19, 2019

luncliff commented Sep 20, 2019 •

edited

Loading

risa2000 commented Sep 22, 2019

luncliff commented Sep 23, 2019 •

edited

Loading

vitaut commented Sep 24, 2019

risa2000 commented Sep 24, 2019

vitaut commented Sep 24, 2019

risa2000 commented Sep 24, 2019

vitaut Sep 25, 2019

risa2000 Sep 25, 2019 •

edited

Loading

vitaut Sep 25, 2019 •

edited

Loading

vitaut commented Sep 25, 2019

Making CUDA test work with CMAKE_MSVC_RUNTIME_LIBRARY #1317

Making CUDA test work with CMAKE_MSVC_RUNTIME_LIBRARY #1317

Conversation

risa2000 commented Sep 19, 2019

luncliff commented Sep 20, 2019 • edited Loading

Related Issues

risa2000 commented Sep 22, 2019

luncliff commented Sep 23, 2019 • edited Loading

vitaut commented Sep 24, 2019

risa2000 commented Sep 24, 2019

vitaut commented Sep 24, 2019

risa2000 commented Sep 24, 2019

vitaut Sep 25, 2019

Choose a reason for hiding this comment

risa2000 Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

vitaut Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

vitaut commented Sep 25, 2019

luncliff commented Sep 20, 2019 •

edited

Loading

luncliff commented Sep 23, 2019 •

edited

Loading

risa2000 Sep 25, 2019 •

edited

Loading

vitaut Sep 25, 2019 •

edited

Loading