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

Compile (static) abseil for different C++ versions? #29

Closed
h-vetinari opened this issue Mar 2, 2022 · 20 comments · Fixed by #35
Closed

Compile (static) abseil for different C++ versions? #29

h-vetinari opened this issue Mar 2, 2022 · 20 comments · Fixed by #35

Comments

@h-vetinari
Copy link
Member

h-vetinari commented Mar 2, 2022

Abseil has a bit of a special situation because the API changes with the C++ version (e.g. using std::string_view for C++17 and onwards, but absl::string_view below that). This can lead to pretty cryptic linker errors for projects consuming the C++ libs of abseil, because the wrong symbols will be present depending on the version that the target project is compiled with.

Considering that conda-forge allows individual projects to set different C++ standards, we should IMO consider using separate outputs depending on the cxx version used to compile (this minimum can be enforced with CMake's target_compile_features that's already used upstream but still off by default).

If I'm not off with this, the newest abseil-cpp builds are already effectively requiring C++17 (since fef86bf). I ran into this while testing older C++ standards in conda-forge/sentencepiece-feedstock#26, and running into missing symbols on osx/linux where there weren't any when compiling with C++17.

CC @conda-forge/abseil-cpp

@xhochy
Copy link
Member

xhochy commented Mar 2, 2022

The different outputs cannot co-exist in the same environment. Thus I would prefer to shift all projects to C++17 that need abseil-cpp. From what I have seen, there are projects that require a C++17 enabled Abseil whereas ones that don't require it still all support it.

@h-vetinari
Copy link
Member Author

Fair point. It would still be possible by distinguishing the include paths and library names, but that'd be pretty invasive.

I think it's reasonable to just demand C++17 everywhere, but then that's easily overlooked and the compile-errors can be super-cryptic. I'd like to have a way to signal the CXX requirement through metadata, but considering how many ways exist to set the standard version, that's probably a fool's errand. Would probably be more realistic to migrate to C++17 conda-forge wide...

@h-vetinari
Copy link
Member Author

h-vetinari commented Mar 4, 2022

Just came up in discussion in #25 - apache/arrow is still at C++11, which means that if we want to have shared builds on windows (something I'd very much like to pull off), we'd be leaving them out in the cold, or forcing a huge C++ standard bump.

@h-vetinari
Copy link
Member Author

Alternatively, we could separate the standards used to compile for windows (C++11) and unix (C++17)...

@h-vetinari
Copy link
Member Author

I think at the very least, we should put the C++ version used to compile into the build_string...

@xhochy
Copy link
Member

xhochy commented Mar 8, 2022

Just came up in discussion in #25 - apache/arrow is still at C++11, which means that if we want to have shared builds on windows (something I'd very much like to pull off), we'd be leaving them out in the cold, or forcing a huge C++ standard bump.

This shouldn't be an issue. It would not bump Arrow overall, just the conda-forge based builds. We already do that on Linux and Mac in Arrow, so moving Windows, too, is just a natural way of things.

Is there any other need for Windows to stay on C++11?

@h-vetinari
Copy link
Member Author

It would not bump Arrow overall, just the conda-forge based builds.

From my understanding of the errors that @lidavidm reported in #25, arrow CI compiles against abseil (and grpc) from conda-forge.

@lidavidm
Copy link

lidavidm commented Mar 8, 2022

Ah, I was unclear in #25, it's not all our CI builds that use conda-forge, just a subset. So yes, we could just move up the language standard for some of our builds (we still do need to support C++11 overall, but if the rest of conda-forge is moving to C++17 then that shouldn't be an issue for Arrow's conda-forge packages either I think)

@h-vetinari
Copy link
Member Author

We could also compile static libs with other standards for those that cannot use our cxx_version (e.g. C++17). That way, downstream packages could still be compiled against a different cxx-baseline without colliding symbols etc.

@h-vetinari h-vetinari changed the title Compile abseil for different C++ versions? Compile (static) abseil for different C++ versions? Mar 17, 2022
@h-vetinari
Copy link
Member Author

@conda-forge/core

