Fix GCC 5 Builds #2939

ax3l · 2020-10-08T20:58:34Z

PR Summary

Use GCC 5 to trigger compiler errors with non-default C++14 compilers.

Follow-up to regressions from #2575 and #2610
Fix #2892

export CC=$(which gcc-5)
export CXX=$(which g++-5)
python3 -m pip wheel -v .

PR Checklist

pass black --check yt/
pass isort . --check --diff
pass flake8 yt/
pass flynt yt/ --fail-on-change --dry-run -e yt/extern
New features are documented, with docstrings and narrative docs
Adds a test for any bugs fixed. Adds tests for new features.

.travis.yml

ax3l · 2020-10-08T21:48:26Z

Hi @matthewturk @cphyc,

I saw your PRs in #2575 and #2610 and am not an expert in the places I touch here. Help & hints welcome :)

ax3l · 2020-10-09T07:28:17Z

@RevathiJambunathan does this PR work for you, with respect to #2892?

python3 -m pip uninstall yt
python3 -m pip install -U pip
python3 -m pip install git+https://github.com/ax3l/yt.git@fix-gcc5

ax3l · 2020-10-09T07:32:24Z

tests/travis_install.sh

@@ -42,4 +43,4 @@ fi
 # Step 3: install yt
 $PIP install -e .


fyi: there is no editable install with PEP517 projects anymore [1] [2]

Oh, I didn't know that! I believe a simple fix would be to use $PIP install . instead, right?

Just found out as well.

Even more complicated. According to my understanding, PEP517 packages should be installed locally like this:

python3 -m pip wheel . python3 -m pip install *whl

brave new world, I guess.

wow. Sorry for asking the naive question, but by "PEP517 project" you mean any project with a pyproject.toml file, right ?

Yep... I know...

thanks for bringing this up, I was the person who introduced pyproject.toml to the project but wasn't aware of this :/

cphyc · 2020-10-09T09:22:08Z

