Skip to content

Do not link unneeded libraries #318

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Do not link unneeded libraries #318

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmake/FindRdKafka.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ try_compile(RdKafka_FOUND ${CMAKE_CURRENT_BINARY_DIR}
if (RdKafka_FOUND)
add_library(RdKafka::rdkafka ${RDKAFKA_LIBRARY_TYPE} IMPORTED GLOBAL)
if (UNIX AND NOT APPLE)
set(RDKAFKA_DEPENDENCIES pthread rt ssl crypto dl z)
set(RDKAFKA_DEPENDENCIES pthread rt dl)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks if librdkafka is found as a static library
On the other hand, the trunk versions is also broken for static librdkafka, because it requires more dependencides (at least sasl2, zstd, lz4, curl depending on the librdkafka build flags)

Copy link
Author

@georgthegreat georgthegreat Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are librdkafka dependencies, not cppkafka ones.
cppkafka possess no (??) knowledge regarding librdkafka configuration and hence is unable to guess the necessary libraries to be linked.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but it is not possible to link statically if library does not provide its cmake config => you should find all dependencies manually and link target executable to them

on Ubuntu, librdkafka debian package provides only headers and .a/.so files without RdKafkaConfig.cmake (like many other packages), though if find_library returns librdkafka.a, it must also link with all dependencies explicitly

by the way, packages often have config file to pkg-config, which can tell which libraries is requires
proper Find cmake module should link with them through pkg-config --libs --static

but the easiest reliable solution, i believe, is to substitute find_library(NAMES rdkafka) with find_library(NAMES librdkafka.so librdkafka.dylib) and place find_package(RdKafka CONFIG) if (RdKafka_FOUND) return() on the top of the FindRdKafka.cmake (to try to find manually installed package at first)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pkg-config --libs rdkafka is the tool for listing necessary libraries.

Stop saying nonsence, please.
This PR is not about librdkafka.

else()
set(RDKAFKA_DEPENDENCIES pthread ssl crypto dl z)
set(RDKAFKA_DEPENDENCIES pthread dl)
endif()
set_target_properties(RdKafka::rdkafka PROPERTIES
IMPORTED_NAME RdKafka
Expand Down