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:
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 
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.
 we can add an interstitial branch if this causes develop to become unstable.
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.
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?
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
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).
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? :/
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).
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.