[cppkafka] bumped to 0.4.0 #31182
[cppkafka] bumped to 0.4.0 #31182
Conversation
|
@microsoft-github-policy-service agree |
krjakbrjak
force-pushed
the
cppkafka-bumped-version
branch
2 times, most recently
from
3feef2a
to
8be6d08
Compare
| if (CPPKAFKA_RDKAFKA_STATIC_LIB) | ||
| add_definitions("-DLIBRDKAFKA_STATICLIB") | ||
| +else () | ||
| + set(RDKAFKA_REQUIRES "rdkafka >= 0.9.4") | ||
| endif() | ||
|
|
||
| if (NOT CPPKAFKA_CONFIG_DIR) | ||
| diff --git a/cmake/cppkafka.pc.in b/cmake/cppkafka.pc.in | ||
| index b5d432c..5dc8af2 100644 | ||
| --- a/cmake/cppkafka.pc.in | ||
| +++ b/cmake/cppkafka.pc.in | ||
| @@ -9,6 +9,6 @@ Url: https://github.com/mfontanini/cppkafka | ||
| Description: C++ wrapper library on top of RdKafka | ||
| Version: @CPPKAFKA_VERSION@ | ||
| Requires: | ||
| -Requires.private: rdkafka >= 0.9.4 | ||
| +Requires.private: @RDKAFKA_REQUIRES@ |
There was a problem hiding this comment.
What's the purpose of this patch? The logic seems inverted.
There was a problem hiding this comment.
@dg0yt Well, the problem here is that the original .pc file always requires rdkafka, and that fails when rdkafka was statically linked (because there would be no such file. When rdkafka is built as static library the config file is rdkafka-static). I agree this particular patch is not suitable for all use cases. A better way would be to have a patch that will append the suffix (-static) to rdkafka in Requires.private.
There was a problem hiding this comment.
A better way would be to have a patch that will append the suffix (-static) to rdkafka in
Requires.private.
This, and even more changes, to rdkafka.
Both Libs.private and Requires.private are exclusively for static linkage, for additonal link libraries.
IIUC rdkafka got it wrong in two ways: By installing different names for different linkage (inconvenient for downstream usage, as in this port), and by publishing additional link libraries only for dynamic linkage (when this information is already carried by DLLs/shared objects).
I assume you don't want to touch librdkafka now, so just replace rdkafka with rdkafka-static. This should have no disadvantage for dynamic linkage. But for a fully working set of pkgconfig files, more work is needed.
There was a problem hiding this comment.
@dg0yt the branch has been updated.
| Libs: -L${libdir} -L${sharedlibdir} -lcppkafka | ||
| Cflags: -I${includedir} -I${includedir}/cppkafka -I@Boost_INCLUDE_DIRS@ |
There was a problem hiding this comment.
I would be concerned about the two lib search dirs and about Boost_INCLUDE_DIRS not being a single dir here.
There was a problem hiding this comment.
That is the part of the original .pc file.
|
|
||
| add_library(${TARGET_NAME} ${CPPKAFKA_LIBRARY_TYPE} ${SOURCES}) | ||
| +IF(MSVC) | ||
| + target_compile_definitions(${TARGET_NAME} PUBLIC NOMINMAX) |
There was a problem hiding this comment.
PUBLIC NOMINMAX has impacts in consumers of the targets, no matter if actually including a TARGET_NAME header. Is this intented?
There was a problem hiding this comment.
Right, should be changed to PRIVATE.
In this version some of the issues were solved: 1. No need for dynamic fix patch as it was solved inside the project. 2. CMake configs are installed
krjakbrjak
force-pushed
the
cppkafka-bumped-version
branch
from
48f1cc4
to
f1b20fa
Compare
In this version some of the issues were solved: