conda-forge / abseil-cpp-feedstock

A conda-smithy repository for abseil-cpp.
BSD 3-Clause "New" or "Revised" License
2 stars 12 forks source link

Customize options.h for ABI consistency #43

Open pitrou opened 2 years ago

pitrou commented 2 years ago

Solution to issue cannot be found in the documentation.

Issue

abseil-cpp is used downstream by different packages, some of which build in C++17 mode, but others in C++14 or C++11 mode.

By default, the Abseil library has a different ABI depending on the C++ language version. However, it also offers a mechanism to solve this issue: https://github.com/abseil/abseil-cpp/blob/37c5c2e5cc51db1bfb4a5d2f2c5494a394d80e56/absl/base/options.h#L15-L51

Quoting from this file:

// Diamond dependency problems can be avoided if all packages utilize the same
// exact version of Abseil. Building from source code with the same compilation
// parameters is the easiest way to avoid such dependency problems. However, for
// package managers who cannot control such compilation parameters, we are
// providing the file to allow you to inject ABI (Application Binary Interface)
// stability across builds. Settings options in this file will neither change
// API nor ABI, providing a stable copy of Abseil between packages.

Of course, once all transitively dependent packages are compiled in C++17 mode it becomes moot.

(see comment in Arrow PR for moving to C++17: https://github.com/apache/arrow/pull/13991#issuecomment-1233151550)

Installed packages

(not relevant)

Environment info

(not relevant)
pitrou commented 2 years ago

@h-vetinari

h-vetinari commented 2 years ago

You have good timing. 😅

I was just made aware and thought about this today during this discussion.

First off, we're aware of the problem w.r.t to C++ versions, but the (current attempt at a) solution is only available from abseil 20220623, and you're still using a way older abseil in arrow CI.

Regarding uniformly using the C++11 ABI, I said in the other PR:

But unless we only build for the C++11 ABI (which I don't think is great for indirection/performance for those who could also use the C++17 stdlib), we'd still get a virality problem where shared builds of libabseil and libabseil-compat (say) could not co-exist in the same environment, and if e.g. google-cloud-cpp were to choose -compat on windows, it wouldn't be co-installable with any package that decides to compile against C++17 libabseil.

and

[to upstream maintainer] What are your thoughts on the impact of using the C++11 ABI unversally on performance / binary footprint? The issue to me is that most packages can use C++17 shared builds just fine, so we'd be pessimizing everyone just for some corner cases.

I'm happy to hear opinions on that (CC @conda-forge/abseil-cpp @conda-forge/core), but replacing direct use of the C++ stdlib (for those that can use it) with lots of indirection through a DLL boundary is something I'm not suuuuuper keen on doing. I'll admit that the escape hatch of using the static libs has been more painful than anticipated, so I'm not going to resist a working solution, but currently I'm still trying to get this to work.

In the case of arrow, why not just upgrade abseil?

pitrou commented 2 years ago

In the case of arrow, why not just upgrade abseil?

To copy my answer from the other thread, we don't have any direct dependency to abseil that I know of, it is getting pulled because of grpc and/or google-cloud-cpp. So I don't know how to upgrade it in our case.

h-vetinari commented 2 years ago

it is getting pulled because of grpc and/or google-cloud-cpp.

~what versions of grpc, google-cloud-cpp (& protobuf) are you using? I'm presuming some parts are pinned / constrained, or is everything free?~

Perhaps better to discuss this in the arrow PR

hmaarrfk commented 2 years ago

I think we should also consider adjusting the header file for windows as they did in https://github.com/microsoft/vcpkg/issues/12021 to avoid https://github.com/conda-forge/mongodb-feedstock/pull/60#issuecomment-1242901106

h-vetinari commented 2 years ago

Basically set ABSL_CONSUME_DLL by default for our DLLs. I think that's a good idea

hmaarrfk commented 2 years ago

For other readers: the addition DLL options are being proposed here: https://github.com/conda-forge/abseil-cpp-feedstock/pull/49