morganstanley / modern-cpp-kafka

A C++ API for Kafka clients (i.e. KafkaProducer, KafkaConsumer, AdminClient)
Apache License 2.0
358 stars 89 forks source link

Feature Request: Support librdkafka from add_subdirectory #169

Open stertingen opened 2 years ago

stertingen commented 2 years ago

Since modern-cpp-kafka only works with recent versions of librdkafka, I see myself forced to not use the system-provided version of librdkafka.

I considered vcpkg, but it did not seem feasible to set up vcpkg just for a single package, since modern-cpp-kafka is not available on vcpkg.

So in my current setup, I have both librdkafka and modern-cpp-kafka next to each other in different subdirectories.

I would expect the following CMake snippet to work:

set(RDKAFKA_BUILD_STATIC ON CACHE BOOL "")
add_subdirectory(libs/librdkafka EXCLUDE_FROM_ALL)
add_subdirectory(libs/modern-cpp-kafka)

Unfortunately, there are 2 issues:

  1. librdkafka makes headers available without librdkafka prefix, so #include <rdkafka.h> is needed instead of #include <librdkafka/rdkafka.h>. Maybe this issue should be fixed in librdkafka, but CMake support there is kind of unofficial.
  2. modern-cpp-kafka requires environment variables to be set to find headers and libraries. I would propose to implement a FindRdKafka CMake module and check for the existence of the rdkafka target.

Are there best practices on how to set up librdkafka with modern-cpp-kafka with compatible versions?

stertingen commented 2 years ago

Alternatively, provide a bundled version of librdkafka.

kenneth-jia commented 2 years ago

Not sure whether add_subdirectory (for these 2 libraries) would work since one is depending on another.

While, if you're interested, I'd suggest to have a try with conan -- in short, you just need to call conan install .. --build=missing before the cmake command, and all dependencies would be built for you. E.g. https://github.com/morganstanley/modern-cpp-kafka/blob/main/.github/workflows/kafka_api_demo_conan_build.yml#L37

I'm not quite expert for conan, but once I tried to add a recipe for modern-cpp-kafka, https://github.com/conan-io/conan-center-index/blob/master/recipes/modern-cpp-kafka , and the compatible version could be assigned there, e.g. https://github.com/conan-io/conan-center-index/blob/master/recipes/modern-cpp-kafka/all/conanfile.py#L18

A demo project for conan build, https://github.com/morganstanley/modern-cpp-kafka/blob/main/demo_projects_for_build/conan_build/

stertingen commented 2 years ago

Not sure whether add_subdirectory (for these 2 libraries) would work since one is depending on another.

This usually works. It is possible to depend on a target from subdirectory A while being in subdirectory B if the subdirectories are added in a valid order.

While, if you're interested, I'd suggest to have a try with conan -- in short, you just need to call conan install .. --build=missing before the cmake command, and all dependencies would be built for you. E.g. https://github.com/morganstanley/modern-cpp-kafka/blob/main/.github/workflows/kafka_api_demo_conan_build.yml#L37

I'm not quite expert for conan, but once I tried to add a recipe for modern-cpp-kafka, https://github.com/conan-io/conan-center-index/blob/master/recipes/modern-cpp-kafka , and the compatible version could be assigned there, e.g. https://github.com/conan-io/conan-center-index/blob/master/recipes/modern-cpp-kafka/all/conanfile.py#L18

A demo project for conan build, https://github.com/morganstanley/modern-cpp-kafka/blob/main/demo_projects_for_build/conan_build/

I could use vcpkg in a similar way as conan, but currently I try to avoid depending on such dependency managers.

kenneth-jia commented 2 years ago

Hi, @stertingen I like your idea but I'm not sure whether it could be achieved with a normal (with no "ugly hack") approach. E.g. the project might contain 2 sub-projects, A and B (which depends on A)

add_subdirectory(A)
add_subdirectory(B)

You might want the sequence of build A->install A->build B->install B, while within the same parent project, it could only be build A->build B ->install A->install B. While building the library for B, it would search the path of A library, and it makes more sense if it's the installation path.

stertingen commented 2 years ago

Hi, @stertingen I like your idea but I'm not sure whether it could be achieved with a normal (with no "ugly hack") approach. E.g. the project might contain 2 sub-projects, A and B (which depends on A)

add_subdirectory(A)
add_subdirectory(B)

You might want the sequence of build A->install A->build B->install B, while within the same parent project,

That's the only way the build currently works. And that will continue to work if done right (see below). Most CMake projects allow multiple kinds of 'being included' by other projects. Fully building and installing a sub-project before configuring another is not always useful, especially when cross-compiling.

it could only be build A->build B ->install A->install B. While building the library for B, it would search the path of A library, and it makes more sense if it's the installation path.

I would expect the following behavior from this project's CMakeLists.txt:

  1. Check if library 'A' was already declared as CMake target.
  2. If not, search library 'A', possibly using find_package().
  3. Link against 'A', either using the previously declared CMake target or the library which was found in 2.

If needed, an option can be added to 'B's CMakeLists.txt to force finding 'A' on it's install path.

The point I'm trying to make is that I would like to get rid of the need to install 'A' before being able to use it.

kenneth-jia commented 2 years ago

About Check if library 'A' was already declared as CMake target, any option suggested? I could only think of one workaround, -- if you could predict the librdkafka build path, env var LIBRDKAFKA_INCLUDE_DIR/LIBRDKAFKA_LIBRARY_DIR could be used, https://github.com/morganstanley/modern-cpp-kafka/blob/main/CMakeLists.txt#L42 https://github.com/morganstanley/modern-cpp-kafka/blob/main/CMakeLists.txt#L47 But please make sure the librdkafka library would be static linked.

stertingen commented 2 years ago

About Check if library 'A' was already declared as CMake target, any option suggested?

if(NOT TARGET rdkafka)
  # determine and LIBRDKAFKA_INCLUDE_DIR and LIBRDKAFKA_LIBRARY_DIR
endif()
if(TARGET rdkafka)
  # Link against target rdkafka
  target_link_libraries(${PROJECT_NAME} rdkafka)
else()
  # Link manually against installed rdkafka
  target_include_directories(${PROJECT_NAME} SYSTEM INTERFACE ${LIBRDKAFKA_INCLUDE_DIR})
  target_link_directories(${PROJECT_NAME} INTERFACE ${LIBRDKAFKA_LIBRARY_DIR})
  target_link_libraries(${PROJECT_NAME} INTERFACE rdkafka)
endif()

However, rdkafka does export the target as RdKafka::rdkafka when installed and as rdkafka when used from add_subdirectory. I'm considering opening an issue for this.

If you want to cover all cases, both target rdkafka and RdKafka::rdkafka should be checked.