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

abseil: pin ABI at compile-time #7443

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Sep 27, 2021

abseil/all

As discussed in #7400 (comment) Abseil cannot be compiled with C++11 and used from C++17, not without first "pinning" its ABI via some patches to absl/base/options.h. This PR changes the recipe to make these changes when the code is built.

Fixes #7249.


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@ghost
Copy link

ghost commented Sep 27, 2021

I detected other pull requests that are modifying abseil/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot
Copy link
Collaborator

All green in build 1 (9205f3fb2702db5b9a6dd919e5a4c46e66385a3e):

  • abseil/20200225.3@:
    All packages built successfully! (All logs)

  • abseil/20200205@:
    All packages built successfully! (All logs)

  • abseil/20200225.2@:
    All packages built successfully! (All logs)

  • abseil/20200923@:
    All packages built successfully! (All logs)

  • abseil/20200923.2@:
    All packages built successfully! (All logs)

  • abseil/20200923.1@:
    All packages built successfully! (All logs)

  • abseil/20210324.0@:
    All packages built successfully! (All logs)

  • abseil/20210324.1@:
    All packages built successfully! (All logs)

  • abseil/20210324.2@:
    All packages built successfully! (All logs)

  • abseil/20200923.3@:
    All packages built successfully! (All logs)

@coryan
Copy link
Contributor Author

coryan commented Sep 28, 2021

@uilianries this is the change we discussed in #7249

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@conan-center-bot conan-center-bot merged commit 2139780 into conan-io:master Sep 28, 2021
@coryan coryan deleted the fix-abseil-pin-abi branch September 28, 2021 12:41
@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 24, 2022

This change prevents any new build of abseil with gcc11. Why it it needed since abseil has a mechanism to detect the C++ headers to use at build and consume time? If compiler.cppstd at build and consume time are consistent and the same compiler is used, it will detect the same headers, so no ABI issue as soon as consumers either use target_compile_features() (if compiler.cppstd not set in profile) or what is injected by compiler.cppstd.
Here we are trying to assume which value to inject and it's almost impossible.

Actually the problem of function-framework-cpp comes from itself, it's here: https://github.com/GoogleCloudPlatform/functions-framework-cpp/blob/1f0e1a987472d669cb5b4323f22d2f9c07298331/CMakeLists.txt#L42-L43, it should use target_compile_features() as a good citizen, not forcing C++17 specifically.

SpaceIm added a commit to SpaceIm/conan-center-index that referenced this pull request Jan 24, 2022
it's almost impossible to predict if those headers are available. abseil has a mechanism to detect these headers, trust it. Since we are trying very hard to inject at build time the default C++ standard of the compiler or the one given in profile, it should be fine.
@coryan
Copy link
Contributor Author

coryan commented Jan 24, 2022

It is possible I misunderstand how this works, if so, I would be happy to learn how to handle this better.

This change prevents any new build of abseil with gcc11.

I remember reading that GCC 11 would make C++17 its default... yup, that is the case

Why it it needed since abseil has a mechanism to detect the C++ headers to use at build and consume time?

Because the headers detected at build time may be different from the headers detected at consume time, see:

#7249
abseil/abseil-cpp#696
#7400 (comment)

If compiler.cppstd at build and consume time are consistent and the same compiler is used, it will detect the same headers,

That sounds reasonable, but that is not what happened in #7400. I think the value of compiler.cppstd was empty, and therefore "the compiler default" was used. For something like GCC 10, that means Abseil was compiled with C++14, but one could try to consume it with C++17. Possibly we need a better fix now that at least one compiler uses C++17 as its default, but at the time that was the best we could think of.

so no ABI issue as soon as consumers either use target_compile_features() (if compiler.cppstd not set in profile) or what is injected by compiler.cppstd. Here we are trying to assume which value to inject and it's almost impossible.

The problem arises when Abseil is compiled with C++11 or C++14. Abseil's ABI is fixed at that point, it uses (for example) absl::string_view. Adding target_compile_features($abseil_stuff PUBLIC cxx_std_11) to Abseil will not force all consumers to use C++11, they may use C++14, C++17 , quoting cmake-compile-features(7) (emphasis mine):

