librdkafka icon indicating copy to clipboard operation
librdkafka copied to clipboard

Support finding LZ4 from installed package

Open m8mble opened this issue 4 years ago • 12 comments
trafficstars

When built with ENABLE_LZ4_EXT=ON, consuming RdKafka via find_package also requires lz4. Since lz4 doesn't bring an LZ4Config.cmake, this package is found using RdKafka's custom FindLZ4.cmake. But this "find module" isn't searched in RdKafka's install target by default 1.

Attempting to do so results in By not providing "FindLZ4.cmake" in CMAKE_MODULE_PATH this project has asked CMake to find a package configuration file provided by "LZ4", but CMake did not find one. [...] with current releases.

This is why extending the CMAKE_MODULE_PATH to the RdKafka install target is necessary. This is actually common practice e.g. 2.

m8mble avatar May 25 '21 18:05 m8mble

I might need some help with Doozer CI errors:

Status: Temporary failure: GIT: Unable to create GIT repo -- failed to make directory './(null)': Permission denied because Unable to create heap repo.BxCgKo6DQq.0 -- Read-only file system, No more retries

m8mble avatar May 26 '21 06:05 m8mble

Doozer had some issues unrelated to your PR, but the cmake job passed so that's good :+1:

edenhill avatar May 26 '21 06:05 edenhill

Pinging @Oxymoron79 @neptoess @benesch @snar for review

edenhill avatar May 26 '21 06:05 edenhill

Pinging @Oxymoron79 @neptoess @benesch @snar for review

Looks good to me. A nice part about the way this is done is that, if LZ4 ever starts shipping their own Find.cmake, this will automatically use that instead of the rdkafka one.

neptoess avatar May 27 '21 01:05 neptoess

Pinging @Oxymoron79 @neptoess @benesch @snar for review

Looks good to me as well.

I just wonder if the chosen installation directory for the FindLZ4.cmake file (see https://github.com/edenhill/librdkafka/blob/master/CMakeLists.txt#L250) should be adjusted to install it to a directory that CMake searches by default?

Oxymoron79 avatar Jun 02 '21 08:06 Oxymoron79

Pinging @Oxymoron79 @neptoess @benesch @snar for review

Looks good to me as well.

I just wonder if the chosen installation directory for the FindLZ4.cmake file (see https://github.com/edenhill/librdkafka/blob/master/CMakeLists.txt#L250) should be adjusted to install it to a directory that CMake searches by default?

Definitely a little bit of a tricky decision. If this were the CMake install command for the LZ4 library, I would 100% agree that FindLZ4 should end up in a directory CMake searches by default. But this is librdkafka. Not sure it makes sense for this library to distribute CMake Find* scripts for other libraries.

neptoess avatar Jun 02 '21 11:06 neptoess

I just wonder if the chosen installation directory for the FindLZ4.cmake file (see https://github.com/edenhill/librdkafka/blob/master/CMakeLists.txt#L250) should be adjusted to install it to a directory that CMake searches by default?

Probably complicated: According to the cmake find procedure documentation, this requires writing files into some folder named lz4. This likely raises concerns of LZ4, e.g. might cause conflicts if they ever were to distribute their own cmake-config.

m8mble avatar Jun 02 '21 18:06 m8mble

@neptoess, @m8mble Good points.Thanks for your explanations.

Oxymoron79 avatar Jun 03 '21 07:06 Oxymoron79

Is there anything I can do to push this PR over the line?

For my future self: the issue can be worked around by adding -DCMAKE_MODULE_PATH=/usr/lib64/cmake/RdKafka at CMake configure time.

m8mble avatar Jun 18 '21 06:06 m8mble

Since this might affect the vcpkg, can you review this @myd7349 ?

edenhill avatar Aug 12 '21 08:08 edenhill

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

cla-assistant[bot] avatar Aug 21 '23 15:08 cla-assistant[bot]

@emasab, still have the same problem with

FetchContent_Declare(
    RdKafka
    GIT_REPOSITORY https://github.com/confluentinc/librdkafka.git
    GIT_TAG v2.4.0
    FIND_PACKAGE_ARGS
)

when the lib is installed

Ender-events avatar Jun 17 '24 09:06 Ender-events