molpopgen / fwdpp

fwdpp is a C++ template library for implementing efficient forward-time population genetic simulations
http://fwdpp.readthedocs.io
GNU General Public License v3.0
27 stars 11 forks source link

OSX/clang compatibility broken by "mutation keys return pair of key/count in sampler, to aid downstream filtering" #44

Closed ilovezfs closed 7 years ago

ilovezfs commented 7 years ago

It looks like cxx11 compatibility was broken by ff696cb80e196b1ea51a3691ce1d943f0e985def.

Before this commit, the build is successful with cxx11, but after this commit I have to set libstdcxx or the build fails on macOS (test on 10.10, 10.11, and 10.12) with the following error:

    /usr/bin/clang++ -std=c++11 -stdlib=libc++ -DHAVE_CONFIG_H -I. -I..   -F/usr/local/Frameworks  -w -pipe -march=native -mmacosx-version-min=10.11 -O2 -MT integration/sugar_matrixTest.o -MD -MP -MF $depbase.Tpo -c -o integration/sugar_matrixTest.o integration/sugar_matrixTest.cc &&\
    mv -f $depbase.Tpo $depbase.Po
In file included from integration/sugar_matrixTest.cc:9:
In file included from ../fwdpp/sugar/matrix.hpp:76:
../fwdpp/sugar/matrix_details.hpp:161:17: error: no matching constructor for initialization of 'std::vector<std::size_t>' (aka 'vector<unsigned long>')
                std::vector<std::size_t>(n.begin(), n.end()),
                ^                        ~~~~~~~~~~~~~~~~~~

Full build log here: https://gist.github.com/ilovezfs/e32657c623110767e9093932f28eb773

molpopgen commented 7 years ago

This is another clang issue specific to OS X that doesn't replicate elsewhere. I'll "fix" this, but what is happening here on OS X is wrong. Clang 3.6 through 3.9 on Linux all do the right thing.

I've deleted the 0.5.4 tag for right now, too, and will re-release when this is fixed.

ilovezfs commented 7 years ago

Thanks. It would be a shame to have to set ENV.libstdcxx since we've been trying to get rid of all of those. Right now it's down to 7 formulae out of 4,300 in Homebrew Core and Homebrew Science combined.

molpopgen commented 7 years ago

This is fixed. Curiously, the libstdcxx apparently was generating the correct code (I did not test that myself, though). I also changed the title to reflect what was actually happening--the c++11 was fine, actually.

ilovezfs commented 7 years ago

@molpopgen by the way, it's not about the compiler itself. GCC-6 will fail in the same way on the old code:

/usr/local/bin/g++-6  -w -pipe -march=core2 -msse4 -mmacosx-version-min=10.11 -O2  -L/usr/local/lib -F/usr/local/Frameworks -Wl,-headerpad_max_install_names -o diploid_ind diploid_ind.o  -lgsl -lgslcblas -lz  -lsequence
Undefined symbols for architecture x86_64:
  "Sequence::PolyTable::assign(__gnu_cxx::__normal_iterator<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const*, std::vector<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > >, __gnu_cxx::__normal_iterator<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const*, std::vector<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > >)", referenced from:
      _main in diploid_ind.o
  "Sequence::operator<<(std::basic_ostream<char, std::char_traits<char> >&, Sequence::PolyTable const&)", referenced from:
      _main in diploid_ind.o
ld: symbol(s) not found for architecture x86_64
collect2: error: ld returned 1 exit status
make[2]: *** [diploid_ind] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2
molpopgen commented 7 years ago

I cannot replicate that with gcc 6 on Linux. On Wed, Dec 7, 2016 at 07:22 ilovezfs notifications@github.com wrote:

@molpopgen https://github.com/molpopgen by the way, it's not about the compiler itself. GCC-6 will fail in the same way on the old code:

/usr/local/bin/g++-6 -w -pipe -march=core2 -msse4 -mmacosx-version-min=10.11 -O2 -L/usr/local/lib -F/usr/local/Frameworks -Wl,-headerpad_max_install_names -o diploid_ind diploid_ind.o -lgsl -lgslcblas -lz -lsequence Undefined symbols for architecture x86_64: "Sequence::PolyTable::assign(gnu_cxx::__normal_iterator<std::pair<double, std::cxx11::basic_string<char, std::char_traits, std::allocator > > const, std::vector<std::pair<double, std::cxx11::basic_string<char, std::char_traits, std::allocator > >, std::allocator<std::pair<double, std::cxx11::basic_string<char, std::char_traits, std::allocator > > > > >, gnu_cxx::__normal_iterator<std::pair<double, std::cxx11::basic_string<char, std::char_traits, std::allocator > > const, std::vector<std::pair<double, std::cxx11::basic_string<char, std::char_traits, std::allocator > >, std::allocator<std::pair<double, std::cxx11::basic_string<char, std::char_traits, std::allocator > > > > >)", referenced from: _main in diploid_ind.o "Sequence::operator<<(std::basic_ostream<char, std::char_traits >&, Sequence::PolyTable const&)", referenced from: _main in diploid_ind.o ld: symbol(s) not found for architecture x86_64 collect2: error: ld returned 1 exit status make[2]: [diploid_ind] Error 1 make[1]: [all-recursive] Error 1 make: *** [all] Error 2

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/molpopgen/fwdpp/issues/44#issuecomment-265475237, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHnH5r-yeKkM5Chn5aCvi8g0N3NoatOks5rFs8KgaJpZM4LGVyz .

ilovezfs commented 7 years ago

Right, the point being it's a macOS thing, not an Apple's-clang thing.

ilovezfs commented 7 years ago

@molpopgen unfortunately although your fix got it working with clang, it's broken with GCC:

/usr/local/bin/g++-6  -w -pipe -march=core2 -msse4 -mmacosx-version-min=10.11 -O2  -L/usr/local/lib -F/usr/local/Frameworks -Wl,-headerpad_max_install_names -o diploid_ind diploid_ind.o  -lgsl -lgslcblas -lz  -lsequence
Undefined symbols for architecture x86_64:
  "Sequence::PolyTable::assign(__gnu_cxx::__normal_iterator<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const*, std::vector<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > >, __gnu_cxx::__normal_iterator<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const*, std::vector<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > >)", referenced from:
      _main in diploid_ind.o
  "Sequence::operator<<(std::basic_ostream<char, std::char_traits<char> >&, Sequence::PolyTable const&)", referenced from:
      _main in diploid_ind.o
ld: symbol(s) not found for architecture x86_64
collect2: error: ld returned 1 exit status
make[2]: *** [diploid_ind] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2
molpopgen commented 7 years ago

So this is not the same issue. Here, you are having trouble linking the example programs to libsequence, which is only a dependency for building the examples. (./configure checks for its presence, and will not build examples if not there.) I am guessing you have libseqeuence installed and compiled using clang, but are attempting to link object code generated using gcc-6. That combo is not guaranteed to work due to ABI issues.

molpopgen commented 7 years ago

On El Capitan with homebrew gcc-6, and libsequence/master (same as 1.9.0) compiled & installed using brew/gcc6, everything compiles and all unit tests pass. So your latest issue is a toolchain problem, not a fwdpp problem.

I can recreate what you posted in the "reverse" direction, too. Install lib sequence with gcc-6, then attempt to compile fwdpp examples with Xcode clang. I get the same missing symbols.

ilovezfs commented 7 years ago

Yup, that fixed it, thanks. It seems I also needed to re-build boost with gcc. Did you need to too?

molpopgen commented 7 years ago

I did not, but I do not remember if I have 'brew boost, Anaconda boost, or one I compiled myself on my system.

I've pulled the 0.5.4 release for right now. I'm tidying a few things up.

