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

Customize options.h for ABI consistency #43

Open
1 task done
pitrou opened this issue Aug 31, 2022 · 7 comments
Open
1 task done

Customize options.h for ABI consistency #43

pitrou opened this issue Aug 31, 2022 · 7 comments
Labels
bug Something isn't working

Comments

@pitrou
Copy link
Member

pitrou commented Aug 31, 2022

Solution to issue cannot be found in the documentation.

  • I checked 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: apache/arrow#13991 (comment))

Installed packages

(not relevant)

Environment info

(not relevant)
@pitrou pitrou added the bug Something isn't working label Aug 31, 2022
@pitrou
Copy link
Member Author

pitrou commented Aug 31, 2022

@h-vetinari

@h-vetinari
Copy link
Member

h-vetinari commented Aug 31, 2022

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
Copy link
Member Author

pitrou commented Aug 31, 2022

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
Copy link
Member

h-vetinari commented Aug 31, 2022

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
Copy link
Contributor

I think we should also consider adjusting the header file for windows as they did in microsoft/vcpkg#12021
to avoid conda-forge/mongodb-feedstock#60 (comment)

@h-vetinari
Copy link
Member

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

@hmaarrfk
Copy link
Contributor

For other readers: the addition DLL options are being proposed here: #49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants