[Bug] Raising the minimum required CMake version to 3.16 breaks macro conan_set_vs_runtime

[Adnn]

After some uninteresting bug chase, we found out the following behaviour:

With cmake_minimum_required(VERSION 3.16), CMake will override any runtime related flag (e.g. /MT) that are explicitly set in the CMake scripts. This notably breaks the conan macro conan_set_vs_runtime, which was doing exactly that.

This is likely due to the introduction of MSVC_RUNTIME_LIBRARY, which is a new way to control Windows runtime. Please see related issue conan-io/cmake-conan#174 , with a suggestion for CMake build helper to set this.

As a side note, we would be tempted to consider that a bug in CMake, since the doc states:

This property has effect only when policy CMP0091 is set to NEW

Yet, the situation is that simply raising a minimal version does break the behaviour as implemented by Conan.

[memsharded]

I think this might be an issue for Conan itself, not for cmake-conan only. Please @czoido have a look when possible.

[Adnn]

@memsharded Sorry, I made a mistake, actually trying to post in the Conan repository!

[czoido]

Hi @Adnn, I'll transfer the issue then

[czoido]

Hi @Adnn,
I have just reproduced the issue and yes, starting in CMake 3.15, unless you specify cmake_policy(SET CMP0091 OLD) the values that Conan is setting are overwritten.
Now the MSVC_RUNTIME_LIBRARY has to be set, instead of setting the MSVC runtime in CMAKE_<LANG>_FLAGS_<CONFIG>.
In case the MSVC_RUNTIME_LIBRARY is not set, CMake will set the variable to MultiThreaded$<$<CONFIG:Debug>:Debug>DLL so this will mostly affect those who want to set the static runtime.

[puetzk]

I think it would be quite appropriate for conan_set_vs_runtime to look at cmake_policy(GET CMP0091) and work by setting CMAKE_MSVC_RUNTIME_LIBRARY (instead of fiddling with CMAKE_<LANG>_FLAGS_<CONFIG> when when the policy is NEW

[puetzk]

As a side note, we would be tempted to consider that a bug in CMake:

The primary effect of cmake_minimum_required is to set policies known as of that version to NEW. If your script doesn't even run with older CMake versions, then it was clearly written for (at least) that minimum version, and CMake policies emulating the behavior of older versions (that you have prohibited anyway) for backward-compatibility are not wanted. https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html#policy-settings.

[Adnn]

The primary effect of cmake_minimum_required is to set policies known as of that version to NEW.

@puetzk Thank you for taking time for the explanation and the link. We were under the wrong impression that CMake behaviour was to keep policies to OLD: this clears things up!

@czoido @memsharded What would be your opinion about having the CMake build helper automatically set CMAKE_MSVC_RUNTIME_LIBRARY, as there is a clear 1-1 mapping? It seems this could be in line with the other variables that are automatically set by the CMake build helper (for example, CMAKE_OSX_DEPLOYMENT_TARGET), while making more recipes "right by default".
@puetzk had an objection here. Yet not every recipe uses the cmake generator, even if they rely on the CMake build helper.

Once again, thank you everyone for taking the time : )

[czoido]

Hi @Adnn,
I'm working in a PR to check the CMP0091 policy and set CMAKE_MSVC_RUNTIME_LIBRARY accordingly with CONAN_LINK_RUNTIME if it's set to NEW

[memsharded]

Fixed in #6626, will be released soon in 1.23

[Adnn]

@czoido Thank you for the quick fix in the CMake generator!

@memsharded We would still argue that some extra step can be taken to actually close this issue: the CMake build helper would be more usable if it also set the CMAKE_MSVC_RUNTIME_LIBRARYdirectly. It seems that could be in line with its current behaviour of directly setting several CMake variables when they directly correspond to a recipe setting.

[memsharded]

@memsharded We would still argue that some extra step can be taken to actually close this issue: the CMake build helper would be more usable if it also set the CMAKE_MSVC_RUNTIME_LIBRARY directly. It seems that could be in line with its current behaviour of directly setting several CMake variables when they directly correspond to a recipe setting.

This is actually opposed to the current design of the helper. Note that it is adding variables like CONAN_CXX_FLAGS if possible, trying to avoid setting CMAKE_CXX_FLAGS and it only does that if explictly requested. In that way, the user can still manage in their own CMakeLists.txt. For example, there are users that will not be calling conan_basic_setup() or conan_set_vs_runtime(), and they will manage themselves via other means, toolchains or whatever. Suddenly injecting direct CMake variables like CMAKE_MSVC_RUNTIME_LIBRARY will most likely break those users.

[slietzau]

It looks like #6626 introduced a bug in conan_get_policy. Instead of setting a output variable, the function sets a variable named policy.
Essentially one has to change

set(policy "${_policy}" PARENT_SCOPE)

to

set(${policy} "${_policy}" PARENT_SCOPE)

Should I open a new issue for that bug?

[memsharded]

Thanks @slietzau

I have created a new issue from your comments, to have a look in a new one. Thanks for reporting.

[memsharded]

@slietzau Fixed in #6982 will be soon released in 1.25.1