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 should install w/ the correct base/options.h #696

Closed
devjgm opened this issue May 27, 2020 · 10 comments
Closed

Abseil should install w/ the correct base/options.h #696

devjgm opened this issue May 27, 2020 · 10 comments

Comments

@devjgm
Copy link

devjgm commented May 27, 2020

We're using Abseil with our https://github.com/googleapis/google-cloud-cpp project. In order to support our CMake build, we compile and install Abseil in our docker images, so that our CMake builds can simply find and use the installed Abseil libraries. Here's how we compile and install Abseil in our docker images.

https://github.com/googleapis/google-cloud-cpp/blob/19d4a7eaf73eeca16278f6ad4852a9340e1a8f37/ci/kokoro/docker/Dockerfile.fedora-install#L55-L68

This has been working okay, until today, when we tried to use absl::StrSplit, which uses absl::string_view, and we got an error linking in our C++17 build (build log):

std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >)::{lambda(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)#13}::operator()(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const [clone .isra.0]':
storage_throughput_vs_cpu_benchmark.cc:(.text+0x2a48): undefined reference to `absl::lts_2020_02_25::ByChar::Find(std::basic_string_view<char, std::char_traits<char> >, unsigned long) const'
/usr/bin/ld: storage_throughput_vs_cpu_benchmark.cc:(.text+0x2c15): undefined reference to `absl::lts_2020_02_25::ByChar::Find(std::basic_string_view<char, std::char_traits<char> >, unsigned long) const'
collect2: error: ld returned 1 exit status

It looks like the problem is that ABSL_OPTION_USE_STD_STRING_VIEW defaults to 2 (link), which says to use std::string_view if >= C++17, otherwise use Abseil's custom absl::string_view. The issue is that we compile and install Abseil using some default compiler options (c++11 in this case), and we don't explicitly set ABSL_OPTION_USE_STD_STRING_VIEW. However ,when we link against the installed library in a C++17 build, we start using std::string_view, and then obviously the link fails.

Question: Are we holding it wrong? Have we installed Abseil wrong? Is there something we should be doing differently to solve this problem? We installed Abseil the way we thought we should, but I wouldn't be surprised if I did that wrong.

Some general thoughts about a solution

It seems like ABSL_OPTION_USE_STD_STRING_VIEW=2 is never the right thing for an installed Abseil. The installed binary artifacts were compiled with exactly one of Abseil's custom absl::string_view OR std::string, so having the installed base/options.h header default to choosing one of those doesn't seem right.

Could Abseil install a base/options.h that's configured w/ the options matching the way the binary artifacts were compiled/installed?

NOTE: In this issue I'm referring to absl::string_view, but the same issue will arise with absl::any, absl::optional, etc. So whatever solution we come up with here, should likely apply to all of those.

/cc: @coryan @derekmauro @vslashg

@devjgm
Copy link
Author

devjgm commented May 29, 2020

I just found https://abseil.io/docs/cpp/guides/options, which suggests that users are supposed to patch the absl/base/options.h file to set the options they want for the their project. From the guide

Setting these flags on the command like (via a -D compiler flag, for instance) won’t work: you must edit the options.h file directly

That's a bit difficult to use. It makes the following (typical) workflow do the wrong thing.

  1. Fetch Abseil tarball; extract
  2. cmake ... configure
  3. cmake --target install ...

This is basically the normal workflow for compiling and installing software, but using this workflow, the installed absl/base/options.h would have the wrong values for the installed software.

Feature request (bug?)

I think Abseil's cmake install should install the absl/base/options.h file that corresponds to the installed binary code. That means that the installed header cannot use option 2, which means to dynamically choose something, because the installed binaries will have already made that choice.

Is this possible?

@coryan
Copy link

coryan commented May 29, 2020

A suggestion: generate (or modify, but that might be harder) absl/base/options.h in the configure step.

You can even compile a small C++ program, detect if std::optional<>, std::variant<>, etc. are supported and turn the flags appropriately:

https://cmake.org/cmake/help/v3.5/module/CheckCXXSourceCompiles.html

@derekmauro
Copy link
Member

This is the single most often reported issue on our issue tracker, so in light of that, we should take it as a signal that we might either be getting this wrong.

According to our current position, it is fine to install Abseil, but you must build your program with the exact same set of ABI affecting compile options that were used to build the installed Abseil. Since GCC defaults to C++14 (and you used this default when you installed Abseil), but you then tried to build your program with C++17, the build failed. This is currently not something we support.

It's fine to use an installed, pre-compiled version of Abseil. This will prevent you from rebuilding it every time you rebuild your program. But regardless of what options are specified in the options file, you must still specify the same ABI affecting compile options for your own program. The options file really only prevents you from having to define things on the command line, and prevents you from redefining them.

I think you are trying to test that your build works in multiple C++ standard modes? If so, you are still going to need to recompile Abseil for each build (but it is fine to use the install step). I doubt that Abseil is actually unique in the sense that -std= is an ABI affecting compile option, but maybe it happens to fail loudly more often than others.

As long as you understand this principle, 2 is the correct default option in my opinion (and it seems certainly correct for build systems like Bazel that try to force you to use the same compile options for your entire build). If you change it to 0, it may happen to cause Abseil to happen work in the way you are trying to use it, but for the reason I've tried to explain about -std= possibly affecting ABI, you should not rely on it.

Maybe our documentation is lacking about this issue. Currently we do have https://github.com/abseil/abseil-cpp/blob/master/FAQ.md#how-to-i-set-the-c-dialect-used-to-build-abseil and https://github.com/abseil/abseil-cpp/blob/master/FAQ.md#what-is-abi-and-why-dont-you-recommend-using-a-pre-compiled-version-of-abseil. And it is possible the the options file has added to the confusion.

Let us know what you think.

@devjgm
Copy link
Author

devjgm commented Jun 1, 2020

Thanks for the detailed and thoughtful reply, @derekmauro. As a library maintainer for many years myself, I'm completely on board with Abseil's principles. I certainly wouldn't want or expect Abseil to make any ABI guarantees -- we do not make any ABI guarantees in our project either.

But that said, practically speaking, most things don't cause ABI breaks. Even changing -std= rarely breaks ABI. Yes, I know, according to the letter of the law, it could break things, it just rarely does. CMake builds are fundamentally based around "make installs", and if things broke every time C++ said it could break, I don't think CMake could possibly exist. When I take off my Google-library-maintainer hat and put on my library-maintainer-for-non-Google-customers hat, I'm forced to care about these things.

As a user of Abseil, it seems like Abseil is going out of its way to make things difficult for "make install" users by installing a header that says "Inspect language version and choose option A or B" along with a compiled artifact that says "Only option A". That mismatch is what caused my confusion, and perhaps other's confusion as well, for example, probably #634 too.

Following C++ tradition, I think Abseil may have gotten the default wrong for these options 😄 I agree that 2 is a fine default Google and other bazel users. But for "make install" users, who are probably the majority of Abseil's non-google users, it results in a surprising user experience, IMO. A workflow where the user first has to patch Abseil's code seems... not ideal.

If Abseil decides to stick w/ the current policy/defaults/behavior, it would at least be good to update the documentation at https://abseil.io/docs/cpp/tools/cmake-installs to mention that "make install" must also patch the absl/base/options.h file. That is, the default absl/base/options.h is never the right thing for a make install. The user should choose either 0 or 1 -- but 2 is not right. Bonus points if you can make the build fail if they try to install the default options.h that uses option 2.

@bduclaux
Copy link

Agreed.

We use Abseil too, and we also use several C++ CMake projects which are either C++11 or C++17.
The defaults in absl/base/options.h are not right for us: we have to patch them to use the zero version.
Would be nice to get a default value which just works when you have a mix of C++11/14/17 versions.

@coryan
Copy link

coryan commented Sep 10, 2021

FWIW, I think #820 is a duplicate of this bug.

@coryan
Copy link

coryan commented Sep 10, 2021

Note that this is surprising folks in weird places, e.g. conan-io/conan-center-index#5766

coryan added a commit to coryan/arrow that referenced this issue Dec 3, 2021
This PR adds support for `google-cloud-cpp` to the Conda files.
Probably the most difficult change to grok is the change to compile with
C++17 when using Conda:

- Conda defaults all its builds to C++17,
  [this bug](conda/conda-build#3375) goes into
  some detail as to why.
- Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is
  provided.
- Abseil's ABI changes when used from C++11 vs. C++17, see
  abseil/abseil-cpp#696
- Therefore, one must compile with C++17 to use Abseil in Conda.
- And because `google-cloud-cpp` has a direct dependency on Abseil,
  exposed through the headers, one must use C++17 to use
  `google-cloud-cpp` too.
coryan added a commit to coryan/arrow that referenced this issue Dec 9, 2021
This PR adds support for `google-cloud-cpp` to the Conda files.
Probably the most difficult change to grok is the change to compile with
C++17 when using Conda:

- Conda defaults all its builds to C++17,
  [this bug](conda/conda-build#3375) goes into
  some detail as to why.
- Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is
  provided.
- Abseil's ABI changes when used from C++11 vs. C++17, see
  abseil/abseil-cpp#696
- Therefore, one must compile with C++17 to use Abseil in Conda.
- And because `google-cloud-cpp` has a direct dependency on Abseil,
  exposed through the headers, one must use C++17 to use
  `google-cloud-cpp` too.
coryan added a commit to coryan/arrow that referenced this issue Jan 19, 2022
This PR adds support for `google-cloud-cpp` to the Conda files.
Probably the most difficult change to grok is the change to compile with
C++17 when using Conda:

- Conda defaults all its builds to C++17,
  [this bug](conda/conda-build#3375) goes into
  some detail as to why.
- Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is
  provided.
- Abseil's ABI changes when used from C++11 vs. C++17, see
  abseil/abseil-cpp#696
- Therefore, one must compile with C++17 to use Abseil in Conda.
- And because `google-cloud-cpp` has a direct dependency on Abseil,
  exposed through the headers, one must use C++17 to use
  `google-cloud-cpp` too.
pitrou pushed a commit to apache/arrow that referenced this issue Feb 8, 2022
This PR adds support for `google-cloud-cpp` to the Conda files.
Probably the most difficult change to grok is the change to compile with
C++17 when using Conda:

- Conda defaults all its builds to C++17,
  [this bug](conda/conda-build#3375) goes into
  some detail as to why.
- Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is
  provided.
- Abseil's ABI changes when used from C++11 vs. C++17, see
  abseil/abseil-cpp#696
- Therefore, one must compile with C++17 to use Abseil in Conda.
- And because `google-cloud-cpp` has a direct dependency on Abseil,
  exposed through the headers, one must use C++17 to use
  `google-cloud-cpp` too.

Closes #11916 from coryan/ARROW-14506-add-google-cloud-cpp-to-conda-files

Lead-authored-by: Uwe L. Korn <uwe.korn@quantco.com>
Co-authored-by: Carlos O'Ryan <coryan@google.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@coryan
Copy link

coryan commented Feb 16, 2022

Abseil already adds the cxx_std_11 property to its CMake targets:

if(ABSL_PROPAGATE_CXX_STD)
# Abseil libraries require C++11 as the current minimum standard.
# Top-level application CMake projects should ensure a consistent C++
# standard for all compiled sources by setting CMAKE_CXX_STANDARD.
target_compile_features(${_NAME} PUBLIC cxx_std_11)

I would argue that when compiled with C++ >= 17, Abseil should set this property to cxx_std_17. That will force all consumers of the library (when so compiled) to also enable C++ 17.

Pros If Abseil is compiled with C++ >= 17 it is always consumed with C++ >= 17. It will work most of the time, though it may be surprising for folks that their code is compiled with a newer C++ standard.

Cons If Abseil is compiled with C++11 but consumed with C++ >= 17 things still break.

@derekmauro
Copy link
Member

I think that makes sense. So basically something like this (untested)?

if(ABSL_CXX_STANDARD EQUAL 17)
  target_compile_features(${_NAME} PUBLIC cxx_std_17)
elseif(ABSL_CXX_STANDARD EQUAL 14)
  target_compile_features(${_NAME} PUBLIC cxx_std_14)
elseif(...)

@coryan
Copy link

coryan commented Feb 17, 2022

So basically something like this (untested)?

Basically. Note that many compilers default to C++14, so even if ABSL_CXX_STANDARD is not set the effective version is C++14. Likewise, a smaller (but growing) number of compilers default to C++17, e.g., GCC >= 11:

https://gcc.gnu.org/gcc-11/changes.html

I think you will want to write a compile test to determine the effective C++ version. See:

https://cmake.org/cmake/help/latest/command/try_compile.html

vfraga pushed a commit to rafael-telles/arrow that referenced this issue Mar 21, 2022
This PR adds support for `google-cloud-cpp` to the Conda files.
Probably the most difficult change to grok is the change to compile with
C++17 when using Conda:

- Conda defaults all its builds to C++17,
  [this bug](conda/conda-build#3375) goes into
  some detail as to why.
- Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is
  provided.
- Abseil's ABI changes when used from C++11 vs. C++17, see
  abseil/abseil-cpp#696
- Therefore, one must compile with C++17 to use Abseil in Conda.
- And because `google-cloud-cpp` has a direct dependency on Abseil,
  exposed through the headers, one must use C++17 to use
  `google-cloud-cpp` too.

Closes apache#11916 from coryan/ARROW-14506-add-google-cloud-cpp-to-conda-files

Lead-authored-by: Uwe L. Korn <uwe.korn@quantco.com>
Co-authored-by: Carlos O'Ryan <coryan@google.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
vfraga pushed a commit to rafael-telles/arrow that referenced this issue Mar 24, 2022
This PR adds support for `google-cloud-cpp` to the Conda files.
Probably the most difficult change to grok is the change to compile with
C++17 when using Conda:

- Conda defaults all its builds to C++17,
  [this bug](conda/conda-build#3375) goes into
  some detail as to why.
- Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is
  provided.
- Abseil's ABI changes when used from C++11 vs. C++17, see
  abseil/abseil-cpp#696
- Therefore, one must compile with C++17 to use Abseil in Conda.
- And because `google-cloud-cpp` has a direct dependency on Abseil,
  exposed through the headers, one must use C++17 to use
  `google-cloud-cpp` too.

Closes apache#11916 from coryan/ARROW-14506-add-google-cloud-cpp-to-conda-files

Lead-authored-by: Uwe L. Korn <uwe.korn@quantco.com>
Co-authored-by: Carlos O'Ryan <coryan@google.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
vfraga pushed a commit to rafael-telles/arrow that referenced this issue Mar 25, 2022
This PR adds support for `google-cloud-cpp` to the Conda files.
Probably the most difficult change to grok is the change to compile with
C++17 when using Conda:

- Conda defaults all its builds to C++17,
  [this bug](conda/conda-build#3375) goes into
  some detail as to why.
- Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is
  provided.
- Abseil's ABI changes when used from C++11 vs. C++17, see
  abseil/abseil-cpp#696
- Therefore, one must compile with C++17 to use Abseil in Conda.
- And because `google-cloud-cpp` has a direct dependency on Abseil,
  exposed through the headers, one must use C++17 to use
  `google-cloud-cpp` too.

Closes apache#11916 from coryan/ARROW-14506-add-google-cloud-cpp-to-conda-files

Lead-authored-by: Uwe L. Korn <uwe.korn@quantco.com>
Co-authored-by: Carlos O'Ryan <coryan@google.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
razmser pushed a commit to razmser/abseil-cpp that referenced this issue Sep 12, 2023
Change `absl/base/options.h` at install time to reflect the ABI used
to compile the libraries, i.e., if Abseil was compiled with
C++ >= 17 then the `ABSL_OPTION_USE_STD_*` macros are set to `1`,
because then the C++ 17 types are embedded in the ABI of the installed
artifacts.

Likewise, set the `target_compile_features()` of Abseil libraries
to reflect the C++ version used to compile them. If they the Abseil
libraries are compiled with C++ >= 17, then all downstream
dependencies must also be compiled with C++ >= 17.

Fixes abseil#696

PiperOrigin-RevId: 472538165
Change-Id: Ic9e346ebe277c882a18ad96e1918c9e3511c91c3
razmser pushed a commit to razmser/abseil-cpp that referenced this issue Sep 12, 2023
Change `absl/base/options.h` at install time to reflect the ABI used
to compile the libraries, i.e., if Abseil was compiled with
C++ >= 17 then the `ABSL_OPTION_USE_STD_*` macros are set to `1`,
because then the C++ 17 types are embedded in the ABI of the installed
artifacts.

Likewise, set the `target_compile_features()` of Abseil libraries
to reflect the C++ version used to compile them. If they the Abseil
libraries are compiled with C++ >= 17, then all downstream
dependencies must also be compiled with C++ >= 17.

Fixes abseil#696

PiperOrigin-RevId: 472538165
Change-Id: Ic9e346ebe277c882a18ad96e1918c9e3511c91c3
netkex pushed a commit to netkex/abseil-cpp that referenced this issue Apr 3, 2024
Change `absl/base/options.h` at install time to reflect the ABI used
to compile the libraries, i.e., if Abseil was compiled with
C++ >= 17 then the `ABSL_OPTION_USE_STD_*` macros are set to `1`,
because then the C++ 17 types are embedded in the ABI of the installed
artifacts.

Likewise, set the `target_compile_features()` of Abseil libraries
to reflect the C++ version used to compile them. If they the Abseil
libraries are compiled with C++ >= 17, then all downstream
dependencies must also be compiled with C++ >= 17.

Fixes abseil#696

PiperOrigin-RevId: 472538165
Change-Id: Ic9e346ebe277c882a18ad96e1918c9e3511c91c3
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

No branches or pull requests

5 participants
@derekmauro @coryan @devjgm @bduclaux and others