Add options to use system abseil, lz4 and cityhash

[DavidKeller]

This change closes #86, and closes #99.

Furthermore, it eases Conan packaging, as Conan already provides abseil, lz4 and cityhash.

Signed-off-by: David Keller [email protected]

[DavidKeller]

Hi,

CI is broken on master due to SSL error, hence I can't verify that my changes aren't causing regression.

See other MRs

• https://github.com/ClickHouse/clickhouse-cpp/actions/runs/2039802185
• https://github.com/ClickHouse/clickhouse-cpp/actions/runs/2042141964
• https://github.com/ClickHouse/clickhouse-cpp/actions/runs/2104298917

Would you be kind enough to have a look at the SSL issue ?

[CLAassistant]

All committers have signed the CLA.

[filimonov]

Would you be kind enough to have a look at the SSL issue ?

Should be fixed in https://github.com/ClickHouse/clickhouse-cpp/pull/170

BTW - maybe that should be also added to a build matrix? I.e. build with system libs, similar to

https://github.com/ClickHouse/clickhouse-cpp/blob/a85a9827792bb91642e0e4511e8083677f0c1b1e/.github/workflows/linux.yml#L40-L42

[Enmk]

BTW - maybe that should be also added to a build matrix? I.e. build with system libs, similar to

We DEFINITELY need to add at least 1 variant to the build matrix for each platform: build with system libs (i.e. -DUSE_SYSTEM_CITYHASH=ON -DUSE_SYSTEM_ABSEIL=ON -DUSE_SYSTEM_LZ4=ON)

[Enmk]

@DavidKeller Tnak you for a PR, few more steps to get it merged:

• sign a CLA
• update the build matrix
• Please also explain why would you need to move third-parties one level deeper? E.g. why contrib/absl/absl istead of just contrib/absl, etc?

[DavidKeller]

• Please also explain why would you need to move third-parties one level deeper? E.g. why contrib/absl/absl istead of just contrib/absl, etc?

In the previous layout, It user wants to use the contrib absl, the build system would have to include

    INCLUDE_DIRECTORIES (contrib)

But that would include other dependencies as well, even if the user prefers to rely on the system lz4 or cityhash.

The extra level allow to add only absl to the include path, i.e.:

    INCLUDE_DIRECTORIES (contrib/absl)

[DavidKeller]

We DEFINITELY need to add at least 1 variant to the build matrix for each platform: build with system libs

I've done this for linux & osx.

Regarding windows (mingw & msvc), it may be more tricky as:

• lz4, cityhash & abseil, don't seem available on chocolatey
• cityhash don't seem available on msys2

[DavidKeller]

Gentlemen,

I've fought quite a bit in order to test dependencies across all platforms. Installing each dependency using platform's package manager when available was getting more and more complex.

So I've switched to conan, as it eases dependencies management.

Pro

• Having the clickhouse-client library within Conan Center would certainly ease adoption.
• A build using LLVM's using libc++ (instead of libstdc++) has been added to the Linux matrix.

Con

• But this Conan work is not complete as demonstrated by the failing tests. When using Conan's LZ4 instead of contrib LZ4, checksum corruptions are detected. This is an important issue IMHO.

• Another issue is that system dependencies can't be used on windows due to a libtool/msvc bug (https://github.com/conan-io/conan-center-index/issues/9002)

I'm running out of time to solve these, but I'm pushing the current state to have your feeling on this work, I may resume later.

[Enmk]

I've fought quite a bit in order to test dependencies across all platforms. Installing each dependency using platform's package manager when available was getting more and more complex.

So I've switched to conan, as it eases dependencies management.

Ok, so could you please reduce the scope of this PR to just system-provided third-parties. I do believe that if it is hard to obtain a library on certain OS, then we can safely omit corresponding CI/CD job type. So you don't need to test system third-parties against all possible combinations, only for those that make sense.

Also, if possible, you may want to hard-code third-parties version installed to avoid accidental breakage in the future.

As for Conan: this is an interesting idea, but could we please move it to another PR? (@DavidKeller ^^)

[Enmk]

@DavidKeller ping?

[DavidKeller]

Hi, will try to split the MR and provide separate MR with conan by the end of the week.

[DavidKeller]

Here is the current state:

• On OSX & Win, nothing has changed, the libary is still using the builtin dependencies.
• On ubuntu-latest (aka ubuntu-20.04), nothing has changed as well.
• A new ubuntun-22.04 build has been created, which uses the system abseil & lz4 and gcc.

Some remarks:

1. It doesn't make sense to me to use system libraries with a different compiler (and libstdc++/libc++) than the one provided by the system, used to build these system libraries.
2. Can't find a cityhash package (unmaintained since Ubuntu 13.0), but as cityhash is not a public dependency, I decided to rely on the builtin version, so the current build doesn't test the WITH_SYSTEM_CITYHASH switch :-( The alternative would be to build the cityhash package and install it locally prior the clickhouse build step.

What's your opinion on these remarks ?

[DavidKeller]

Rebased.

[xakod]

@Enmk sorry for ping. Are there any chances for this pr?

[Enmk]

@Enmk sorry for ping. Are there any chances for this pr?

Sure, just need to rebase and verify that it does what it advertizes.

[DavidKeller]

@Enmk sorry for ping. Are there any chances for this pr?

Sure, just need to rebase and verify that it does what it advertizes.

We're no longer using clickhouse, so I'm afraid I won't spend more time rebasing it again.

@Jihadist, feel free to complete this MR.

I believe the power move here would be - once this get merged - to rely on conan to build everything and retrieve dependencies. See https://github.com/ClickHouse/clickhouse-cpp/commit/d34cd97263ae65e3b473ddb5827afee81fddd5b9

[xakod]

@DavidKeller thank you for this pr and for conanfile especially. I'll try to continue your ideas. Looks like we do not need to rebase it again because no one changed cmake files since last rebasing.

[Enmk]

Hi @DavidKeller ! Thank you for your contribution, I think this is a good start for proper Conan support for clickhouse-cpp. Sorry if the review process wasn't smooth for you

[xakod]

@Enmk this can be closed