norlab-ulaval / libpointmatcher

An Iterative Closest Point (ICP) library for 2D and 3D mapping in Robotics
BSD 3-Clause "New" or "Revised" License
1.59k stars 544 forks source link

libnabo (master) started to require c++11 for its dependers #202

Closed HannesSommer closed 7 years ago

HannesSommer commented 7 years ago

This is a difficult and rather urgent (currently two master branches do not compile together) issue iff libpointmatcher cannot make that step, yet.

I only see a few affordable options:

  1. require c++11 for libpointmatcher and all its dependers, possibly only if the libnabo version found is actually requiring c++11 (the forward option)
  2. revert nabo to pre c++11 (the backward option)
  3. make libpointmatcher (master) depend officially on a former version of libnabo (e.g. <=1.0.6) (the pinning option)
  4. Hide the "third_party/any.h" (optionally) in libnabo from the depender packages (the mixed option). This would allow nabo to depend on c++11 while pointmatcher doesn't.
  5. Hide the use of nabo from libpointmatcher's dependers and only require c++11 for libpointmatcher but not its dependers (another mixed option, actually the simples of all, IMO)

The mixed options would satisfying some CI jobs and break this chain of c++11 requirement that otherwise continues for the packages currently depending on libpointmatcher.

@pomerlef , I guess the big question is: How important is it still for libpointmatcher (master) to support ancient compilers?

pomerlef commented 7 years ago

libpointmatcher was fully reliant on C++11 at its first release. Then an industrial asked to run it on Windows (Visual Studio), which had a terrible/incomplete support for C++11 at that time (late 2014). It took me almost two months to replace all C++11 functionalities with boost calls.

From what I understood, you're doing the inverse move with libnabo (i.e., replacing boost calls with C++11)? What is the status of libnabo on Mac/Windows/other than Ubuntu?

I've no idea what is the status on Windows right now. For sure, all this back and forth is not optimal... Also, it doesn't make sense to have one on C++11 and not the other one as libpointmatcher wouldn't install on an old platform anyway.

I guess the forward solution is better, but I'm sure there will be other instabilities to handle...

HannesSommer commented 7 years ago

Yes, libnabo seems to have started its journey from boost to c++11. Good. What about option 5. then? It can also be seen as the friendly version of the forward option and there is almost no difference except that the

#include "nabo/nabo.h"

must be be moved away from PointMatcher.h e.g. to MatchersImpl.h. This makes it more friendly (and a bit faster to compile against) because it doesn't require the depending package to go through libnabo headers (and therefore forces it to require c++11, too)

I've just did this move here: https://github.com/ethz-asl/libpointmatcher/commit/17e47e2a47b64d1fcf8c28d8aa10b0430c5f3d17.

This branch, https://github.com/ethz-asl/libpointmatcher/compare/fix/moveNaboIncludeToWhereItIsNeeded, builds nicely with and without c++11 on the client package side. I've just run its unit tests compiled without and without c++11 (i.e. with c++98), against libpointmatcher compiled with c++11.

pomerlef commented 7 years ago

Alright, let's move forward with that and see what happen ;)

You can accept the pull.

stephanemagnenat commented 7 years ago

C++11 core features are supported starting from gcc 4.8.1, clang 3.3 and msvc 19 (see http://en.cppreference.com/w/cpp/compiler_support). Moreover, msys2 distribution on Windows provides gcc 6.3 so no problem there.

I think that today the gain provided by using C++11 is really worth the simple work of upgrading the compiler infrastructure. Actually, personally I would suggest to switch to C++14, which is supported by gcc 5.0, clang 3.4 and msvc 19.1.

HannesSommer commented 7 years ago

I would wait with c++14, event though it is always nice to have ;). Currently gcc 4.8.2 / 4.8.4 seems to be current default for Ubuntu trusty (14.04), a target that is probably still widely used and important for libpointmatcher. Fortunately libnabo compiles and runs with 4.8.2 and so does libpointmatcher with the latest nabo. So, there is no need to give up support for trusty if we go for c++11 only (at least concerning compile time).

But I'm a bit afraid we could cause load / run-time issues on trusty with c++11 - c++03/98 mixed executables especially as libpointmatcher uses boost heavily, which should be compiled c++03/98 on trusty and is well known to cause trouble here. Hopefully only if users use rather old versions of boost. In such cases we can still recommend to not use master but some (older) release for libpointmatcher and libnabo.

HannesSommer commented 7 years ago

Okay, this done with #204 .