In the end, though, isn't GCC6 moot on OSX/brew? Its existence in home-brew seems largely academic to me, and its presence has caused problems for people using some of my code. They've installed a dependency like libsequence from 'brew, but then decided to use GCC6 to compile a package depending on that library (but not in homebrew-science), and got failures to link due to ABI issues.

On Wed, Dec 7, 2016 at 4:49 PM ilovezfs notifications@github.com wrote:

Yup, that fixed it, thanks. It seems I also needed to re-build boost with gcc. Did you need to too?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/molpopgen/fwdpp/issues/44#issuecomment-265620043, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHnH6WiVnmBtYWDle4lLqGA2zCUt9Qwks5rF1OGgaJpZM4LGVyz .

ilovezfs commented 7 years ago

Nope, not moot.

We have several versions of gcc, and try to mark known failure cases with gcc with the fails_with DSL (which may be restricted to certain versions of gcc or macOS).

By default anything with a fails_with clang (which may be restricted to specific versions of clang or macOS), will attempt to use gcc or prompt the user to install gcc if not already installed. If the latest version of gcc is also marked with a fails, then brew will attempt to use the next most recent version of gcc, and prompt the user to install it if not already installed, etc.

gcc is also the default way to satisfy some special requirements such as fortran and openmp. Recently, llvm 3.9 was added as a new way to satisfy the openmp requirement, and may eventually be the default way to satisfy it.

When gcc is used to build the binaries of a formula during bottling for whatever reason, and the binaries end up linked to libgcc, we will also try to add depends_on "gcc" explicitly to make sure gcc gets installed before CI tests the formula when it's a reverse dependency of something else that's being modified in a PR, and so that it can be revision bumped when the gcc libraries are version bumped.

In homebrew/science gcc is particularly relevant because everything in homebrew/science tries to be compatible with both macOS and Linux, and because the special requirements are more common.

That said, in the general case where gcc is not being used deliberately by brew, we only officially support clang in homebrew/core. Anything that cannot build with clang on the latest version of macOS is subject to boneyarding (being moved to homebrew/boneyard) at any time, and typically that will be determined based on whether upstream has expressed an intent to restore clang compatibility or not. If the clang breakage only occurs on versions of macOS that are not the current version, that is more likely to be tolerated with a longer term gcc dependency assuming it's the latest version of gcc.

Anyway, probably much more info than you wanted, but suffice it to say gcc is far from an academic concern, and is a rather popular formula.

molpopgen commented 7 years ago

Does this work recursively to satisfy dependencies? For C++, it sounds a bit dangerous. Basically, if the last thing in a series of dependencies need GCC >= X, then all dependencies need that, too, which could quickly ripple across most of 'brew.

On Wed, Dec 7, 2016 at 7:05 PM ilovezfs notifications@github.com wrote:

Nope, not moot.

We have several versions of gcc, and try to mark known failure cases with gcc with the fails_with DSL (which may be restricted to certain versions of gcc or macOS).

By default anything with a fails_with clang (which may be restricted to specific versions of clang or macOS), will attempt to use gcc or prompt the user to install gcc if not already installed. If the latest version of gcc is also marked with a fails, then brew will attempt to use the next most recent version of gcc, and prompt the user to install it if not already installed, etc.

gcc is also the default way to satisfy some special requirements such as fortran and openmp. Recently, llvm 3.9 was added as a new way to satisfy the openmp requirement, and may eventually be the default way to satisfy it.

When gcc is used to build the binaries of a formula during bottling for whatever reason, and the binaries end up linked to libgcc, we will also try to add depends_on "gcc" explicitly to make sure gcc gets installed before CI tests the formula when it's a reverse dependency of something else that's being modified in a PR, and so that it can be revision bumped when the gcc libraries are version bumped.

In homebrew/science gcc is particularly relevant because everything in homebrew/science tries to be compatible with both macOS and Linux, and because the special requirements are more common.

