Anybody recently had success running the math lib tests with this clang++ version (or near it) I can’t quite tell if it’s the clang package or my config that are bad but it can’t find any of the basic libraries (I can include them individually but that’s a mess): clang version 4.0.0-1ubuntu1 (tags/RELEASE_400/rc1)
Given it’s an rc1 I’m not surprised it’s having problems but it doesn’t really make sense.
The real questions: 1) why do we do this, the justification seems pretty thin; and 2) is this something that can be worked around using make/local?
ifneq (,$(findstring clang++,$(CC)))
CC_TYPE ?= clang++
# Clang often seems to find the wrong gcc installation, especially on travis.
# To deal with that we force it to use llvm's bundled libc++ replacement for
# gcc's stdlibc++ here. Plus it's better to rely on just one compiler if we
# can.
CXXFLAGS += -stdlib=libc++
endif
It does work to add it to make/local if I move the make/local include down in the stan-dev/math makefile. @seantalts is that a change we could make so these build issues were easier to override instead of hacking the makefile?
@sakrejda, you found the reason above. I would love for someone to figure out how to fix that issue on Travis - I spent a few days or a week on it and this is the best I could come up with. The issue that Ubuntu’s clang would have trouble finding the right version of stdlibc++ makes (some) sense to me; the issue that Ubuntu’s clang would have trouble finding the (typically bundled?) libc++ seems slightly stranger (so I wonder if it’s more local?). Also, Travis VMs are Trusty and custom in ways that they don’t seem to publish.
Wanting to use make/local to override this setting seems pretty reasonable. Would you mind making an issue and filing a PR if you have a solution?
Another thing we could do would be test if vanilla Trusty has this same issue (I thought it did but it’s worth testing again) and if it doesn’t then we can just add code to the .travis.yml such that when it’s using clang it will use libc++, and leave that section out of the makefiles. Though it does generally feel safer to me to use a single compiler toolchain than to expect that by default people will have two toolchains installed at the correct version. (e.g. if you’re using Trusty and you just install llvm’s tool chain, I don’t think it would work with a system default gcc installation or a lack thereof. Whereas if you just install gcc’s ecosystem, it will work by default).
krzysztof@hagrid:~/packages/math-compare$ cat /proc/version
Linux version 4.10.0-33-generic (buildd@lgw01-52) (gcc version 6.3.0 20170406 (Ubuntu 6.3.0-12ubuntu2) ) #37-Ubuntu SMP Fri Aug 11 10:55:28 UTC 2017
If the math library tests build with stdlib=libc++, gtest can’t find two headers: cxxabi.h and bits/c++config.h, if I add the -I to include those from the stdlib, it also finds conflicting implementations of other stuff and fails. If I set it to use the libstdc++ instead of libc++ then clang and g++ work fine. What I’m missing is what problem is caused by Travis if you let the system pick it’s standard library (omit the -stdlib flag.). I could see this being touchy since Ubuntu manages it using the update-alternatives symlinks and there’s a pretty wide range of combinations available when it comes to c++ compilers and it does seem like an Ubuntu bug.
LLVM’s libc++ library goes through quite a bit of macro gymnastics to get all this right in its own build so I don’t have much hope for us having a simple solution (travis or no travis).
I just verified - Vanilla Trusty’s clang installs do not work with stdlibc++ (only libc++). It’s possible there’s some way to fix that, maybe using update-alternatives, but I don’t see anything obvious and like the libc+±for-clang idea better anyway. Very weird that the new Ubuntu can’t seem to install an appropriate libc++ with clang… Sad, was hoping this would be an easy possible fix.
@sakrejda we could add something to the makefiles that tries to figure out which ubuntu one is running and switch the standard library accordingly?
Not sure what to do about Travis - it’s one of those “well, we’ve already invested so much” + unclear alternatives are a whole lot better. I’m interested in trying out the Jenkins EC2 plugin - do we have an AWS account? If I can get that up and running fairly easily that might be a much better replacement for Travis.
That’s totally fair. At one point I found some information that said Ubuntu was just buggy in its packaging for Trusty, and looks like it might also be for Xenial but in the opposite way 😅
@seantalts and @sakrejda: I think the fix is to add a new make variable just for this link library. I think we should set it with reasonable defaults for the OS / compiler combinations, but if we had it as a variable, a user could then overwrite it easily in their own config.
I didn’t (and still don’t) understand all the subtleties so it wasn’t made into a separate variable sooner.
I think I just got to the same place, I think. Here’s my thinking: one reason this is currently not straightforward is that detect_cc is doing too much. It’s supposed to just be detecting the in-use compiler, not setting compiler flags. The choice of compiler flags can be delayed. Since we use some of the scripts in further projects we should just push off actually combining compiler flags until the end (like CmdStan does for GTEST_CXXFLAGS).
I can do a PR for this in math and make it all pass but I was also thinking of separating CC (the C-compiler) from CXX (the c++ compiler) at the same time. We are shipping cvode and that requires its own options for the c-compiler so it should simplify things to do that. Going by this for the variables: https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html
We already separate the flags for the C compiler vs the C++ compiler (CFLAGS vs CXXFLAGS; CPPFLAGS for both). But we do set the C++ compiler to the same thing as the C compiler. I think this is a reasonable default but you might be right that we don’t need it to be the same necessarily.
Yeah, I think it’s more of a readability issue. Currently our pre-processor flags are in CXXFLAGS even though CPPFLAGS is for pre-processor flags. If we move our pre-processor flags to CPPFLAGS then we’re passing Stan includes to CVODES for building if we use the standard COMPILE.c recipe. If would be nice to follow make conventions (it confused me when I first read these that we weren’t following make conventions) but that means setting up CPPFLAGS in separate variables (like we set up GTEST_* flags). It should be clearer, straightforward, and easier to use in downstream builds. I started this on a branch but I can’t get at a machine that’ll build Stan for a few days.
I guess the proposal is to prepare compiler variables per component. They are different for:
Google test (IIRC these pile on top of the Stan flags)
CVODES (it’s convenient this is the only C project we compile so we could hijack CFLAGS for its options, but that’s just luck, for now).
stan-dev/math
stan-dev/stan (mostly optimization, but the Boost parser lib flags are relevant here)
Breaking these up should make the individual makefiles simpler. Then the standard flags (CPPFLAGS and either CXXFLAGS or CFLAGS) can be set at the time the target is compiled/linked. When the math library makefiles get included in downstream Stan projects they can be sure they’re just borrowing MATH_CXXFLAGS to inject into their own CXXFLAGS rather than accidentally getting something unrelated (as we saw with the detect_cc makefile that injects -stdlib=libc++ into CXXFLAGS). It would be nice to make these modular enough to be useful as includes for rstan/pystan so that there aren’t multiple sets of flags floating around.
When googling around for how people use make, it seems common and maybe even conventional to use CPPFLAGS in the way that we use them (e.g. this stackoverflow q/a).
-stdlib=c++ is not unrelated to any of our projects; it was required to enable C++11 on Trusty across projects.
It seems like make is not really meant for this single-makefile-multiple-project task; all of its implicit targets and default macros assume you want the same flags across targets (with some weird C++ variants). I wonder if there is some other higher level option we could use - like separate makefiles for separate projects or libraries? Could we do that? For example, imagine giving CVODES, the gtest library, the math tests, the stan tests, and the stan compiler all their own makefiles? The math tests could call the CVODES and gtest makefiles recursively… Just brainstorming, curious what you think about this.