cmake-conan icon indicating copy to clipboard operation
cmake-conan copied to clipboard

conan_cmake_detect_vs_runtime and MSVC_RUNTIME_LIBRARY

Open puetzk opened this issue 4 years ago • 15 comments

CMake 3.15 added a new target property MSVC_RUNTIME_LIBRARY, and no longer places /MD, /MDd, etc into CMAKE__FLAGS if CMP0091 is enabled. This results in conan.cmake not detecting the compiler.runtime.

Conan obviously cannot handle per-target variation, or see targets that aren't defined yet at all, but it would make sense for it to at least follow the defaults controlled by [CMAKE_MSVC_RUNTIME_LIBRARY](https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html#variable:CMAKE_MSVC_RUNTIME_LIBRARY, and the fallback is through the CMAKE_MSVC_RUNTIME_LIBRARY_DEFAULT).

However, the new mechanism can (and by default, does) use a generator-expression in its value, so I'm not sure how conan.cmake should evaluate it (i.e. which of its possible values should be used), especially with CMAKE_MULTI...

puetzk avatar Sep 04 '19 20:09 puetzk

Good point regarding the new CMake variable to control the MSVC runtime.

Additionally, it seems it could make sense if the CMake build helper would automatically translate the Conan compiler.runtime setting to the corresponding value for CMAKE_MSVC_RUNTIME_LIBRARY. Any opinion on this?

Adnn avatar Mar 04 '20 13:03 Adnn

I don't think it's the CMake build helper's job to do that. But the CMake generator fiddles with this in CMAKE_<lang>_FLAGS_<CONFIG> in the conanbuildinfo.cmake conan_set_vs_runtime(), and yes, that should be taught to check cmake_policy(GET CMP0091) and work via CMAKE_MSVC_RUNTIME_LIBRARY when the policy is NEW

puetzk avatar Mar 04 '20 16:03 puetzk

Thanks @puetzk for reporting, the issue is now in the conan repo and pending to be solved soon. https://github.com/conan-io/conan/issues/6624

czoido avatar Mar 04 '20 17:03 czoido

This issue was actually the opposite half of the issue and won't be fixed by a conan change (though conan-io/conan#6624 is also real, and needs a fix in conan). conan_cmake_run has code to try and examine CMAKE_<LANG>_FLAGS_<CONFIG> in order to set compiler.runtime via PROFILE_AUTO

https://github.com/conan-io/cmake-conan/blob/70179860c81f2aad79b21e3262bbe518ac628e0f/conan.cmake#L298-L318 https://github.com/conan-io/cmake-conan/blob/70179860c81f2aad79b21e3262bbe518ac628e0f/conan.cmake#L229

if a project using conan.cmake is using CMP0091 NEW this auto-detection needs to look at CMAKE_MSVC_RUNTIME_LIBRARY instead.

puetzk avatar Mar 04 '20 17:03 puetzk

@czoido Sorry for the confusion due to my posting in the wrong repository. This issue is a separate and valid issue, actually impacting cmake-conan.

Adnn avatar Mar 05 '20 09:03 Adnn

The issue is still persisting. As I'm also depending on building my projects with static runtime I also use CMAKE_MSVC_RUNTIME_LIBRARY to select the static runtime setting it to MultiThreaded$<$CONFIG:Debug:Debug>.

I patched function conan_cmake_detect_vs_runtime in conan.cmake like this:

function(conan_cmake_detect_vs_runtime result)
    set(cmp_ninetyone OLD)
    if(POLICY CMP0091)
        cmake_policy(GET CMP0091 cmp_ninetyone)
    endif()
    string(TOUPPER ${CMAKE_BUILD_TYPE} build_type)
    set(static_runtime OFF)
    if(cmp_ninetyone STREQUAL "NEW")
        if(NOT "${CMAKE_MSVC_RUNTIME_LIBRARY}" STREQUAL "")
            string(STRIP ${CMAKE_MSVC_RUNTIME_LIBRARY} runtime_library)
            string(TOUPPER ${CMAKE_MSVC_RUNTIME_LIBRARY} runtime_library_upper)
            string(LENGTH ${runtime_library_upper} runtime_library_length)
            if(${runtime_library_length} GREATER_EQUAL 3)
                math(EXPR runtime_library_begin "${runtime_library_length}-3")
                string(SUBSTRING ${runtime_library_upper} ${runtime_library_begin} 3 runtime_library_tail)
                if(NOT "${runtime_library_tail}" STREQUAL "DLL")
                    set(static_runtime ON)
                endif()
            endif()
        endif()
    else()
        set(variables CMAKE_CXX_FLAGS_${build_type} CMAKE_C_FLAGS_${build_type} CMAKE_CXX_FLAGS CMAKE_C_FLAGS)
        foreach(variable ${variables})
            if(NOT "${${variable}}" STREQUAL "")
                string(REPLACE " " ";" flags ${${variable}})
                foreach (flag ${flags})
                    if(${flag} STREQUAL "/MD" OR ${flag} STREQUAL "/MDd" OR ${flag} STREQUAL "/MT" OR ${flag} STREQUAL "/MTd")
                        string(SUBSTRING ${flag} 1 -1 runtime)
                        set(${result} ${runtime} PARENT_SCOPE)
                        return()
                    endif()
                endforeach()
            endif()
        endforeach()
    endif()
    if(${static_runtime} STREQUAL "ON")
        if(${build_type} STREQUAL "DEBUG")
            set(${result} "MTd" PARENT_SCOPE)
        else()
            set(${result} "MT" PARENT_SCOPE)
        endif()
    else()
        if(${build_type} STREQUAL "DEBUG")
            set(${result} "MDd" PARENT_SCOPE)
        else()
            set(${result} "MD" PARENT_SCOPE)
        endif()
    endif()
