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

Current setup with absl is dangerous #99

Closed
Dlougach opened this issue Aug 9, 2021 · 3 comments · Fixed by #281
Closed

Current setup with absl is dangerous #99

Dlougach opened this issue Aug 9, 2021 · 3 comments · Fixed by #281
Assignees

Comments

@Dlougach
Copy link

Dlougach commented Aug 9, 2021

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:

  1. User decides to use link with both clickhouse-cpp and absl from master branch and in their project
  2. int128.cc is compiled twice - once as part of clickhouse build, once as part of absl build.
  3. Build likely fails because the same functions will be exported twice from different translation units

Scenario 2:

  1. User decides to use link with both clickhouse-cpp and absl from lts_* branch and in their project. Let's assume neither of those are installed in the system, but are rather plugged into the project via FetchContent/ExternalProject in CMake.
  2. lts_ branch puts all code into something like 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 in absl::.
  3. columns/numeric.cpp will then use absl from contrib.
  4. Any user code that directly or indirectly includes columns/numeric.h will probably use the other absl.
  5. This will lead to mysterious linker failures because now declaration from columns/numeric.h and definition in columns/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.

@Enmk
Copy link
Collaborator

Enmk commented Feb 2, 2022

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.

@Enmk Enmk self-assigned this Feb 16, 2022
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]>
@DavidKeller
Copy link
Contributor

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 :-)

@Enmk
Copy link
Collaborator

Enmk commented Apr 13, 2022

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
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Jul 28, 2022
DavidKeller pushed a commit to woorton/clickhouse-cpp that referenced this issue Nov 23, 2022
xakod pushed a commit to xakod/clickhouse-cpp that referenced this issue Feb 2, 2023
@Enmk Enmk closed this as completed in #281 Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants