aja-video / libajantv2

Open-source library for AJA Video Systems desktop IO cards.
MIT License
22 stars 14 forks source link

CMake commands in Defines.cmake override preprocessor defines for multi-config generators (VS) #9

Open wesselsga opened 8 months ago

wesselsga commented 8 months ago

In the file cmake/Defines.cmake, the following block will not "play nicely" with other projects using multi-config generators for cmake such as Visual Studio because its using CMAKE_BUILD_TYPE which is normally only used for single-config generators:

# Common preprocessor defines
if(CMAKE_BUILD_TYPE MATCHES Debug)
    list(APPEND AJANTV2_TARGET_COMPILE_DEFS
        -DAJA_DEBUG
        -D_DEBUG)
elseif(CMAKE_BUILD_TYPE MATCHES RelWithDebInfo)
    list(APPEND AJANTV2_TARGET_COMPILE_DEFS
        -DNDEBUG)
else()
    list(APPEND AJANTV2_TARGET_COMPILE_DEFS
        -DNDEBUG)
endif()

Basically, the above is forcing #define NDEBUG for other c++ projects that link to ajantv2 in all Visual Studio targets (even Debug). Since NDEBUG defines are built into cmake for visual studio targets it might be better to do something using generator expressions:

target_compile_definitions(${PROJECT_NAME} PUBLIC $<$<CONFIG:DEBUG>:AJA_DEBUG>)

paulh-aja commented 8 months ago

@wesselsga I believe the issue is actually that, in the ajantv2/CMakeLists.txt the current usage of target_compile_definitions is marked PUBLIC. I think that the correct fix is going to be marking ajantv2's compile defs as PRIVATE. In the main branch I've done this for target_include_directories and target_link_libraries but I overlooked it for target_compile_definitions. If you set the target_compile_definitions in ajantv2/CMakeLists.txt to PRIVATE, does it fix your issue?

I do agree that using generator expressions is probably the better way to go, as far as modern CMake style goes. I would like to move towards using generator expressions for this kind of thing in general.

wesselsga commented 8 months ago

@paulh-aja Yes, switching target_compile_definitions to use PRIVATE vs PUBLIC does also solve the issue.

The only (minor) downside really is NDEBUG is still defined for the ajantv2 project itself for Visual Studio (Debug targets) incorrectly. But that probably really isn't a concern for us since we don't manage that code.