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

conda-build should not set std=c++17 by default #3375

Closed
looooo opened this issue Feb 1, 2019 · 10 comments
Closed

conda-build should not set std=c++17 by default #3375

looooo opened this issue Feb 1, 2019 · 10 comments
Labels
locked [bot] locked due to inactivity

Comments

@looooo
Copy link

looooo commented Feb 1, 2019

encountered here:
conda-forge/boost-cpp-feedstock#43

problem: boost is build with -std=c++17. But the boost-depending library is not yet working with c++17 standard. Therefor building with c++14. But afterwards runtime-errors occur due to missing symbols (symbols would be available if boost is build with same c++-standard or lower.) So theoretically a lower c++-standart gives a more compatible library. Therefor I guess it's best to set the default c++-standart to c++11 or lower.

- -fvisibility-inlines-hidden -std=c++17 -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -pipe

@msarahan
Copy link
Contributor

msarahan commented Feb 1, 2019

conda-build does not set any of this. It is done in the compiler activation scripts: https://github.com/AnacondaRecipes/aggregate/blob/master/ctng-compilers-activation-feedstock/recipe/conda_build_config.cos6.x86_64.yaml

@mingwandroid can explain more, but I don't think we're going to change that. That setting has been in place for nearly 2 years now, so there's definitely ways to make things work.

@looooo
Copy link
Author

looooo commented Feb 1, 2019

For conda-forge this setting was introduced with the compiler update happening in the last year (forcing us to use new-compilers since beginning of this year).

@mingwandroid can explain more, but I don't think we're going to change that. That setting has been in place for nearly 2 years now, so there's definitely ways to make things work.

Yes we have to rebuild boost and deps... This would have been avoided if an older c++-standard was used by default.

Can you explain why c++17 was chosen?

@mingwandroid
Copy link
Contributor

We aren't interested in this, sorry. There are no compat issues with C++ standards so long as you use a modern compiler. We provide that.

C++17 was picked because it's the most modern version our compilers support.

@mingwandroid
Copy link
Contributor

If c++17 is too modern for a given package then build.sh should mask that value away. This has been discussed many times before.

We want the most simple C++ package (e.g. one using the most basic Makefile possible) to be able to detect and use c++17 to take advantage of such things as move semantics in order to generate the fastest code possible.

@mingwandroid
Copy link
Contributor

problem: boost is build with -std=c++17. But the boost-depending library is not yet working with c++17 standard. Therefor building with c++14

The changes from c++14 to c++17 are minor, I suggest you apply a patch to remove dynamic exception specification used in LaughlinResearch/SMESH

@looooo
Copy link
Author

looooo commented Feb 1, 2019

If c++17 is too modern for a given package then build.sh should mask that value away. This has been discussed many times before.

This won't work for (some) packages depending on boost.

I understand that you want to stay with c++17 by default, but for boost and maybe other libraries using c++17 makes no sense as this only increase the incompatibilities.

@mingwandroid
Copy link
Contributor

I understand that you want to stay with c++17 by default, but for boost and maybe other libraries using c++17 makes no sense as this only increase the incompatibilities

This is not the case. To be clear, project A could contain:

#if __cplusplus < 20110210
int some_symbol_a;
#endif

While some project B could contain:

#if __cplusplus >= 20110210
int some_symbol_b;
#endif

.. and there is no setting for -std= that will allow both symbols to be defined. In other words it has nothing to do with the decision about which standard is selected.

Also, pessimissing (i.e. making it slower) a library which is as low-level and important as Boost because some other project hasn't modernised (esp. when it's as simple as it is in the case of LaughlinResearch/SMESH) is not something we should entertain IMHO. Where does this end? If there was a project which uses Boost but also requires C++98, would you propose trying to build Boost in C++98 mode?

This is a case of "throwing the baby out with the bathwater"!

@looooo
Copy link
Author

looooo commented Feb 2, 2019

Boost was build with c++98 for good old conda with toolchain. Also arch Linux uses c++14. Jumping to c++17 is a huge step.

Yes it's maybe easy solveable for smesh. But the next library I fear about is freecad. This library doesn't yet work with c++14. So maybe I am lucky and everything works with c++11. We will see.

This is not the case. To be clear, project A could contain:

Ok so we would have the same problem if boost was build with c++11 and smesh with c++17.
But then please don't give the suggestion to set the standart in the build.sh. it may works, but in some cases it won't work.

In the end, yes it's good if you aim for consistency.

@looooo looooo closed this as completed Feb 2, 2019
@mingwandroid
Copy link
Contributor

But then please don't give the suggestion to set the standart in the build.sh.

This is frequently exactly what needs to be done. We've done this for 1.5 years now.

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>
@github-actions
Copy link

Hi there, thank you for your contribution!

This issue has been automatically locked because it has not had recent activity after being closed.

Please open a new issue if needed.

Thanks!

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Mar 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity
Projects
None yet
Development

No branches or pull requests

3 participants