mfontanini / cppkafka

Modern C++ Apache Kafka client library (wrapper for librdkafka)
BSD 2-Clause "Simplified" License
600 stars 206 forks source link

Fix RdKafka_LIBRARY_DIR-NOTFOUND #219

Closed accelerated closed 5 years ago

accelerated commented 5 years ago

Add additional default search directories for find_xxx cmake functions. By default CMAKE_PREFIX_PATH and CMAKE_INCLUDE_PATH are empty.

NOTE: The bug reported in 218 has 2 easy workarounds:

Fixes #218

mfontanini commented 5 years ago

This gives me the impression that there's something wrong with the FindRdKafka script and this is attempting to fix it in a dirty way. Testing this locally, it seems that it complains about the link directory. Is there a reason why the link directory is being exported along with the cppkafka target?

accelerated commented 5 years ago

I don't think exporting this is wrong as it helps not have to add the rdkafka binary directory to the target_link_directories in the dependent lib/application. From the testing that I conducted I notice that find_path and find_library don't behave exactly as per documentation. While find_library tries paths like /usr/lib/<arch> and /usr/local/lib/<arch>, find_path does not, although CMAKE_LIBRARY_ARCHITECTURE is properly set. Find_path seems to rely on <prefix> as per docs which defaults to CMAKE_PREFIX_PATH and which is not set (at least on my ubuntu box). I think that removing the export may not trigger the issue but you will also not have that property defined at all in the target and thus more tests would have to be conducted in apps linking with RdKafka::rdkafka target. PS: A clear indication that both functions don't work identically can be seen from @kerenor23 workaround which was to infer the directory from the .so/.a location found by find_library which doesn't even use a HINT since the HINT provided is NOTFOUND.

mfontanini commented 5 years ago

helps not have to add the rdkafka binary directory to the target_link_directories in the dependent lib/application

This is not right. I created a new project, did find_package(CppKafka) and did target_link_libraries(my-binary cppkafka) and it worked fine, meaning it both linked successfully and it ran successfully using both cppkafka and librdkafka.

accelerated commented 5 years ago

I see...was RdKafka installed as a module-only (from DPKG or autotools) or did you also have the RdKafka cmake config present (i.e. build and installed via cmake)? This could affect the way the lib was discovered. In any case if you prefer to have that line removed from the target, it's ok too, it's just that I haven't tested extensively without it. I personally prefer to define the prefixes in case you may need to use find_path in the future. LMK how you want to proceed.

mfontanini commented 5 years ago

I installed it via apt-get. Let me try building+installing it manually and see if it works as well. I don't really like this solution as it feels like it's trying to patch something rather than fixing the base issue. If it's the only way to get the right behavior, I'm all for it but if we can just change the rdkafka find script, that's probably better.

accelerated commented 5 years ago

BTW I got this working as well by adding the same directories as HINTS instead of adding them to the prefix in the main CMakeLists.txt. So if you want to just change the FindRdKafka script it's also possible but other find_paths may not work properly as well.

mfontanini commented 5 years ago

I ran this by removing those lines in the FindRdKafka script and compiling+installing librdkafka manually and it also worked. Unless you can find a case where this fix doesn't work, I'd like to go ahead with that rather than the one proposed here.

accelerated commented 5 years ago

I assume that besides removing the line in the exported target we have to remove find_path and change find_library to:

find_library(RdKafka_LIBRARY_PATH
    NAMES ${RdKafka_LIBNAME} rdkafka
    HINTS ${RdKafka_ROOT}/lib ${RdKafka_ROOT}/lib64
)

so that if you ever install RDKafka in another location it will still work.

mfontanini commented 5 years ago

Looks good to me. @kerenor23 can you double check to make sure this works on your environment?

kerenor23 commented 5 years ago

sorry, we got a bit of a timezone difference here, will test it tomorrow. I'm pretty sure in my own specific case it would work (I installed librdkafka via apt-get, not from source, so module only and obviously no cmake files). I installed it in the default lib directory /usr/lib/x86_64-linux-gnu/. Since it's the default path my build would find & link to librdkafka.so anyways, with or without this DIR defined. A more meaningful test would be to remove librdkafka & install it in a non-default location, and then try this. Will also try to get to that tomorrow. Thanks anyways.

kerenor23 commented 5 years ago

@mfontanini @accelerated

Test in current environment

Test with librdkafka in different location

Many thanks! A Very interesting repo. When can we expect the next stable release? which features are expected? :)

mfontanini commented 5 years ago

Perfect, thanks for testing this! Regarding the next release, I'm not sure. I currently don't really have the time to work on this library anymore so there's no changes scheduled on my side. @accelerated has made most of the recent contributions (thanks again!) to this repo. I had started working on the kafka admin API but got side tracked and never finished it. Maybe one of these days!