I believe that the decision to not impose a global C++ standard version (and letting feedstocks choose) is at odds with having abseil builds for only one C++ standard version (currently C++17 on unix, C++11 on windows). Since abseil's ABI changes based on the standard version used to compile, it leads to pretty obscure compilation and linker errors (though admittedly, the mismatch is not as sensitive on unix as it is on windows).

I'm aware of CFEP 18, but I think this falls under the following caveat listed there:

Building static archives (in separated packages) is still useful for some non-common workflows. These don't represent the majority of uses of our packages but can profit a lot from our using our existing infrastructure.

What would people think about building:

  • one dynamic lib (say, C++17)
  • one static lib each for C++11, C++14, C++17

That way, each feedstock could consume abseil in a compatible way at least statically; and if the consuming feedstock is C++17 compatible, they can also use the dynamic lib.

@h-vetinari
Copy link
Member Author

h-vetinari commented Mar 31, 2022

Actually, windows cannot fully be built dynamically see here. I'm thinking it would be good to add

outputs:
  - name: libabseil_dynamic
    build:
      skip: True  # [win]
      string: h{{ PKG_HASH }}_{{ PKG_BUILDNUM }}_cxx{{ cxx_standard }}
      run_exports:
        - libabseil_dynamic ={{ version }}=*_cxx{{ cxx_standard }}

  - name: libabseil_static
    build:
      string: h{{ PKG_HASH }}_{{ PKG_BUILDNUM }}_cxx{{ cxx_standard }}

  # choose sensible default per platform
  - name: libabseil
    requirements:
      host:
        - libabseil_dynamic ={{ version }}=*_cxx17  # [unix]
        - libabseil_static ={{ version }}=*_cxx11   # [win]

  # for compatibility
  - name: abseil-cpp
    requirements:
      host:
        - {{ pin_subpackage('libabseil', exact=True) }}

This is assuming we want to use this opportunity to resolve one more of the old -cpp remnants, cf. conda-forge/conda-forge.github.io#1073.

@hmaarrfk
Copy link
Contributor

I guess the other issue is that certain cuda builds are stuck on old compilers too. Do those all support c++17? (Maybe this is an other reason to drop old cuda versions)

@kkraus14
Copy link

I guess the other issue is that certain cuda builds are stuck on old compilers too. Do those all support c++17? (Maybe this is an other reason to drop old cuda versions)

No they do not. I don't think C++17 support was introduced until 11.0.

@h-vetinari
Copy link
Member Author

No they do not. I don't think C++17 support was introduced until 11.0.

Yup: https://en.cppreference.com/w/cpp/compiler_support#cpp17

@hmaarrfk
Copy link
Contributor

You mean nvcc 11.0. If we have to drop 10.2 then so be it :/

@hmaarrfk
Copy link
Contributor

hmaarrfk commented May 4, 2022

How about just building it with both versions of abseil? Doesn't that work for tensorflow?

conda-forge/grpc-cpp-feedstock#138

@xhochy
Copy link
Member

xhochy commented May 4, 2022

As mentioned in the other thread: This sounds like the simplest way forward for the moment. Tensorflow will probably need some time to fully migrate to C++17 whereas some other Google tools cannot build with C++14 abseil. Thus, we won't be able to settle on one C++ standard for the next weeks/months.

@h-vetinari
Copy link
Member Author

Hey @hmaarrfk @xhochy

I was stuck in a transcontinental move and couldn't really keep up with the discussions/changes here. Was building for two abseil-versions necessary, or more a work-around for the fact that we don't have C++-standard-specific versions? Would something like the above comment still be useful?

@ngam
Copy link

ngam commented Jun 22, 2022

I don't know if this tensorflow/tensorflow@262777e means c++17 is coming to tensorflow or not, but we may be stuck with issues related to absl in conda-forge/tensorflow-feedstock#240.

@ngam
Copy link

ngam commented Jun 22, 2022

fyi jax moved to c++17 jax-ml/jax@jaxlib-v0.3.10...jaxlib-v0.3.14, so a move for tf must be imminent

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 a pull request may close this issue.

6 participants