It seems the bug triggered on OSX may be fixed by replacing -fopenmp by -fopenmp=libomp (https://stackoverflow.com/questions/47822547/non-default-compilers-gcc-or-vanilla-clang-on-macos-high-sierra-linking-issue/50313990#50313990). I don't understand why it is failing now and used to work before?

cphyc · 2020-10-09T10:01:50Z

I am sorry, I pushed onto your branch by mistake. I meant to push on my fork instead…

ax3l

The compiler flags are wrong.

setup.py

ax3l

Fix compiler flags for OpenMP and C++ std.

ax3l · 2020-10-09T17:56:46Z

@cphyc can you please add the bug label to this PR? the mergable CI test fails due to that missing classification.

ax3l · 2020-10-09T17:58:12Z

setup.py

@@ -36,8 +37,20 @@
 with open("README.md") as file:
    long_description = file.read()

+OMP_CONFIG = defaultdict(
+    lambda: ["-fopenmp"], {"unix": ["-fopenmp"], "msvc": ["/openmp"]}


comment on @cphyc's change here:

unix: this is a bit optimistic but will work as before for GCC and Clang at least

furthermore: AppleClang does not support OpenMP on a vanilla macOS.
If you want to unconditionally add an OpenMP prerequisite, you could install brew install libomp on CI (and need to document this new dependency on macOS). with AppleClang, this will still not be a -fopenmp flag then, but another set of flags.

Practically, one might want to implement a compiler check and keep OpenMP optional. Maybe instead of duplicating and guessing -fopenmp here, refactor check_for_openmp() in setupext.py to return the OpenMP flags (compiler and linker) it found to be working?

ax3l · 2020-10-09T18:05:02Z

@cphyc if compile errors occur in the yt cythonization process then the overall build process does not always abort.

I am no cython / distutils expert, but ideally this should be addressed as well, in an independent PR: users get faulty installs because such errors are not aborting the build process and get lost between many build output messages. Ideally, abort on first build error.

RevathiJambunathan · 2020-10-09T18:54:03Z

@ax3l Thank you for this PR!
I tried installing yt from your branch and I am able to successfully import it.

(yt_env) rjambunathan@zilla:~/Documents/SOFTWARES/yt$ python
Python 3.7.9 (default, Aug 18 2020, 06:24:24) 
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import yt
>>> yt.__version__
'4.0.dev0'

Xarthisius · 2020-10-09T19:09:06Z

setup.py

+)
+
+_COMPILER = get_default_compiler()
+
 if check_for_openmp():


check_for_openmp() makes some assumptions about openmp flags. As a result your code will only be called only if -fopenmp works, otherwise it will default to []

yes, the assumption about flags (tested and applied) should be in one place, e.g. check_for_openmp. we currently duplicate the flags (and tested assumptions).

note that I did not put this into the change set.

pep8speaks · 2020-10-09T20:01:26Z

Hi there, @ax3l! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Hooray! 🎆

Comment last updated at 2020-12-02 16:19:36 UTC

ax3l · 2020-10-09T20:02:54Z

@cphyc I simplified and fixed the compiler logic changes you pushed on this PR.

If you don't mind, I would love to not add features in this bugfix PR. If you like to widen the support for compilers and OpenMP to more platforms, can we do so in a follow-up PR? I would suggest to improve the compile tests performed in check_for_openmp by just testing more known flags of popular compilers. Let's focus on the bugfix here.

setup.py

matthewturk · 2020-10-09T20:21:30Z

Oh my goodness, thank you for taking such a close look at this. C++ ABIs and whatnot are things I had not even considered when I was looking at the level of C++ support we needed. (Although, maybe I'm also a bit oversensitive on this because I remember the endless discussion of C++ ABI breakages back in the early 2000s... ;-)

…

On Fri, Oct 9, 2020 at 3:19 PM Axel Huebl ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In setup.py <#2939 (comment)>: > @@ -65,6 +70,7 @@ "FIXED_INTERP": "yt/utilities/lib/fixed_interpolator.cpp", "ARTIO_SOURCE": glob.glob("yt/frontends/artio/artio_headers/*.c"), "CPP14_FLAG": CPP14_FLAG, + "CPP03_FLAG": CPP03_FLAG, If possible, build all binaries with the same C++ standard. Especially between C++03 and C++11 were ABI breaks in the STL and with MSVC the math libs and STL will also cause problems. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2939 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO624F6MPWL7MEKRIRTSJ5V4HANCNFSM4SJICCGA> .

cphyc · 2020-10-14T10:58:01Z

Hm, I wonder why macOS still tries to build with OpenMP and what is wrong with the erf on Windows?

According to storpipfugl/pykdtree#51, this is an issue on pykdtree's side. One solution is to not use openmp when compiling on MacOSX. This can be done by setting USE_OMP=0 in the environment variables.

I have tried that in cphyc@2ad8d1b, and it seems to do the trick, but then yt's build fails on OSX... I am still investigating.

cphyc · 2020-10-14T11:35:13Z

Good news, with the HEAD of https://github.com/cphyc/yt/tree/fix-gcc5 (cphyc@af550d9), the OSX job now builds! Note that it still doesn't work with Windows.

[EDIT]: now working with Windows

cphyc · 2020-10-14T13:35:23Z

I've issued my fixes in another PR (#2943), which hopefully should contribute to make the tests pass on this one.

cphyc

Provided the tests pass, I would be happy to merge this!

cphyc · 2020-10-15T15:35:21Z

yt/utilities/lib/field_interpolation_tables.pxd

+cdef extern from "<cmath>" namespace "std":
+    bint isnormal(double x) nogil


Note: for some reason, Visual Studio doesn't happy about this. However, if we replace by

Suggested change

cdef extern from "<cmath>" namespace "std":

bint isnormal(double x) nogil

from libc.math cimport isnormal

or

Suggested change

cdef extern from "<cmath>" namespace "std":

bint isnormal(double x) nogil

cdef extern from "<math.h>":

bint isnormal(double) nogil

it compiles.

cphyc · 2020-10-28T14:58:47Z

/isort

cphyc · 2020-10-28T18:05:39Z

@neutrinoceros @Xarthisius @ax3l I have pushed some more commits to make the Windows build build, do you think this is ready to go? I have added some code myself so I don't feel like pushing the merge button ;)

[EDIT]: I just realized b181ee4 may cancel the effect of 3347196 (esp. the cdef extern from "<cmath>" namespace "std": bit, which was preventing the windows build). Maybe it would be worth reapplying b7ccea3, to ensure it compiles with GCC5.

[EDIT2]: I have now checked locally that the gcc-5 build works, so this is ready to go on my side

Xarthisius · 2020-11-18T03:50:22Z

.travis.yml

@@ -17,6 +17,8 @@ addons:
      - proj-bin
      - libgeos-dev
      - libopenmpi-dev
+      - gcc-5
+      - g++-5


Our testing setup change a bit with the merge of #2963, so this will need to be updated. However, I don't think we want to exclusively use gcc-5 in all our builds. Instead it would be worthwhile to set up a separate, compile-only test.

Xarthisius · 2020-11-18T03:52:36Z

setup.py

+CPP03_CONFIG = defaultdict(
+    lambda: ["-std=c++03"], {"unix": ["-std=c++03"], "msvc": ["/std:c++03"]}
+)


I don't think it's used anywhere. At least git grep doesn't show anything. If I'm not missing anything, let's not add things that might be used, but are not.

Xarthisius · 2020-11-18T03:54:02Z

yt/utilities/lib/cykdtree/kdtree.pyx

@@ -3,7 +3,7 @@
 # distutils: sources = yt/utilities/lib/cykdtree/c_utils.cpp
 # distutils: depends = yt/utilities/lib/cykdtree/c_kdtree.hpp, yt/utilities/lib/cykdtree/c_utils.hpp
 # distutils: language = c++
-# distutils: extra_compile_args = -std=c++03
+# distutils: extra_compile_args = CPP14_FLAG


Shouldn't it be CPP03_FLAG that I couldn't find in previous comment ?

Xarthisius · 2020-11-18T03:54:15Z

yt/utilities/lib/cykdtree/utils.pyx

@@ -2,7 +2,7 @@
 # distutils: sources = yt/utilities/lib/cykdtree/c_utils.cpp
 # distutils: depends = yt/utilities/lib/cykdtree/c_utils.hpp
 # distutils: language = c++
-# distutils: extra_compile_args = -std=c++03
+# distutils: extra_compile_args = CPP14_FLAG


Shouldn't it be CPP03_FLAG that I couldn't find in previous comment ?

It depends, this was changed assuming that one flag was better than two. In this approach, the default C++ language version is C++14. If we want to make minimal changes, then this should be replaced by CPP03_FLAG. What do you think is best?

Xarthisius · 2020-11-18T04:00:48Z

setup.py

+
+_COMPILER = get_default_compiler()
+
+omp_args, _ = check_for_openmp()


If we are distinguishing between compile and link flags inside check_for_openmp(), shouldn't we do the same thing during cythonization, i.e. use two separate aliases? Otherwise, why return two variables?

These files includes C++11 features and GCC<6 does still default to C++98/03.

Co-authored-by: Corentin Cadiou <[email protected]>

munkm · 2020-12-02T16:18:39Z

/isort

ax3l · 2020-12-04T22:23:11Z

Thank you everyone for getting this PR done and merged.

@RevathiJambunathan just a last check, does the yt master branch compile again on your system with GCC 5?

munkm · 2020-12-04T23:41:23Z

Thank you so much for the fix!! We really appreciate your contribution.

ax3l changed the title ~~[CI] [Temporary] GCC 5~~ [WIP] Fix GCC 5 Builds Oct 8, 2020

ax3l commented Oct 8, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

ax3l force-pushed the fix-gcc5 branch from 63bcc81 to 63b5681 Compare October 8, 2020 21:42

ax3l force-pushed the fix-gcc5 branch 4 times, most recently from ce27c43 to b7e178d Compare October 9, 2020 07:24

ax3l changed the title ~~[WIP] Fix GCC 5 Builds~~ Fix GCC 5 Builds Oct 9, 2020

ax3l commented Oct 9, 2020

View reviewed changes

ax3l mentioned this pull request Oct 9, 2020

Fresh yt installation not importing #2892

Closed

ax3l force-pushed the fix-gcc5 branch 2 times, most recently from 5fd256b to a231da6 Compare October 9, 2020 07:49

ax3l commented Oct 9, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

ax3l commented Oct 9, 2020

View reviewed changes

Xarthisius reviewed Oct 9, 2020

View reviewed changes

ax3l force-pushed the fix-gcc5 branch 3 times, most recently from f0a7b1e to 9647efd Compare October 9, 2020 20:15

ax3l commented Oct 9, 2020

View reviewed changes

setup.py Show resolved Hide resolved

cphyc added bug infrastructure Related to CI, versioning, websites, organizational issues, etc labels Oct 14, 2020

cphyc mentioned this pull request Oct 14, 2020

[Bugfix] CPP support + OSX build fix. #2943

Merged

cphyc approved these changes Oct 15, 2020

View reviewed changes

cphyc force-pushed the fix-gcc5 branch 2 times, most recently from adf26db to 5adf61f Compare October 29, 2020 21:20

cphyc added the yt-4.0 feature targeted for the yt-4.0 release label Nov 13, 2020

Xarthisius requested changes Nov 18, 2020

View reviewed changes

ax3l and others added 8 commits November 18, 2020 09:20

image_samplers, geometry: C++14 flags

17242bd

These files includes C++11 features and GCC<6 does still default to C++98/03.

More robust build

9e85549

Fix compiler flags for OpenMP and C++ std.

ff5eb22

OpenMP: Unify Check & Flag logic

2cf6649

Compile everything in C++14

116f820

Co-authored-by: Corentin Cadiou <[email protected]>

[isort-command] fixes

688ecc2

Fix GCC-5 build

09187c1

Use CPP03 flag where they used to be

bd6c0f2

cphyc force-pushed the fix-gcc5 branch from 5adf61f to bd6c0f2 Compare November 18, 2020 09:22

[isort-command] fixes

92e25a8

Xarthisius approved these changes Dec 2, 2020

View reviewed changes

munkm merged commit 1905842 into yt-project:master Dec 4, 2020

ax3l deleted the fix-gcc5 branch December 4, 2020 22:20

neutrinoceros mentioned this pull request Jun 13, 2022

TST: start testing on Python 3.11 #3956

Merged

		cdef extern from "<cmath>" namespace "std":
		bint isnormal(double x) nogil

	cdef extern from "<cmath>" namespace "std":
	bint isnormal(double x) nogil
	from libc.math cimport isnormal


		_COMPILER = get_default_compiler()

		omp_args, _ = check_for_openmp()

Fix GCC 5 Builds #2939

Fix GCC 5 Builds #2939

Conversation

ax3l commented Oct 8, 2020 • edited Loading

PR Summary

PR Checklist

ax3l commented Oct 8, 2020

ax3l commented Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ax3l Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cphyc commented Oct 9, 2020

cphyc commented Oct 9, 2020

ax3l left a comment

Choose a reason for hiding this comment

ax3l left a comment

Choose a reason for hiding this comment

ax3l commented Oct 9, 2020

ax3l Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

ax3l Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

ax3l commented Oct 9, 2020 • edited Loading

RevathiJambunathan commented Oct 9, 2020

Choose a reason for hiding this comment

ax3l Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

pep8speaks commented Oct 9, 2020 • edited Loading

Comment last updated at 2020-12-02 16:19:36 UTC

ax3l commented Oct 9, 2020 • edited Loading

matthewturk commented Oct 9, 2020 via email

cphyc commented Oct 14, 2020 • edited Loading

cphyc commented Oct 14, 2020 • edited Loading

cphyc commented Oct 14, 2020 • edited Loading

cphyc left a comment

Choose a reason for hiding this comment

cphyc Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

cphyc commented Oct 28, 2020

cphyc commented Oct 28, 2020 • edited Loading

Xarthisius Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

munkm commented Dec 2, 2020

ax3l commented Dec 4, 2020

munkm commented Dec 4, 2020

ax3l commented Oct 8, 2020 •

edited

Loading

ax3l commented Oct 9, 2020 •

edited

Loading

ax3l Oct 9, 2020 •

edited

Loading

ax3l Oct 9, 2020 •

edited

Loading

ax3l Oct 9, 2020 •

edited

Loading

ax3l commented Oct 9, 2020 •

edited

Loading

ax3l Oct 9, 2020 •

edited

Loading

pep8speaks commented Oct 9, 2020 •

edited

Loading

ax3l commented Oct 9, 2020 •

edited

Loading

cphyc commented Oct 14, 2020 •

edited

Loading

cphyc commented Oct 14, 2020 •

edited

Loading

cphyc commented Oct 14, 2020 •

edited

Loading

cphyc Oct 15, 2020 •

edited

Loading

cphyc commented Oct 28, 2020 •

edited

Loading

Xarthisius Nov 18, 2020 •

edited

Loading