-
Notifications
You must be signed in to change notification settings - Fork 173
Current setup with absl is dangerous #99
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
Comments
Hi @Dlougach ! Thanks for pointing this out, it is less likely that this issue will get any attention soon. So you are welcome to submit a PR or a proposal on how to fix it. |
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Apr 5, 2022
This change solves ClickHouse#86, ClickHouse#99. Furthermore, it eases Conan packaging, as Conan already provides abseil, lz4 and cityhash. Signed-off-by: David Keller <[email protected]>
Hello @Enmk ! Would you be kind enough to approve my MR or at least allow the CI to run in order to see if there is any issue :-) |
Sure, no problem, please sign the CLA and address the issues mentioned in the comments. |
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Apr 13, 2022
This change solves ClickHouse#86, ClickHouse#99. Furthermore, it eases Conan packaging, as Conan already provides abseil, lz4 and cityhash. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Apr 13, 2022
This change solves ClickHouse#86, ClickHouse#99. Furthermore, it eases Conan packaging, as Conan already provides abseil, lz4 and cityhash. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Apr 13, 2022
This change solves ClickHouse#86, ClickHouse#99. Furthermore, it eases Conan packaging, as Conan already provides abseil, lz4 and cityhash. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Apr 13, 2022
This change solves ClickHouse#86, ClickHouse#99. Furthermore, it eases Conan packaging, as Conan already provides abseil, lz4 and cityhash. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Apr 20, 2022
This change solves ClickHouse#86, ClickHouse#99. Furthermore, it eases Conan packaging, as Conan already provides abseil, lz4 and cityhash. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Jul 28, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
DavidKeller
pushed a commit
to woorton/clickhouse-cpp
that referenced
this issue
Nov 23, 2022
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
xakod
pushed a commit
to xakod/clickhouse-cpp
that referenced
this issue
Feb 2, 2023
This change solves ClickHouse#86, ClickHouse#99. Signed-off-by: David Keller <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The way this library currently uses
absl
can mess up a lot of things. In my opinion it should be reconsidered or at least (as a temporary workaround measure) a switch must be added to CMakeLists.txt to switch off int128 support (and any absl dependency coming with it).Scenario 1:
master
branch and in their projectScenario 2:
lts_*
branch and in their project. Let's assume neither of those are installed in the system, but are rather plugged into the project viaFetchContent
/ExternalProject
in CMake.absl::lts_20210324::
inline namespace. So from linker point of view all absl symbols will be there, even though from compiler point of view they will be visible inabsl::
.columns/numeric.cpp
will then use absl from contrib.columns/numeric.h
will probably use the other absl.columns/numeric.h
and definition incolumns/numeric.cpp
are ABI-incompatible.My understanding is that scenario 2 will only occur if the user attempts to actually use Int128 columns. Scenario 1 may occur if the the project uses int128 somewhere else, even if it doesn't need Int128 columns. There is a way to convert scenario 1 to scenario 2 by enabling inline namespace (and giving it unique name) inside
absl/base/options.h
. The disadvantage is that scenario 2 will likely produce more incomprehensible diagnostic messages from the linker.The text was updated successfully, but these errors were encountered: