redis / hiredis

Minimalistic C client for Redis >= 1.2
BSD 3-Clause "New" or "Revised" License
6.2k stars 1.81k forks source link

CMake FetchContent support #1119

Open ivan-ushakov opened 1 year ago

ivan-ushakov commented 1 year ago

Is it possible to make FetchContent support? If I use this declaration:

FetchContent_Declare(
  hiredis
  GIT_REPOSITORY https://github.com/redis/hiredis
  GIT_TAG origin/master
  GIT_SHALLOW TRUE
)

set(DISABLE_TESTS OFF)

FetchContent_MakeAvailable(hiredis)

I get header files inside _deps/hiredis-src, not include/hiredis/ and this leads to build error for Redis++ library.

michael-grunder commented 1 year ago

Hi,

I'm honestly not sure what the right way to get this to work is. It looks like using _deps/<project>-src is just what CMake does by default.

Here's a blog post saying as much.

I suppose you would need to somehow forward the custom include directory to redisplusplus, but I'm not familiar enough with CMake to tell you how 😅

Edit: Looks like you can tell redisplusplus where to look for hiredis with -DCMAKE_PREFIX_PATH=/path/to/hiredis, so perhaps you can set that after fetching and before trying to build redisplusplus.

bjosv commented 1 year ago

@ivan-ushakov I tried this out and got hiredis and redis++ to build using following gist.

I found two issues during this:

The tests in redis++ uses find_library(hiredis) instead of find_package(hiredis) and I believe FetchContent only works well with find_package(). By disabling the building of tests the cmake generation works. To fix the exported include path I had to include <redis++.h> instead of <sw/redis++/redis++.h> in main.cc. I believe both are possible issues in redis++.

Would the setup from the gist work for you?

bjosv commented 1 year ago

Sorry! The gist above might not work after all.. I found out that I had hiredis installed in an additional non-default/weird place and CMake actually found that when building.. back to the drawing board..

bjosv commented 1 year ago

Updated the gist to make it work, but requires CMake 3.24. I had to to jump through hoops, so there might be a nicer solution..

ivan-ushakov commented 1 year ago

Updated the gist to make it work, but requires CMake 3.24. I had to to jump through hoops, so there might be a nicer solution..

With this two hacks:

SOURCE_DIR _deps/hiredis

# Set include directory to enable redis++ to find it
include_directories(${CMAKE_BINARY_DIR}/_deps)

It works, but maybe in future this could be solved without hacks.

cwaldren commented 9 months ago

Thank you for the gist.

A note for people searching: redis-plus-plus's 1.3.8 release breaks this workaround by introducing a feature test using CheckSymbolExists ("Support keepalive with customized interval" changelog entry.)

It seems that the hiredis::hiredis alias isn't available for the macro to use. Haven't found a solution yet.

vrince commented 4 months ago

Worked for me out of the box (the same way I FetchContent other projects):

# hiredis
include(FetchContent)
FetchContent_Declare(hiredis 
  GIT_REPOSITORY https://github.com/redis/hiredis.git 
  GIT_TAG v1.2.0 
  GIT_SHALLOW TRUE 
  GIT_PROGRESS TRUE)

if(NOT hiredis)
    FetchContent_Populate(hiredis)
    add_subdirectory(${hiredis_SOURCE_DIR} ${hiredis_BINARY_DIR} EXCLUDE_FROM_ALL)
endif()

Then later you can use hiredis with:

target_link_libraries(your-target PRIVATE hiredis)
...

Cmake v 3.22.1