That said, in the general case where gcc is not being used deliberately by brew, we only officially support clang in homebrew/core. Anything that cannot build with clang on the latest version of macOS is subject to boneyarding (being moved to homebrew/boneyard) at any time, and typically that will be determined based on whether upstream has expressed an intent to restore clang compatibility or not. If the clang breakage only occurs on versions of macOS that are not the current version, that is more likely to be tolerated with a longer term gcc dependency assuming it's the latest version of gcc.

Anyway, probably much more info than you wanted, but suffice it to say gcc is far from an academic concern, and is a rather popular formula.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/molpopgen/fwdpp/issues/44#issuecomment-265640592, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHnH0TCXLmp8ng3eSGbNpRjY8QR0OM1ks5rF3M_gaJpZM4LGVyz .

ilovezfs commented 7 years ago

The user will get a warning if there's a C++ standard mismatch, but no it's not recursively enforced up and down the dependency tree. With special requirements there will often be an option in the dependencies for the same requirement so we'll have like foo

depends_on :fortran => :optional
if build.with? "fortran"
  depends_on "bar" => "with-fortran"
else
  depends_on "bar"
end

and bar

depends_on :fortran => :optional
molpopgen commented 7 years ago

I see. That will be quite fragile. Standard mismatches are not a problem. A C++11 library can link to a C++98 library with no problem, unless the later uses std::auto_ptr, which is deprecated.

The much bigger problem is ABI compatibility. Details are at https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html. GCC6 changes how C++ code is laid out in object files, unless GCC6 is configured to use the "old" layout. So, two libs compiled with the same standard, but different GCC (say 5 vs 6), will fail to link. In my experience, it is even harder to track clang/GCC ABI compatibility.

I am not sure the extent to which C is affected by these same issues. My guess is not nearly so much, but who knows.

On Wed, Dec 7, 2016 at 7:27 PM ilovezfs notifications@github.com wrote:

The user will get a warning if there's a C++ standard mismatch, but no it's not recursively enforced up and down the dependency tree. With special requirements there will often be an option in the dependents for the same requirement so we'll have like foo

depends_on :fortran => :optional if build.with? "fortran" depends_on "bar" => "with-fortran" else depends_on "bar" end

and bar

depends_on :fortran => :optional

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/molpopgen/fwdpp/issues/44#issuecomment-265643400, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHnH9VMwoI_0wtcFuf-jTOypJcdJhYhks5rF3i5gaJpZM4LGVyz .

ilovezfs commented 7 years ago

Truth be told, I tend to agree with you about this but at this point I think we're in the minority. It's not even possible in brew to do things like depends_on "bar" => "gcc6" if ENV.compiler == :gcc. That is possible with :cxx11 if the dependency has option :cxx11. The only thing we have currently that does enforcement up and down the tree is :universal for combo 32-bit/64-bit binaries.

ilovezfs commented 7 years ago

@molpopgen if you want to open a PR for stricter C++ handling, here's how :universal is implemented: https://github.com/Homebrew/brew/search?utf8=✓&q=inherited_options+OR+inherited_options_for&type=Code

Also you could open a proposal if you don't want to implement it yourself here: https://github.com/Homebrew/brew-evolution

ilovezfs commented 7 years ago

By the way when you re-publish the new version, you'll probably want to make it 0.5.5 or 0.5.4.1 since tags won't be updated when people git pull.

molpopgen commented 7 years ago

Realistically, no one will have pulled 0.5.4 yet. I've not made it requires for anything else, and all of this has happened in one day. Anyone who has a problem can delete their repo and re -clone. On Wed, Dec 7, 2016 at 20:09 ilovezfs notifications@github.com wrote:

By the way when you re-publish the new version, you'll probably want to make it 0.5.5 or 0.5.4.1 since tags won't be updated when people git pull.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/molpopgen/fwdpp/issues/44#issuecomment-265648077, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHnH9BxwwNP4Wumb3ds5O7XJl5loIqGks5rF4KRgaJpZM4LGVyz .