endfunction()

For me this is working, but I only did test on Windows/VS (but it's a function used only on Windows anyhow). I can send a patch or clone and issue a pull request.

Without correct cmake support for static runtime linkage conan unfortunately is not usable.

aholzinger avatar Jan 06 '21 15:01 aholzinger

@aholzinger Like you, I still have the issue on my project. I tried your patch and it fixed it perfectly. Could you make a pull request for adding that to cmake-conan ? I think it will avoid people to get this issue again as CMP0091 is now in the CMake standard.

Alexis-Gentil avatar Jan 26 '21 09:01 Alexis-Gentil

@Alexis-Gentil I didn't yet fork Conan. So I have to do this first an then merge the patch. Will take some time :-(

aholzinger avatar Jan 26 '21 10:01 aholzinger

~~Any progress on this? Multi configs are broken right now as debug builds will not link due to missmatched _ITERATOR_DEBUG_LEVEL (#268). This is actually huge, if you have to tell that VS support is not working currently...~~

Edit: It looks like it is a result on how conan_target_link_libraries works. sigh

IceflowRE avatar Apr 27 '21 18:04 IceflowRE

However, the new mechanism can (and by default, does) use a generator-expression in its value, so I'm not sure how conan.cmake should evaluate it (i.e. which of its possible values should be used), especially with CMAKE_MULTI...

I think it could be possible to detect this nearly correctly, even in the face of generator expressions and multi config generators, with some assumptions made:

  • CMAKE_MSVC_RUNTIME_LIBRARY isn't redefined at various points
  • CMAKE_MSVC_RUNTIME_LIBRARY's generator expression isn't target dependent (i.e. evaluates to the same thing project-wide)

The idea would be to have cmake evaluate the generator expression for us, using file(GENERATE). Unfortunately, this doesn't actually happen until generation time, so it would require spawning a cmake subprocess on a small dummy CMakeLists.txt like:

cmake_minimum_required(VERSION 3.12)
project(bla)
file(GENERATE OUTPUT "msvc_runtime_$<CONFIG>.txt" CONTENT "${CMAKE_MSVC_RUNTIME_LIBRARY}")

Care would have to be taken to forward all necessary settings to the subprocess, (i.e. CMAKE_GENERATOR, CMAKE_BUILD_TYPE/CMAKE_CONFIGURATION_TYPES, CMAKE_MSVC_RUNTIME, CMAKE_C_COMPILER, CMAKE_CXX_COMPILER, possibly CMAKE_MAKE_PROGRAM, ...).

Once this finishes, we can loop over CMAKE_CONFIGURATION_TYPES and read out the evaluated generator expressions! 🎉

However,

this is will take a bit of work, and is potentially quite a brittle approach.

I like @aholzinger's idea, maybe make it a little stricter, and provide an opt-out/user override:

  • if CMAKE_MSVC_RUNTIME_LIBRARY is unset, or set to the default MultiThreaded$<$<CONFIG:Debug>:Debug>DLL, assume dynamic runtime
  • if CMAKE_MSVC_RUNTIME_LIBRARY is set to MultiThreaded$<$<CONFIG:Debug>:Debug>, assume static runtime
  • for any other value, show an error or warning, and ask the user to specify the compiler.runtime setting explicitly?

melak47 avatar Mar 10 '22 14:03 melak47

I recently ran into this problem, and also recently wrote some code that might help solve it. Basically my code is two cmake functions that can be used to evaluate properties with $<$<CONFIG:X>:Y> generator expressions at configure time.

First, there's a function to get all the applicable configurations. For example, if we do:

cesium_get_value_configurations("Applies Always $<$<CONFIG:Debug>:Only Debug>$<$<CONFIG:Release>:Only Release>" TEST_OUT)

Then TEST_OUT will be set to the value *;Debug;Release because the value varies in the Debug and in the Release configuration. The * indicates that some parts of the value exist in all configurations.

Then, we can "evaluate" the property for a given configuration:

cesium_get_configuration_value("Applies Always $<$<CONFIG:Debug>:Only Debug>$<$<CONFIG:Release>:Only Release>" "Debug" TEST_OUT)

And the value of TEST_OUT will be Applies Always Only Debug.

So this is a way of evaluating the properties with generator expressions at configure time as long as it only includes CONFIG generator expressions. Other generator expressions are removed.

That won't solve every use case, but I think it will hit a lot of them.

Here are the two functions:

# Given a `value`, which is a string or list where some parts or items are
# wrapped in $<$<CONFIG:X>:Y> generator expressions, returns in outputVariable
# the  complete list of configurations X for which the value varies. If the value
# has a meaningful part (not just a semicolon) that exists for all configurations,
# the outputVariable will contain an element "*".
# Not supported:
#  - CONFIG generator expressions with a list of configurations (instead of just one)
#  - The effect of MAP_IMPORTED_CONFIG_<CONFIG>
function(cesium_get_value_configurations value outputVariable)
  set(INPUT "${value}")
  set(OUTPUT "")
  set(ADDED_STAR "") # Have we added the "*" configuration to the list yet?
  set(CONFIGURATION_REGEX "\\$<\\$<CONFIG:([^>]+)>:([^>]*)>")

  while (INPUT MATCHES ${CONFIGURATION_REGEX})
    # Remove from the beginning through to the matched part
    string(FIND "${INPUT}" "${CMAKE_MATCH_0}" MATCH_START)
    string(LENGTH "${CMAKE_MATCH_0}" MATCH_LENGTH)
    math(EXPR MATCH_END "${MATCH_START}+${MATCH_LENGTH}")
    string(SUBSTRING "${INPUT}" 0 ${MATCH_START} REMOVED)
    string(STRIP "${REMOVED}" REMOVED)
    if (NOT ADDED_STAR AND REMOVED AND NOT REMOVED STREQUAL ";")
      list(APPEND OUTPUT "*")
      set(ADDED_STAR "yes")
    endif()
    string(SUBSTRING "${INPUT}" ${MATCH_END} -1 INPUT)
    # MATCH_1: configuration, MATCH_2: value for configuration
    list(APPEND OUTPUT "${CMAKE_MATCH_1}")
  endwhile()

  if (NOT ADDED_STAR AND INPUT AND NOT INPUT STREQUAL ";")
    list(APPEND OUTPUT "*")
    set(ADDED_STAR "yes")
  endif()

  set(${outputVariable} "${OUTPUT}" PARENT_SCOPE)
endfunction()

# Gets the value of a property by evaluating $<$<CONFIG:X>:Y> generator
# expressions for X=configuration and removing all other generator expressions.
function(cesium_get_configuration_value value configuration outputVariable)
  set(INPUT "${value}")
  set(OUTPUT "")
  set(CONFIGURATION_REGEX "\\$<\\$<CONFIG:${configuration}>:([^>]*)>")

  while (INPUT MATCHES ${CONFIGURATION_REGEX})
    # Add everything up to the matched part to the output, but strip
    # out any generator expressions from it.
    string(FIND "${INPUT}" "${CMAKE_MATCH_0}" MATCH_START)
    string(SUBSTRING "${INPUT}" 0 ${MATCH_START} BEFORE_MATCH)
    string(APPEND OUTPUT "${BEFORE_MATCH}")

    # Add the content of the matched configuration generator
    # expression to the output.
    list(APPEND OUTPUT "${CMAKE_MATCH_1}")

    # Remove the processed part from the INPUT string
    string(LENGTH "${CMAKE_MATCH_0}" MATCH_LENGTH)
    math(EXPR MATCH_END "${MATCH_START}+${MATCH_LENGTH}")
    string(LENGTH "${CMAKE_MATCH_0}" MATCH_LENGTH)
    string(SUBSTRING "${INPUT}" ${MATCH_END} -1 INPUT)
  endwhile()

  # Append any remaining INPUT to the end, minus any generator expressions
  string(APPEND OUTPUT "${INPUT}")
  string(GENEX_STRIP "${OUTPUT}" OUTPUT)

  set(${outputVariable} "${OUTPUT}" PARENT_SCOPE)
endfunction()

I'm happy to open a pull request to apply this technique in conan_cmake_detect_vs_runtime if it sounds like an acceptable approach to the maintainers.

kring avatar Mar 17 '22 02:03 kring

that looks great. should work to deal with the effect of that policy change

earonesty avatar Apr 08 '22 22:04 earonesty

Are there any updates on this issue? Does a PR exist already? Does it need refinement?

tim-goto avatar Feb 09 '23 13:02 tim-goto

Moving to the new toolchains this feature already appears to be implemented https://github.com/conan-io/conan/blob/e0a8ee058bc8dc2d9811b8aeb6999f69aeb78d85/conan/tools/cmake/toolchain/blocks.py#L100

So perhaps if this is required upgrading to the new Generators would be good option. Since those will carry over to Conan 2.0 and new features are not planned for 1.x. (see the readme for reference)

prince-chrismc avatar Feb 10 '23 16:02 prince-chrismc

Thanks for your reply! So instead of cmake-conan use the CMake Toolchain generator? That did not fit into our workflow because of other issues but I will take another look..

tim-goto avatar Feb 13 '23 07:02 tim-goto