Fix CMake broken option gtest_force_shared_crt

[charles-lunarg]

CMake 3.15 added MSVC_RUNTIME_LIBRARY, an official mechanism to configure the runtime library used by targets. Setting the property will properly set the MSVC runtime library regardless of the values in the compiler flags variables.

Fixes #4731

[mathstuf]

Can AND NOT DEFINED CMAKE_MSVC_RUNTIME_LIBRARY be added here so that it can be controlled from without using the property's initializer variable?

[charles-lunarg]

I can see the utility of having CMAKE_MSVC_RUNTIME_LIBRARY be respected when set, since that is the 'standard' way to control runtime library behavior in CMake. I should probably add a blurb to the docs to indicate the gtest_force_shared_crt option being superceeded by CMAKE_MSVC_RUNTIME_LIBRARY.

Initially, I was going to push back on the suggestion, citing that setting CMAKE_MSVC_RUNTIME_LIBRARY would cause gtest_force_shared_crt to no longer function which breaks (what I gather to be) the pattern googletest's CMake uses, which is to isolate as much of the external input as possible. But, gtest_force_shared_crt exists only because CMAKE_MSVC_RUNTIME_LIBRARY didn't exist, so in the future its forseeable that gtest_force_shared_crt could be removed.

Actually, putting it like that I wonder if a better PR is to set CMAKE_MSVC_RUNTIME_LIBRARY based on gtest_force_shared_crt, so the default is correct everywhere, rather than needing to set a property on every target individually. Thoughts? (I haven't investigated this, so do let me know if this is not a good idea).

[mathstuf]

How about deferring to CMAKE_MSVC_RUNTIME_LIBRARY if it is set and deprecating gtest_force_shared_crt's use in conjunction with it?

Something like:

if (NOT DEFINED CMAKE_MSVC_RUNTIME_LIBRARY)
  option(gtest_force_shared_crt …)
  if (some_suitable_condition)
    message(DEPRECATION
      "gtest_force_shared_crt is deprecated in preference to `CMAKE_MSVC_RUNTIME_LIBRARY` instead")
  endif ()
  if (gtest_force_shared_crt)
    set(CMAKE_MSVC_RUNTIME_LIBRARY "…")
  else ()
    set(CMAKE_MSVC_RUNTIME_LIBRARY "…")
  endif ()
elseif (DEFINED gtest_force_shared_crt)
  message(DEPRECATION
    "Since CMAKE_MSVC_RUNTIME_LIBRARY is set, gtest_force_shared_crt settings are ignored")
endif ()

[charles-lunarg]

I've done you one better and went full on deprecating gtest_force_shared_crt. It was broken before, no sense restoring 'old' behavior that wasn't working. I'd love extra help refining the README.md to make it clear how to 'use CMAKE_MSVC_RUNTIME_LIBRARY' to fix this problem, as setting an option is straightforward while adding a chunk of code or a command line variable with a complex body isn't. It almost makes me think keeping gtest_force_shared_crt around to simplify using the library is preferable.

[mathstuf]

You can do something like:

MultiThreaded$<$<CONFIG:Debug>:Debug>$<$<OR:$<BOOL:${gtest_force_shared_crt}>,$<BOOL:${BUILD_SHARED_LIBS}>>:DLL>

[charles-lunarg]

I have reverted the change that would deprecate gtest_force_shared_crt and simply add the condition to not set CMAKE_MSVC_RUNTIME_LIBRARY if it is already defined, as you asked.

Would rather this be a "No rock the boat" PR so that it is easy for maintainers to accept.

Edit: Also I reverted the changes to the readme since they were only needed with deprecation.