Skip to content
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

Separate building static and shared libs #219

Merged
merged 2 commits into from
Feb 1, 2023

Conversation

xakod
Copy link
Contributor

@xakod xakod commented Oct 1, 2022

Introduce separating building static and shared libs, depends on cmake out-of-the-box option BUILD_SHARED_LIBS https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html. Also creates alias target for transparently linking to static or shared lib.

@CLAassistant
Copy link

CLAassistant commented Oct 1, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@Enmk Enmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @Jihadist, thank you for contribution! Few notes:

  • Please provide motivation for the change in PR description.
  • It is not clear which version is now used in CI/CD testing, could you please add job to test both static and shared build?
  • Any reason (besides final binary file name) to keep both targets:clickhouse-cpp-lib-static and clickhouse-cpp-lib? From my perspective, only one is going to be built every time.

IMO, for static builds only, we'd better add an alias: clickhouse-cpp-lib-static (and also make sure that output binary name stays clickhouse-cpp-lib-static. Everywhere else just use a clickhouse-cpp-lib. This is not to break user's code.

@xakod
Copy link
Contributor Author

xakod commented Oct 3, 2022

Ok, @Enmk

  • Reason: i wanna provide a conan recipe to conan-center-index for clickhouse-cpp. Conan does not support static and shared libs at the same time. Library should be build as static or as shared.
  • Ok, i'll try to figure out how to do that and change pr
  • If project uses library with cmake FetchContent or as subdirectory project may depend on static target name. If we change target name for static libs it will be cmake error. There is only one reason why I do not change target name for static.

@xakod xakod changed the title Separate building static and shared libs [WIP] Separate building static and shared libs Oct 3, 2022
@xakod xakod marked this pull request as draft October 3, 2022 17:14
@Enmk
Copy link
Collaborator

Enmk commented Oct 4, 2022

Regarding the CI/CD testing: IMO you might want to add another matrix parameter, like here.
Something like:

      matrix:
        compiler: [clang-6, gcc-7, gcc-8, gcc-9]
        ssl: [ssl_ON, ssl_OFF]
+       build_type: [static, shared]
        include:
...
+       - build_type: shared
+         CMAKE_BUILD_TYPE_FLAG: -BUILD_SHARED_LIBS=ON

etc.

@Enmk
Copy link
Collaborator

Enmk commented Oct 4, 2022

Also, if you wish to add clickhouse-cpp as a Conan module, please free to add any related CI/CD workflows

@xakod
Copy link
Contributor Author

xakod commented Oct 4, 2022

Regarding the CI/CD testing: IMO you might want to add another matrix parameter,

I did this but looks like windows shared build is broken

@xakod
Copy link
Contributor Author

xakod commented Oct 14, 2022

If someone can help with windows shared build with export all, it will be nice

@1261385937
Copy link
Contributor

1261385937 commented Oct 16, 2022

If someone can help with windows shared build with export all, it will be nice

Addressed in #226

CI/CD workflows, the exe maybe miss the dll, be careful of their path

@Enmk
Copy link
Collaborator

Enmk commented Oct 17, 2022

Another attempt to fix windows build here: #227 227

@xakod xakod force-pushed the separateSharedStaticLibs branch 3 times, most recently from d4f3370 to e2856dc Compare December 22, 2022 14:31
@xakod xakod force-pushed the separateSharedStaticLibs branch from e2856dc to 3359603 Compare December 22, 2022 14:42
@xakod xakod marked this pull request as ready for review December 22, 2022 14:42
@xakod xakod changed the title [WIP] Separate building static and shared libs Separate building static and shared libs Dec 22, 2022
@xakod
Copy link
Contributor Author

xakod commented Dec 22, 2022

@Enmk I dropped support for shared builds for windows and macos due to platforms require exporting symbols. Also @1261385937 will close his draft #226 (comment)

@xakod xakod requested a review from Enmk December 22, 2022 14:48
Do not use target_link_libraries with alias
Copy link
Collaborator

@Enmk Enmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Enmk Enmk merged commit df4ae9f into ClickHouse:master Feb 1, 2023
@xakod xakod deleted the separateSharedStaticLibs branch February 2, 2023 19:49
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.

4 participants