intel / pcm

Intel® Performance Counter Monitor (Intel® PCM)
BSD 3-Clause "New" or "Revised" License
2.67k stars 459 forks source link

The build failed because of a runtime libraries incompatibility #774

Open artiomn opened 5 days ago

artiomn commented 5 days ago

@rdementi , why do you added /MT$<$<CONFIG:Debug>:d> in the CMakeLists?

It breaks builds with another types of runtime and doesn't work in the most cases, due to library incompatibility. If I comment out the two lines with target_compile_options, the library and my project are built successfully.

Could you remove this options?

    target_compile_options(PCM_STATIC PRIVATE "/MT$<$<CONFIG:Debug>:d>")

    # Graphical perfmon front-end: pcm-lib, pcm-service
    # Files: COMMON_FILES() + pcm-lib.cpp winpmem\winpmem.cpp dllmain.cpp
    file(GLOB PCM_LIB_SOURCES winpmem/winpmem.cpp dllmain.cpp pcm-lib.cpp )
    add_library(pcm-lib SHARED ${COMMON_SOURCES} ${PCM_LIB_SOURCES})
    target_compile_definitions(pcm-lib PRIVATE _WINDOWS _USRDLL PCM_EXPORTS _WINDLL _UNICODE UNICODE)

    target_compile_options(pcm-lib PRIVATE "/MT$<$<CONFIG:Debug>:d>")

P.S.: It's incorrect to use compiler options to specify runtime: CMake has built-in variables and properties for it, but libraries should not to use this.

rdementi commented 5 days ago

Thanks for the suggestion. The change was introduced to reduce the risks of DLL hijacking.

Would you be able to submit a pull request with a CMakeLists.txt change to specify the runtime using the built-in variables and properties?

artiomn commented 5 days ago

Perhaps reducing the risk of DLL hijacking is the responsibility of the application developer?

Several compilers. I'm porting framework to Windows from Linux. Ideally, I should not specify runtime, and everything should be built "out of the box", like in Linux.

But neither MSVS 19 nor MSVS 2022 was able to build due to linking errors.

And I must to set only static MT runtime to build.

This gives rise to several problems:

I just commented lines with target_compile_options(PCM_STATIC PRIVATE "/MT$<$<CONFIG:Debug>:d>").

Correct "CMake-style way" to change runtime is using MSVC_RUNTIME_LIBRARY property, i.e.:

if (MSVC)  # AND ($<COMPILE_LANGUAGE> STREQUAL CXX OR $<COMPILE_LANGUAGE> STREQUAL C))
        set_property(TARGET "${target}" PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>DLL")
    endif()

And variable to set runtime: set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>DLL").

But, please, don't specify runtime in the library.

rdementi commented 5 days ago

thanks for the recipe. We will evaluate it. As a work-around we can offer a new -DNO_STATIC_MSVC_RUNTIME_LIBRARY=1 option which you can use with cmake when compiling pcm.

artiomn commented 5 days ago

Thank you. It's better than nothing. But if you have a real reason to set the runtime library (like formal SDL requirements), perhaps in the future it would be better to add an option where the user can set an arbitrary runtime?