Skip to content

redis++_static should link to hiredis_static #392

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

Merged
merged 3 commits into from
Sep 16, 2022

Conversation

msardonini
Copy link
Contributor

Hi, thanks for the project I've been finding it very useful!

This pull request is fairly simple, it allows the redis++_static target to link to the hiredis_static target instead of the current hiredis target.

My project uses the static version of redis++ because it's intended to be deployed on embedded devices, and having as many dependencies included in the libredis++.a library makes configuration management much easier. Because libredis++.a currently links to libhiredis.so, I need to make sure that all deployment devices also receive the exact version of libhiredis.so, a problem is made a little more difficult because the hiredis can't be built in the same project, redis++ expects to find hiredis installed at the system level. (This PR for reference)

The proposed PR introduces the cmake variable REDIS_PLUS_PLUS_HIREDIS_LIBS_STATIC, which is constructed from REDIS_PLUS_PLUS_HIREDIS_LIBS, but appends _static where necessary. I have tested on version with and without SSL.

Let me know if you have any questions, or if other edits are necessary.

Thanks

@sewenew
Copy link
Owner

sewenew commented Sep 13, 2022

Thanks for your contribution!

I did some research, and found that hiredis' static target was added recently. Before that, even the latest stable release of hiredis-v1.0.2, does not supporthiredis::hiredis_static. So when running cmake for redis-plus-plus with an older version hiredis, cmake can not find hiredis::hiredis_static target, and fails.

In short, this PR breaks legacy code...

In order to solve the problem, we need to check if hiredis::hiredis_static exists, if it does not exist, link to hiredis::hiredis instead.

B.T.W. I also use redis-plus-plus and hiredis with static libs. Although cmake links redis-plus-plus with dynamic lib of hiredis, I can still link my application to both redis-plus-plus and hiredis' static libs.

c++ -std=c++17 -I/path/to/include -o app app.cpp /path/to/libredis++.a /path/to/libhiredis.a -pthread

Not sure why it cannot work with you. Maybe you can share some ideas :)

Regards

…hem. If the checks fail, default behavior is used
@msardonini
Copy link
Contributor Author

Hi @sewenew,

Thanks for your research and thoughts!

Good point that the hiredis::hiredis_static cmake target is new. I probably should have added that check regardless. Luckily, cmake makes this pretty easy, I've added some checks that make sure that the hiredis::hiredis_static cmake target is defined before trying to link to it. Otherwise it will use the same behavior as before.

I have tested this branch with hiredis version v1.0.2 and v0.14.2, both work as expected with the new cmake change.

Let me know if you have any more questions/concerns,

Thanks,

@sewenew sewenew merged commit 4057bb7 into sewenew:master Sep 16, 2022
@sewenew
Copy link
Owner

sewenew commented Sep 16, 2022

@msardonini Thanks a lot for your contribution!

Regards

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

Successfully merging this pull request may close these issues.

2 participants