Skip to content
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.

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

Feature Request: Support librdkafka from add_subdirectory #169

Open
stertingen opened this issue Sep 28, 2022 · 7 comments
Open

Feature Request: Support librdkafka from add_subdirectory #169

stertingen opened this issue Sep 28, 2022 · 7 comments

Comments

@stertingen
Copy link
Contributor

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
Copy link
Contributor Author

Alternatively, provide a bundled version of librdkafka.

@kenneth-jia
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants