One Compiler Per OS

I’m assuming code supported by clang 5 will be supported by clang 6 but not necessarily the reverse. But I’m happy to limit our support to the more recent one if that’s what two people are clamoring for.

I think I like @ChrisChiasson’s proposal, @syclik do you like it? I think it fits @bgoodri’s criteria though in some sense we had already resolved this in a different way and moved on to the organizational issues over who is responsible for fixing bugs for which compilers :P And I’m sticking with gcc 5 for now until someone shows me gcc 7 compiling or the RTools people actually successfully complete the change to gcc 7 in the next couple of years.

To flesh out the proposal a little:

Math repo

PRs:

  • mac / clang 6 / libc++ Threading
  • linux / gcc 5 / stdlibc++ OpenMPI
  • mac clang OR linux clang distribution tests (without row vectors)

Additional tests on merge to develop [0]

  • linux / gcc 5 / stdlibc++ Threading
  • mac / clang OR linux / gcc GPU
  • mac clang OR linux clang distribution tests (with row vectors)

I don’t think we have any Windows Math repo tests right now - I think they have been broken for a while? I think the only place the math library gets implicitly tested on Windows is during the CmdStan integration tests. But we’ll be sticking with the RTools compiler (gcc 4.9.3) for those tests.

[0] we can add an interstitial branch if this causes develop to become unstable.

1 Like

That last proposal sounds great. I’d like to request that we write out our policy somewhere.

g++-5 is not used by much of anyone and is not being updated by the gcc developers anymore. Again, I don’t like saying "g++-7 is crashing on one test, so let’s not use it". If it is a g++-7 bug, we should report it. If it is a Stan bug, we should fix it. If it is somewhere in between, we should work around it. I believe stan::math::lgamma actually works fine with g++-7 but the test for it is uncompilable for some reason. But with C++14, there is no reason for stan::math::lgamma to be calling boost::math::lgamma anyway, since there is a std::lgamma.

Earlier you and others claimed it was important to test the default compiler for Ubuntu. gcc+±5 is the default compiler for your Xenial Ubuntu server.

It sounds like you know how to fix it. I retain my position - once someone shows me gcc-7 working on our Math unit tests, I’ll be happy to switch it over in the CI config. Our tests also have to work with gcc-5 (as far as I understand it) because of the way MPI should be compiled with the default system compiler.

That is true, although Xenial is not the current LTS. I’ll look into the bug more.

1 Like

clang > 3.8 also seems broken on Xenial unless I add -stdlib=libc++… any Ubuntu people know if I need to do something besides echo CC=clang++-6.0 > make/local for that to work? Or I guess whenever we use clang on linux it’s okay to use libc++?

I don’t know what’s going on, but -stdlib=libc++ always broke my builds (and the builds of folks who used the models I was working on) on 16.04 and 18.04. I only used whatever the default clang compilers were. Are these new VMs? Or could this possibly be a weird config issue with a computer?

What’s the bug without it? Hard to help when I don’t get this error

Oh, sorry, should have put the error message (here is one of many like it):

In file included from test/unit/math/fwd/mat/fun/to_fvar_test.cpp:1:
In file included from ./stan/math/fwd/mat.hpp:4:
In file included from ./stan/math/fwd/core.hpp:4:
In file included from ./stan/math/fwd/core/fvar.hpp:5:
In file included from ./stan/math/prim/scal/fun/is_nan.hpp:4:
In file included from lib/boost_1.66.0/boost/math/special_functions/fpclassify.hpp:15:
lib/boost_1.66.0/boost/config/no_tr1/cmath.hpp:21:12: fatal error: 'cmath' file not found
#  include <cmath>
           ^~~~~~~
1 error generated.
clang++-6.0 -Wall -I . -isystem lib/eigen_3.3.3 -isystem lib/boost_1.66.0 -isystem lib/sundials_3.1.0/include -std=c++1y -DBOOST_RESULT_OF_USE_TR1 -DBOOST_NO_DECLTYPE -DBOOST_DISABLE_ASSERTS -DBOOST_PHOENIX_NO_VARIADIC_EXPRESSION -Wno-unused-function -Wno-uninitialized -DGTEST_USE_OWN_TR1_TUPLE -DGTEST_HAS_PTHREAD=0 -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DGTEST_USE_OWN_TR1_TUPLE -DGTEST_HAS_PTHREAD=0 -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DNO_FPRINTF_OUTPUT -pipe  -c -o lib/gtest_1.7.0/src/gtest-all.o lib/gtest_1.7.0/src/gtest-all.cc
In file included from lib/gtest_1.7.0/src/gtest-all.cc:39:
lib/gtest_1.7.0/include/gtest/gtest.h:54:10: fatal error: 'limits' file not found
#include <limits>
         ^~~~~~~~
1 error generated.
<builtin>: recipe for target 'lib/gtest_1.7.0/src/gtest-all.o' failed
make: *** [lib/gtest_1.7.0/src/gtest-all.o] Error 1

This is on Ben Goodrich’s Xenial server, same one as always (but we were always using clang 3.8 in the past).

Those are the errors I get with -stdlib=libc++ hahaha

1 Like

(╯°□°)╯/ ┻━┻)

1 Like

does that server have g++ installed?

Yes, a few of them.

I clone a fresh math repo, here’s my Ubuntu:

krzysztof@sph-3s9bjl2linu:~/packages/math$ cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
DISTRIB_DESCRIPTION="Ubuntu 16.04.4 LTS"

I tried the specific clang+±6.0 you have failing and it works:

krzysztof@sph-3s9bjl2linu:~/packages/math$ clang++-6.0 -Wall -I . -isystem lib/eigen_3.3.3 -isystem lib/boost_1.66.0 -isystem lib/sundials_3.1.0/include -std=c++1y -DBOOST_RESULT_OF_USE_TR1 -DBOOST_NO_DECLTYPE -DBOOST_DISABLE_ASSERTS -DBOOST_PHOENIX_NO_VARIADIC_EXPRESSION -Wno-unused-function -Wno-uninitialized -DGTEST_USE_OWN_TR1_TUPLE -DGTEST_HAS_PTHREAD=0 -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DGTEST_USE_OWN_TR1_TUPLE -DGTEST_HAS_PTHREAD=0 -isystem lib/gtest_1.7.0/include -isystem lib/gtest_1.7.0 -O3 -DNO_FPRINTF_OUTPUT -pipe  -c -o lib/gtest_1.7.0/src/gtest-all.o lib/gtest_1.7.0/src/gtest-all.cc

If I add --stdlib=libc++ I get:

lib/gtest_1.7.0/include/gtest/gtest.h:54:10: fatal error: 'limits' file not
      found
#include <limits>
         ^~~~~~~~
1 error generated.

If I add --stdlib=libstdc++ it works again. If I run with the default and -H I get that the limits file is available from:

/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/limits
/usr/include/clang/6.0.0/include/limits.h

If I run with --stdlib=libstdc++ I get the above plus /usr/include/limits.h.

If I run with --stdlib=libc++ and -H I get:

/usr/include/clang/6.0.0/include/limits.h
/usr/include/limits.h

So… it should find the file… (which does in fact exist on my system and contain the right thing…) This is an include-ordering problem because if I add -isystem /usr/include (which has limits.h all of a sudden it compiles… so… does anybody remember why we have all these -isystem options? :/

Hmm, will have to look into this further. The ‘-isystem’ stuff doesn’t emit warnings for files included in that fashion, maybe there are other reasons too.

It’s not so much the -isystem itself as the fact that we include a lot of stuff that way so apparently we’re fighting with the system headers for some reason.

Yeah, maybe that was the original logic. Probably would make sense to not worry about stopping warnings but just pipe them to a log.

@seatbelts: sorry we were over confident about that solution, it looks like our include order might be triggering this problem even though the error itself looks nonsensical (“limits.h” is on the search path).

We probably need to look at our include order more broadly

Yes, the isystem was originally introduced to suppress warnings from Boost and Eigen. A couple relevant points:

  • since we’ve used it, Boost and Eigen have gotten a lot better with their warnings. I don’t think they’re fully gone yet, but it’s close

  • it’s not well documented by any of the compilers, but the way includes are handled by isystem and I differ from compiler to compiler and I’ve even seen it differ version to version within a compiler

Yes, we should look at include order more broadly, but I don’t think there’s going to be a neat way to solve this problem. Since it differs within versions of a compiler, we can only set up defaults that work across most situations and then special-case all the places where it fails. Right now, we’re not doing that.

If I recall, the place where this really messes up (besides for the compiler errors you’re seeing) is with system installs of Boost. The includes don’t pick out the one in the math library if you’re not careful. I thought I wrote this all out somewhere… let me look for it.

I can’t find the old thread / pull request comment / issue comment / whatever I think I wrote.