In projects that use a large number of commonly available features from a particular language standard (e.g. C++ 11) one may specify a meta-feature (e.g. cxx_std_11) that requires use of a compiler mode that is at minimum aware of that standard, but could be greater.

Any consumer using C++17 will try to use std::string_view for Abseil APIs, but the binary artifacts will not have functions matching those types.

Actually the problem of function-framework-cpp comes from itself, it's here: https://github.com/GoogleCloudPlatform/functions-framework-cpp/blob/1f0e1a987472d669cb5b4323f22d2f9c07298331/CMakeLists.txt#L42-L43,
it should use target_compile_features() as a good citizen, not forcing C++17 specifically.

Most likely that would be better, it is unclear that it would solve the problem. Compiling functions-framework-cpp (and anything that consumes it) will still force the compiler to be in C++17 mode, while the compiler may have been in C++11 or C++14 mode when compiling Abseil.

@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 24, 2022

Yes you're right, it's a mess.
Even target_compile_features() doesn't solve it.

  • abseil uses (or will use) target_compile_features(<lib asbseil> PUBLIC cxx_std_11)
  • a downstream project like function framework may use target_compile_features(<lib> PUBLIC cxx_std_17)

So depending on compiler version, CMake injects any standard greater than C++11 while compiling abseil, then any standard greater than C++17 while consuming it in functions framework cpp.

It cannot be solved in CCI without building packages with specific compiler.cppstd in profile.

SpaceIm added a commit to SpaceIm/conan-center-index that referenced this pull request Jan 24, 2022
revert conan-io#7443

it's almost impossible to predict if those headers are available. abseil has a mechanism to detect these headers, trust it. Since we are trying very hard to inject at build time the default C++ standard of the compiler or the one given in profile, it should be fine.
@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 24, 2022

Actually there would be a solution:

  • enable ABSL_PROPAGATE_CXX_STD option in abseil recipe so that it uses target_compile_features(), and backport it in previous versions of abseil handled in CCI => what I did in f336e83
  • pin ABI just after the build of abseil (or even during the build), not before. So let it use its internal mechanism, then catch the result and edit definitions in absl/base/options with either 0 or 1 to pin the ABI in packaged headers.

Therefore, even if abseil was compiled with C++11 standard, a recipe using C++17 at consume time would not be tricked by going into the wrong branch (the opposite would not work, abseil compiled with C++17 could not work in a project forcing C++11, but in conan it shouldn't happen).

conan-center-bot pushed a commit that referenced this pull request Jan 24, 2022
… versions + modernize

* add shared option

* system libs for FreeBSD and iOS/tvOS/watchOS

* move checks to validate()

* stop maintenance of unofficial version 20200205

* fix abseil_dll component for windows shared build

* stop maintenance of unofficial version 20200205

* finer grained export of patches

* drop 20200205 in config.yml as well

* modernize

* absl-string may link to libm

* drop maintenance of several patch versions

keep last patch of each "major" version

* drop shared with msvc for the moment

* ensure C++ standard consistency

revert #7443

it's almost impossible to predict if those headers are available. abseil has a mechanism to detect these headers, trust it. Since we are trying very hard to inject at build time the default C++ standard of the compiler or the one given in profile, it should be fine.
Sahnvour pushed a commit to Sahnvour/conan-center-index that referenced this pull request Jan 30, 2022
…ew patch versions + modernize

* add shared option

* system libs for FreeBSD and iOS/tvOS/watchOS

* move checks to validate()

* stop maintenance of unofficial version 20200205

* fix abseil_dll component for windows shared build

* stop maintenance of unofficial version 20200205

* finer grained export of patches

* drop 20200205 in config.yml as well

* modernize

* absl-string may link to libm

* drop maintenance of several patch versions

keep last patch of each "major" version

* drop shared with msvc for the moment

* ensure C++ standard consistency

revert conan-io#7443

it's almost impossible to predict if those headers are available. abseil has a mechanism to detect these headers, trust it. Since we are trying very hard to inject at build time the default C++ standard of the compiler or the one given in profile, it should be fine.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[package] abseil/*: consider pinning the ABI at install time
6 participants