Fix 'crashpad' Remove NOT condition for SENTRY_BUILD_SHARED_LIBS as it blocks adding crashpad handler target.cmake

[podlaszczyk]

When sentry lib is built as shared library (SENTRY_BUILD_SHARED_LIBS = ON) - that is default value
then sentry_crashpad-targets.cmake is not included.
Thus
$<TARGET_FILE:sentry_crashpad::crashpad_handler>
won't work.

Open question:

1. Why sentry_crashpad-targets.cmake used to be included only for static library?
2. Could it be included for both static and dynamic?
3. improvement: find_dependency for ZLIB instead of find_package - as we are calling from *.cmake file
   https://cmake.org/cmake/help/latest/module/CMakeFindDependencyMacro.html

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.86%. Comparing base (235f837) to head (e936429).
Report is 6 commits behind head on master.

❗ Current head e936429 differs from pull request most recent head 3edd077

Please upload reports for the commit 3edd077 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1007      +/-   ##
==========================================
- Coverage   86.53%   83.86%   -2.67%     
==========================================
  Files          40       53      +13     
  Lines        3736     5443    +1707     
  Branches        0     1207    +1207     
==========================================
+ Hits         3233     4565    +1332     
- Misses        503      764     +261     
- Partials        0      114     +114     

[supervacuus]

Why sentry_crashpad-targets.cmake used to be included only for static library?

This is a bug.

Could it be included for both static and dynamic?

Yes.

improvement: find_dependency for ZLIB instead of find_package - as we are calling from *.cmake file
https://cmake.org/cmake/help/latest/module/CMakeFindDependencyMacro.html

Yes, that makes sense.

Thanks a lot for your contribution ❤️

[podlaszczyk]

In sentry-config.cmake.in there should be probably also following include that defines FindDependency. I am not sure if project includes this macro somewhere.

#sentry-config.cmake.in
...
include(CMakeFindDependencyMacro)

...
find_dependency(...)

[supervacuus]

We will have to add it to the crashpad subproject too. I will finalize it there.

[dg0yt]

REQUIRED is only to be used with find_package. With find_dependency(foo REQUIRED), it breaks continuing search in different locations, and also optional search.

[supervacuus]

REQUIRED is only to be used with find_package. With find_dependency(foo REQUIRED), it breaks continuing search in different locations, and also optional search.

Thanks, @dg0yt, TIL! The docs only mention that REQUIRED will be forwarded from the original find_package(). I was not aware of the side effects of passing it explicitly because I mistakenly thought the argument would be ignored, but seemingly it passes them on.

[dg0yt]

It takes a few more changes to properly support config search mode:
All find_dependendency calls must be done before including the exported cmake files.

[supervacuus]

It takes a few more changes to properly support config search mode: All find_dependendency calls must be done before including the exported cmake files.

Thanks a lot! To be clear: all includes, or only the dependent includes?

This would be the "all" variant:

@PACKAGE_INIT@

set(SENTRY_BACKEND @SENTRY_BACKEND@)
set(SENTRY_TRANSPORT @SENTRY_TRANSPORT@)
set(SENTRY_BUILD_SHARED_LIBS @SENTRY_BUILD_SHARED_LIBS@)
set(SENTRY_LINK_PTHREAD @SENTRY_LINK_PTHREAD@)

if(SENTRY_BACKEND STREQUAL "crashpad" AND NOT MSVC AND NOT SENTRY_BUILD_SHARED_LIBS)
	find_dependency(ZLIB)
endif()

if(SENTRY_BACKEND STREQUAL "breakpad" AND NOT SENTRY_BUILD_SHARED_LIBS)
	set(SENTRY_BREAKPAD_SYSTEM @SENTRY_BREAKPAD_SYSTEM@)
	if(SENTRY_BREAKPAD_SYSTEM)
		find_dependency(PkgConfig)
		pkg_check_modules(BREAKPAD REQUIRED IMPORTED_TARGET breakpad-client)
	endif()
endif()


