Skip to content

Commit

Permalink
Allow building Catch2 as dynamic library
Browse files Browse the repository at this point in the history 
Also have a check that warns users if they try to combined dynamic
library with hidden visibility, which we do not support.

Closes #2397
Closes #2398
  • Loading branch information
@horenmar
horenmar committed Jun 24, 2022
1 parent 34d9724 commit bea58bf
Showing 1 changed file with 19 additions and 4 deletions.
23 changes: 19 additions & 4 deletions src/CMakeLists.txt
View file
Expand Up @@ -285,9 +285,7 @@ set(REPORTER_SOURCES
)
set(REPORTER_FILES ${REPORTER_HEADERS} ${REPORTER_SOURCES})

# Fixme: STATIC because for dynamic, we would need to handle visibility
# and I don't want to do the annotations right now
add_library(Catch2 STATIC
add_library(Catch2
${REPORTER_FILES}
${INTERNAL_FILES}
${BENCHMARK_HEADERS}
Expand Down Expand Up @@ -340,7 +338,7 @@ target_include_directories(Catch2
)


add_library(Catch2WithMain STATIC
add_library(Catch2WithMain
${SOURCES_DIR}/internal/catch_main.cpp
)
add_build_reproducibility_settings(Catch2WithMain)
Expand Down Expand Up @@ -431,3 +429,20 @@ endif()

list(APPEND CATCH_WARNING_TARGETS Catch2 Catch2WithMain)
set(CATCH_WARNING_TARGETS ${CATCH_WARNING_TARGETS} PARENT_SCOPE)


# We still do not support building dynamic library with hidden visibility
# so we want to check & warn users if they do this. However, we won't abort
# the configuration step so that we don't have to also provide an override.
if (BUILD_SHARED_LIBS)
get_target_property(_VisPreset Catch2 CXX_VISIBILITY_PRESET)
get_target_property(_ExportAll Catch2 WINDOWS_EXPORT_ALL_SYMBOLS)

This comment has been minimized.

Sign in to view

Copy link
@dimztimz

dimztimz Jun 24, 2022

Contributor

You are checking for this target property but you are never setting it. Do you intent for the end user to set it via https://cmake.org/cmake/help/latest/variable/CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS.html ?

IMO it would be better just to set the property here.

This comment has been minimized.

Sign in to view

Copy link
@horenmar

horenmar Jun 25, 2022

Author Member

The ideal state is that the end user uses static library build.

If they do not, then it is up to them to ensure that things work. The issue with WINDOWS_EXPORT_ALL_SYMBOLS is that there are multiple ways it can fail, so I don't want to set it up implicitly without user action.

This comment has been minimized.

Sign in to view

Copy link
@dimztimz

dimztimz Jun 25, 2022 •
edited

Contributor

Ok, but this will complicate usage of Catch2 in one way. Consider a project that uses bundled Catch2, i.e. it uses add_subdirectory(Catch2) and the same project uses BUILD_SHARED_LIBS=YES for its own libraries. After updating to the next unreleased version they will end up with broken builds on MSVC. WINDOWS_EXPORT_ALL_SYMBOLS is a low level detail that probably should not be pushed to the end-users.

This comment has been minimized.

Sign in to view

Copy link
@horenmar

horenmar Jun 27, 2022

Author Member

Hmmm, that's not a bad point.

The issue I have with WINDOWS_EXPORT_ALL_SYMBOLS boils down to the fact that when I last tried to use it for a project, it ended up completely breaking the build, with useless error message to boot (post build step failed).

However, I tested it for Catch2, and currently it works (thankfully Catch2 has no data members that need exporting, only magic statics), so I am willing to always enable it on Windows.

This comment has been minimized.

Sign in to view

Copy link
@dimztimz

dimztimz Jun 27, 2022

Contributor

One more thing, if you want the tests to work with shared library under Windows, because there is no RPATH, you need to add the following code in tests/CmakeLists.txt to copy the dlls in the same directory as SelfTest.exe.

if (BUILD_SHARED_LIBS AND WIN32)
    add_custom_command(TARGET SelfTest PRE_LINK
        COMMAND ${CMAKE_COMMAND} -E copy_if_different $<TARGET_FILE:Catch2>
        $<TARGET_FILE:Catch2WithMain> $<TARGET_FILE_DIR:SelfTest>
    )
endif()
if (MSVC AND NOT _ExportAll
OR _VisPreset STREQUAL "hidden")

message(WARNING
"Catch2 does not support being built as a dynamic library with hidden"
" visibility. You have to ensure that visibility is handled yourself."
)
endif()
endif()

0 comments on commit bea58bf

Please sign in to comment.