Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cmake static or shared #1160
Cmake static or shared #1160
Changes from 15 commits
7a31646
0033ad4
359e3a6
c72e8ea
a773609
980e952
9c35d3a
d39cd76
ace2ef5
f1c7aff
95ed681
220bb81
880a15b
22fb29b
420938e
41b3ef1
9f3265f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
After you have removed this you have broke the possibility of having both shared and static libraries installed and exported to hiredis-targets.cmake. Existing projects which specify hiredis::hiredis_static in
TARGET_LINK_LIBRARIES
would be broken and if the target will be changed to hiredis::hiredis they will be linked to shared library.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.
Yeah that is true. I could add the following (behind an option)
then you have your
hiredis::hiredis_static
target back.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.
Ok. Go ahead please.
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.
JFYI, there is a brilliant article for libraries' authors about exporting shared/static configurations in CMake.
https://alexreinking.com/blog/building-a-dual-shared-and-static-library-with-cmake.html
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.
No rush @autoantwort but if you add the final change to allow building both I'll get this merged.
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.
Nice read.
@michael-grunder Done. Thank you for the ping, forgot it.