if(SENTRY_TRANSPORT STREQUAL "curl" AND (NOT @BUILD_SHARED_LIBS@ OR NOT SENTRY_BUILD_SHARED_LIBS))
	find_dependency(CURL)
	set_property(TARGET sentry::sentry APPEND
		PROPERTY INTERFACE_LINK_LIBRARIES ${CURL_LIBRARIES})
endif()

if(SENTRY_LINK_PTHREAD AND NOT SENTRY_BUILD_SHARED_LIBS)
	find_dependency(Threads)
endif()

if(SENTRY_BACKEND STREQUAL "crashpad")
	include("${CMAKE_CURRENT_LIST_DIR}/sentry_crashpad-targets.cmake")
endif()

include("${CMAKE_CURRENT_LIST_DIR}/sentry-targets.cmake")

vs. "only dependent" variant:

@PACKAGE_INIT@

set(SENTRY_BACKEND @SENTRY_BACKEND@)
set(SENTRY_TRANSPORT @SENTRY_TRANSPORT@)
set(SENTRY_BUILD_SHARED_LIBS @SENTRY_BUILD_SHARED_LIBS@)
set(SENTRY_LINK_PTHREAD @SENTRY_LINK_PTHREAD@)

if(SENTRY_BACKEND STREQUAL "crashpad")
	if(NOT MSVC AND NOT SENTRY_BUILD_SHARED_LIBS)
		find_dependency(ZLIB)
	endif()
	include("${CMAKE_CURRENT_LIST_DIR}/sentry_crashpad-targets.cmake")
endif()

if(SENTRY_BACKEND STREQUAL "breakpad" AND NOT SENTRY_BUILD_SHARED_LIBS)
	set(SENTRY_BREAKPAD_SYSTEM @SENTRY_BREAKPAD_SYSTEM@)
	if(SENTRY_BREAKPAD_SYSTEM)
		find_dependency(PkgConfig)
		pkg_check_modules(BREAKPAD REQUIRED IMPORTED_TARGET breakpad-client)
	endif()
endif()


if(SENTRY_TRANSPORT STREQUAL "curl" AND (NOT @BUILD_SHARED_LIBS@ OR NOT SENTRY_BUILD_SHARED_LIBS))
	find_dependency(CURL)
	set_property(TARGET sentry::sentry APPEND
		PROPERTY INTERFACE_LINK_LIBRARIES ${CURL_LIBRARIES})
endif()

if(SENTRY_LINK_PTHREAD AND NOT SENTRY_BUILD_SHARED_LIBS)
	find_dependency(Threads)
endif()

include("${CMAKE_CURRENT_LIST_DIR}/sentry-targets.cmake")

[dg0yt]

General rule: all find_dependency before all include, i.e. prefer the first.

In your second example, when checking for find_dependency(CURL), the effects of include("${CMAKE_CURRENT_LIST_DIR}/sentry_crashpad-targets.cmake") would already have taken place. So when find_dependency(CURL) would fail, CMake might continue search for a matching config package in other locations. But it would be unable to create a different setup of the targets produced by sentry_crashpad-targets.cmake, eventually leading to mixed results, literally.

The problem may sound a little bit hypothetical. But finding the root cause may be hard to track it down in case of subtle runtime errors caused by linking mixed providers. And it is easy to do it in a more robust way.

---

If sentry_crashpad-targets.cmake is only installed when it is needed, you could also omit the condition and do

include("${CMAKE_CURRENT_LIST_DIR}/sentry_crashpad-targets.cmake" OPTIONAL)

or given that SENTRY_BACKEND is defined at build time, just put these target into the other export set and avoid the extra include completely.

[supervacuus]

So when find_dependency(CURL) would fail, CMake might continue search for a matching config package in other locations. But it would be unable to create a different setup of the targets produced by sentry_crashpad-targets.cmake, eventually leading to mixed results, literally.

The problem may sound a little bit hypothetical. But finding the root cause may be hard to track it down in case of subtle runtime errors caused by linking mixed providers. And it is easy to do it in a more robust way.

This is precisely the kind of issue I would like to prevent, so thanks for the explanation.

[supervacuus]

Followup to the above conversation here: #1013