Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wesselsga opened this issue Feb 28, 2024 · 2 comments

Comments

@wesselsga
Copy link

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
Copy link
Collaborator

@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
Copy link
Author

wesselsga commented Feb 28, 2024

@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.

sandercox added a commit to sandercox/libajantv2 that referenced this issue Apr 2, 2024
this allows the AJA NTV2 repo to be added as port to vcpkg
and also fixes the issue with include overrides aja-video#9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants