Fix system cityhash setup

[ZhongRuoyu]

cityhash does not provide any CMake packages. Add a Findcityhash.cmake script to help find cityhash.

[CLAassistant]

All committers have signed the CLA.

[Enmk]

Thank you for a contribution!

Quick question: how do you test this setup? It looks like libcityhash-dev or any other cityhash packages are missing from any recent Ubuntu repos. I've tried 22.04, 20.04, and 18.04, all to no avail.

[ZhongRuoyu]

Homebrew does provide the cityhash package :) . We are already working around this in Homebrew/homebrew-core#128575, with the following adjustments:

    # `cityhash` does not provide a pkg-config or CMake config file.
    # Help CMake find it.
    inreplace "CMakeLists.txt", "FIND_PACKAGE(cityhash REQUIRED)",
                                "FIND_LIBRARY(CITYHASH NAMES cityhash REQUIRED)"
    inreplace "clickhouse/CMakeLists.txt", "cityhash::cityhash", "cityhash"

With this, the build passed on both macOS and Linux.

I suggest trying the following in a homebrew/ubuntu22.04 Docker container, to verify the fix:

brew install cityhash cmake

git clone https://github.com/ClickHouse/clickhouse-cpp.git
cd clickhouse-cpp

# This should fail
cmake -S . -B build -DWITH_SYSTEM_CITYHASH=ON -DCMAKE_FIND_ROOT_PATH="$(brew --prefix)"

# This should work
git fetch origin refs/pull/301/head:fix
git checkout fix
export CXXFLAGS="-I`brew --prefix cityhash`/include"
cmake -S . -B build -DWITH_SYSTEM_CITYHASH=ON -DCMAKE_FIND_ROOT_PATH="$(brew --prefix)"
cmake --build build -j "$(nproc)"

[Enmk]

Nice! Could you please add a workflow that verifies such setup? That would be a more solid guarantee that it doesn't break in the future :)

[ZhongRuoyu]

Sure, I've pushed 45edfc1 and fefc0ee. I also adjusted the patch to avoid additional -L linker flags, because it seems that /usr/local/lib is not in macOS's default linker search paths (error here).

However, when testing this on my fork, some of the tests failed. Specifically, these on Linux, and these on macOS. The failing tests reported DB::Exception: Checksum doesn't match:, so I suspect that this is due to a mismatch between the cityhash versions (clickhouse-cpp uses v1.0.2, while Homebrew ships the latest v1.1.1).

According to cityhash's changelog, cityhash v1.1 did "Change existing functions to improve their hash quality and/or speed". Does this raise a concern?

[Enmk]

According to cityhash's changelog, cityhash v1.1 did "Change existing functions to improve their hash quality and/or speed". Does this raise a concern?

Breaking a hash compatibility with CH server is a big "no-no", perhaps you can choose another version from homebrew to install?

[ZhongRuoyu]

Understood. In that case, I think we'll have to use the vendored cityhash because Homebrew does not support installing a specific version of a formula (package). I opened Homebrew/homebrew-core#129222 to fix this in Homebrew's clickhouse-cpp.

And as for this PR, let me see what I can do here. Some of the failures seem unrelated -- are they transient SSL issues?

[Enmk]

SSL errors seem to be unrelated, maybe just some network issues, but rest are pretty suspicious, especially:

Windows MSVC

https://github.com/ClickHouse/clickhouse-cpp/actions/runs/4772342534/jobs/8517401004?pr=301
ArrayOfLowCardinality.InsertAndQuery
GenericColumnTest/0.RoundTrip
GenericColumnTest/0.NulableT_RoundTrip
GenericColumnTest/1.RoundTrip
GenericColumnTest/1.NulableT_RoundTrip
closed: The operation completed successfully.

MacOS

https://github.com/ClickHouse/clickhouse-cpp/actions/runs/4772342519/jobs/8517401026?pr=301
ArrayOfLowCardinality.InsertAndQuery
"closed: Operation now in progress"

https://github.com/ClickHouse/clickhouse-cpp/actions/runs/4772342519/jobs/8517401280?pr=301
ArrayOfLowCardinality.InsertAndQuery
"closed: Operation now in progress"

[ZhongRuoyu]

OK, I think I'll need to roll back the workflow changes because I don't know any system that ships this very version of cityhash (v1.0.2). (The fact that the clickhouse library was built should suggest that the changes are fine, though.)

As for the other failures, I am very confused because I haven't touched the Windows / MinGW workflows at all. Anyway, I've tried to keep only the CMake changes in another testing branch; the tests should work fine.

[Enmk]

Perhaps we can add something similar to https://github.com/ClickHouse/clickhouse-cpp/blob/master/cmake/Findlz4.cmake to properly set a cityhash::cityhash imported library instead?

[ZhongRuoyu]

That's actually a good idea. I pushed a Findcityhash.cmake and that should do it. I've verified this with a workflow in my fork: https://github.com/ZhongRuoyu/clickhouse-cpp/actions/runs/4840656304/jobs/8626442071#step:7:41. (Failure is due to a mismatched cityhash version, as discussed earlier -- but it's functional.)

[xakod]

Looks nice, thank you!

[Enmk]

Temporary closing to re-trigger against latest master

[ZhongRuoyu]

Thanks for the help to get this merged, @Enmk @Jihadist!