-
Notifications
You must be signed in to change notification settings - Fork 174
CMake packaging improvements #418
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
base: master
Are you sure you want to change the base?
Conversation
@@ -110,15 +118,23 @@ SET_TARGET_PROPERTIES (clickhouse-cpp-lib | |||
VERSION ${CLICKHOUSE_CPP_VERSION} | |||
) | |||
TARGET_LINK_LIBRARIES (clickhouse-cpp-lib | |||
absl::int128 |
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.
Why did you mark dependencies private?
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.
Since all dependencies are static libraries the exported target should not propagate these dependencies further downstream.
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.
Clickhouse-cpp-lib can be built as 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.
Yes, this is how I build it for packaging. The dependencies are still static though.
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.
Yes, this is how I build it for packaging. The dependencies are still static though.
Dependencies may be shared, for example, conan packaging. In this case, looks like package will be broken.
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.
The package should not be broken, but the exported CMake target config will be missing dependencies. Is clickhouse-cpp built with conan? I don't see any configuration for it anywhere.
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.
https://conan.io/center/recipes/clickhouse-cpp?version=2.5.1 and here is the example of usage https://github.com/userver-framework/userver/blob/develop/conanfile.py#L162
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.
Okay, it seems like conan generates its own CMake config file, so shouldn't be affected.
https://github.com/conan-io/conan-center-index/blob/master/recipes/clickhouse-cpp/all/conanfile.py#L106
I know nothing about conan though, so I'd be grateful if you verify this.
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.
it seems like conan generates its own CMake config file, so shouldn't be affected
Okay, I forgot about this. From this point of view, everything is ok. Thank you :)
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.
There is of course some room for improvement in how I generate the config file (it's probably not really how you are supposed to do it) and it will most likely not cover all possible usecases. I figured out that it might still be useful as is, after all it's an improvement from not having one at all.
I started working on Debian packaging for the clickhouse-cpp library (not officialy, I need the packages for internal use) and I made some improvements to CMake setup that makes it play nicely with packaging tools.
I thought some of them can be useful for the project.
I tried to split my changes into logical commits so that it's easy to cherry-pick a subset of the changes.
Apparently also closes #105