CMake dependency fixes #1011
CMake dependency fixes #1011
Conversation
| @@ -62,6 +62,7 @@ option(SENTRY_BUILD_EXAMPLES "Build sentry-native example(s)" "${SENTRY_MAIN_PRO | |||
|
|
|||
| option(SENTRY_LINK_PTHREAD "Link platform threads library" ON) | |||
| if(SENTRY_LINK_PTHREAD) | |||
| set(THREADS_PREFER_PTHREAD_FLAG ON) | |||
There was a problem hiding this comment.
Generally recommended, https://cmake.org/cmake/help/latest/module/FindThreads.html#variables-affecting-behavior.
| @@ -276,15 +277,15 @@ if(CMAKE_SYSTEM_NAME STREQUAL "OS400") | |||
| endif() | |||
|
|
|||
| if(SENTRY_TRANSPORT_CURL) | |||
| if(NOT CURL_FOUND) # Some other lib might bring libcurl already | |||
| if(NOT TARGET CURL::libcurl) # Some other lib might bring libcurl already | |||
There was a problem hiding this comment.
There is no problem with calling find_package repeatedly. However, you are really interested in CURL::libcurl already being created, e.g. by a subproject.
| find_package(CURL REQUIRED COMPONENTS AsynchDNS) | ||
| endif() | ||
|
|
||
| target_link_libraries(sentry PRIVATE CURL::libcurl) | ||
| endif() | ||
|
|
||
| if(SENTRY_TRANSPORT_COMPRESSION) | ||
| if(NOT ZLIB_FOUND) | ||
| if(NOT TARGET ZLIB::ZLIB) | ||
| find_package(ZLIB REQUIRED) |
There was a problem hiding this comment.
There is no problem with calling find_package repeatedly. However, you are really interested in ZLIB::ZLIB already being created, e.g. by a subproject.
| else() | ||
| target_link_libraries(sentry PRIVATE ${_SENTRY_PLATFORM_LIBS}) | ||
| endif() | ||
| target_link_libraries(sentry PRIVATE ${_SENTRY_PLATFORM_LIBS}) |
There was a problem hiding this comment.
You want to export link libs for static linkage. PRIVATE does that.
PUBLIC is needed if include dirs etc. need to be used, too. This normally doesn't depend on the type of linkage but on the design of the headers.
There was a problem hiding this comment.
Honestly, that use of PUBLIC predates my involvement, and I don't know why it was added in detail. I guess the difference resolves to a LINK_ONLY generator expression around the interface link libraries in the exported sentry target, so switching to PRIVATE in both cases shouldn't break anything.
| 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@) |
There was a problem hiding this comment.
Note that in the following lines, there is now a mix of variables exported explicitly in this way, and variables expanded directly via "@VAR@".
There was a problem hiding this comment.
I already started with a similar clean-up; I guess you just wanted to summarize the changes here rather than actually work on a PR, right?
| endif() | ||
| if(SENTRY_LINK_PTHREAD) | ||
| set(THREADS_PREFER_PTHREAD_FLAG ON) | ||
| find_dependency(Threads REQUIRED) |
There was a problem hiding this comment.
Is there a reason why you left this REQUIRED here?
| if("@SENTRY_TRANSPORT_CURL@") | ||
| find_dependency(CURL COMPONENTS AsynchDNS) | ||
| endif() | ||
| if("@SENTRY_TRANSPORT_COMPRESSION@" AND "@CRASHPAD_ZLIB_SYSTEM@") |
There was a problem hiding this comment.
Again, I guess you just wanted to quickly summarize the changes, rather than do the whole PR, but since this refers to changes in #1005: while SENTRY_TRANSPORT_COMPRESSION is one of the reasons why the "system" zlib will be used, it is independent of crashpad.
Thanks a lot for the summary and suggestions, but I will probably do a separate PR where I apply similar changes more globally. |
|
Closing in favor of the tested and slightly extended variant #1013. Thanks once more! |
Collection of review remarks, untested.