conda-forge / ctng-compiler-activation-feedstock

A conda-smithy repository for ctng-compiler-activation.
BSD 3-Clause "New" or "Revised" License
13 stars 22 forks source link

GCC 9.3 compilers should not include `-fopenmp` or `-std=c++17` by default #42

Closed rgommers closed 3 years ago

rgommers commented 3 years ago

This seems worth a bug report. Adding compile flags by default to integrate with the rest of the stack in the same conda env is fine, but -fopenmp is not one of those flags. So I'd consider this a bug.

@mckib2 noticed this when trying to integrate a new Fortran library into SciPy. From https://github.com/mckib2/scipy/pull/9#issuecomment-860372498:

_So I can see where the issue is, e.g. for _zpropack_

Fortran fix compiler: /opt/conda/envs/scipy-dev/bin/x86_64-conda-linux-gnu-gfortran
-Wall -g -ffixed-form -fno-second-underscore -Wall -g -fno-second-underscore -fPIC
-fopenmp -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong
-fno-plt -O2 -ffunction-sections -pipe -isystem /opt/conda/envs/scipy-dev/include -O3 -funroll-loops

Specifically, it's giving the option -fopenmp during compilation. I'm not sure yet how to remove this or work around it or why it doesn't show up on my machine.

And from my response: https://github.com/mckib2/scipy/pull/9#issuecomment-864163986

Here's the log after explicitly adding undef statements for _OPENMP (appears to resolve GOMP errors). The Fortran compilers from numpy.distutils seem to be adding -fopenmp by default for the PROPACK libraries and extensions. Not sure what's going on here

This is pretty hairy. The flags aren't added by numpy.distutils, but by the conda compiler config. I believe it comes from:

/path/to/conda-install/envs/env-name/lib/gcc/x86_64-conda-linux-gnu/9.3.0/specs

I don't see anything in the conda-build docs or elsewhere about this.

So my questions are:

  1. Can we please get rid of -fopenmp?
  2. Is there a conda-specific way of modifying these flags, or is the only way to work around this in the build system or setup.py files of the package one is trying to build?
rgommers commented 3 years ago

There's more in this vein:

$ echo $CXXFLAGS
-fvisibility-inlines-hidden -std=c++17 -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /home/rgommers/anaconda3/envs/scipy-dev/include

For local development this is not great. In particular -std=c++17, but from a principled point of view CXXFLAGS should only include the absolute minimum to account for the cross-build nature of conda envs. Which probably is -isystem and nothing else.

The problem is a local gcc may not work because binutils is too old, so the encouraged way is to use compilers. But then compilers are configured in a way that's way too nonstandard and leads to really hard to debug build problems. It looks to me like there perhaps should be two sets of configs:

  1. I'm doing local development
  2. I'm building a package to be uploaded to a conda channel.

But irrespective of that, adding -fopenmp and -std=c++17 is not good.

isuruf commented 3 years ago

I agree that explicitly adding -std=c++17 is not good, but note that -std=c++17 (or the gnu variant, can't remember) is the default for gcc 11.

isuruf commented 3 years ago

Note that even if we removed the other flags from CXXFLAGS, they are added by distutils because they were present when python itself was built.

rgommers commented 3 years ago

I agree that explicitly adding -std=c++17 is not good, but note that -std=c++17 (or the gnu variant, can't remember) is the default for gcc 11.

It'll take a while before we get to gcc 11 in conda-forge though. And it's much easier to see what goes wrong then ("I'm using gcc 11 and my project does not build with that yet").

For both -fopenmp and -std=c++17 I've seen people lose multiple hours without being able to find out what the problem is.

Note that even if we removed the other flags from CXXFLAGS, they are added by distutils because they were present when python itself was built.

Agree that's less of a problem then. We don't all use distutils though:)

isuruf commented 3 years ago

It'll take a while before we get to gcc 11 in conda-forge though.

I'm working on that and I guess we are a couple of weeks away from that.

isuruf commented 3 years ago

For both -fopenmp and -std=c++17 I've seen people lose multiple hours without being able to find out what the problem is.

Sure, but I don't want to change it in a build number change. A version upgrade is what I feel comfortable with.

rgommers commented 3 years ago

A version upgrade is what I feel comfortable with.

That sounds reasonable.

I'm working on that and I guess we are a couple of weeks away from that.

NumPy and PyTorch both don't work with GCC 11 yet, and I expect that to be the case for most large C/C++ projects- GCC 11 is still too new. So that may give some hiccups. I guess compilers == 1.2.0 is then the thing to do to retain GCC 9.3.0?

isuruf commented 3 years ago

-fopenmp is removed in 10+ and -std=c++17 is removed in 11+

rgommers commented 3 years ago

Thanks @isuruf!

rgommers commented 3 years ago

Now that I have looked at gh-50, I think I finally understand how this activation actually works. There's a long list of per-platform env vars in conda_build_config.yaml, and those are then modified at compiler build time. Then they end up in the activation script for a compiler - ~but not for an env, which is why it's hard to figure out where these flags come from (an echo $CXXFLAGS on the command line won't show anything).~

There is still a more fundamental problem here: many of the flags in conda_build_config.yaml do not belong there for local development. They may make sense for building conda packages, although it's not clear to me why everything is there (e.g. use of -O2 vs. -O3 is platform-dependent). However, for local development, there are still way too many flags there. This makes for hard to debug problems (see issues that led to me opening this issue in the first place), and even when you do find the root cause it's annoying to deal with. To stay with the simple -O2/-O3 flags: those interfere with the debug/release/etc. modes that a build system typically offers (example, second table under https://mesonbuild.com/Builtin-options.html#core-options). If I'm debugging an issue and want to see if switching from one mode to another fixes it, then I don't want conda silently inserting a -O flag behind my back.

Basically it looks to me like there should be an activation mode for local development, and one for "build me a conda-forge package". For the latter it seems reasonable to add many flags so they're consistent across conda-forge packages. For the former it's extremely annoying.

isuruf commented 3 years ago

an echo $CXXFLAGS on the command line won't show anything

It wouldn't? Why not?

rgommers commented 3 years ago

It wouldn't? Why not?

Ah wrong env for testing (had runtime libs only), please ignore that part. The rest of my comment is still valid.

isuruf commented 3 years ago

PRs are welcome for this. You can see the two different code paths at https://github.com/conda-forge/ctng-compiler-activation-feedstock/blob/master/recipe/activate-gcc.sh#L83-L97

rgommers commented 3 years ago

Ah those two activation modes already exist, that's great. That should make this a lot simpler than I feared; perhaps I can find time to give this a go.