-
Notifications
You must be signed in to change notification settings - Fork 181
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
add new package redis++ including hiredis #641
base: master
Are you sure you want to change the base?
Conversation
update libuv to version 1.44.2 and use offical repro for download instead hunter
add example for hires package add docs for hiredis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move the libuv update to a new PR, to keep the single PRs small and easy to review
cmake/projects/Redis++/hunter.cmake
Outdated
|
||
hunter_pick_scheme(DEFAULT url_sha1_cmake) | ||
|
||
hunter_cmake_args(Redis++ CMAKE_ARGS REDIS_PLUS_PLUS_BUILD_ASYNC=libuv REDIS_PLUS_PLUS_BUILD_TEST=OFF CMAKE_PREFIX_PATH=${HUNTER_INSTALL_PREFIX}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use multiple lines instead of a very long one for better git diffs in the future
and why is the CMAKE_PREFIX_PATH=${HUNTER_INSTALL_PREFIX})
needed? This should be done automatically by hunter
hunter_cmake_args(Redis++ CMAKE_ARGS REDIS_PLUS_PLUS_BUILD_ASYNC=libuv REDIS_PLUS_PLUS_BUILD_TEST=OFF CMAKE_PREFIX_PATH=${HUNTER_INSTALL_PREFIX}) | |
hunter_cmake_args(Redis++ CMAKE_ARGS | |
REDIS_PLUS_PLUS_BUILD_ASYNC=libuv | |
REDIS_PLUS_PLUS_BUILD_TEST=OFF | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i
please move the libuv update to a new PR, to keep the single PRs small and easy to review
had to update libuv so that i'am able to compile redis++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and why is the
CMAKE_PREFIX_PATH=${HUNTER_INSTALL_PREFIX})
needed? This should be done automatically by hunter
this is needed so redis is able to find libuv and hiredis packages.
this is the way they describe in there docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link to documentation please
and please try it without. I thought this path is set by Hunter itself and you should not be required to set it as argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tested i localy without CMAKE_PREFIX_PATH=${HUNTER_INSTALL_PREFIX}
but then redis++ is not able to find the libs uvlib and hiredis.
https://github.com/sewenew/redis-plus-plus
If hiredis is installed at non-default location, you should use CMAKE_PREFIX_PATH to specify the installation path of hiredis. By default, redis-plus-plus is installed at /usr/local. However, you can use CMAKE_INSTALL_PREFIX to install redis-plus-plus at non-default location.
Co-authored-by: NeroBurner <[email protected]>
Co-authored-by: NeroBurner <[email protected]>
@NeroBurner so i fixed all feedaback from review, how we want proceed? |
as mentioned before
If I've overlooked the libuv PR please ping me there |
You'll need to create a fork of redis++ (@rbsheth please fork https://github.com/sewenew/redis-plus-plus ) and add documentation at: https://hunter.readthedocs.io/en/latest/creating-new/create/cmake-dependencies.html |
Creating a fork only for calling hunter_add_package increases the complexity to keep packages up to date for no real reason. |
yes, the overhead of forks is a known issue (unfortunately). But the goal of Hunter is to provide the dependency for the project with all the other dependencies. Otherwise dependency hell breaks loose More on this topic with detailed explanation why we need forks: https://hunter.readthedocs.io/en/latest/faq/why-do-we-need-forks.html |
HiRedis | ||
hiredis | ||
|
||
.. index:: HiRedis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add the package to at least one of the already listed index-groups
Redis++ | ||
redis | ||
|
||
.. index:: Redis++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add the package to at least one of the already listed index-groups
@@ -0,0 +1,16 @@ | |||
[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed? if not please remove
@@ -0,0 +1,17 @@ | |||
[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed? if not please remove
add new packages hiredis and redis++
update libuv to version 1.44.2 and use offical repro for download instead hunter
step by step carefully. [Yes]
step by step carefully. [Yes]
I tested all localy with vs-17-2022-win64-cxx17