CMake: build either static or shared + don't force PIC + add ALIAS targets #951
Conversation
CMake imported targets are more robust
3789e29 to
9c35d3a
Compare
|
Thanks for the PR, the updated CMakeLists.txt is much cleaner. I'm not very familiar with CMake, but is this change likely to cause pain for anyone if we were to release it in v1.0.1? -ADD_LIBRARY(hiredis_static STATIC ${hiredis_sources})
+ADD_LIBRARY(hiredis::hiredis ALIAS hiredis |
|
Since commits in #851 don't land in any release for the moment, I guess that this change is harmless. It could eventually break people who were using master branch. |
|
I forgot: do be consistent with 1.0.0,
I remember when I've packaged hiredis 1.0.0 for conan, and I'm pretty sure that there was only hardcoded shared libs (and I had to patch CMakeLists to allow static). |
|
This file is used by cpack when building a nuget package. https://github.com/redis/hiredis/blob/master/hiredis.targets Will the shared and static packages have different IDs? |
|
Hi @SpaceIm , any chance of getting this rebased on current master? Thanks |
I don't know nuget, I guess they manage this detail themself. |
|
Closing based on Jan 28 comment since this is in. |
This PR tries to improve CMakeLists.txt:
BUILD_SHARED_LIBSto control SHARED or STATIC, it's good practice to use this instead of building both flavors in the same build:OpenSSL::SSLimported target, it's far more robust. This imported target is available since CMake 3.4 in built-inFindOpenSSL.cmakemodule, and fortunatly it's the min CMake version in hiredis.I did not remove it, but this file -commited in #851 - seems to be useless (some generated file by Visual Studio?): https://github.com/redis/hiredis/blob/master/hiredis.targets