Svalorzen / AI-Toolbox

A C++ framework for MDPs and POMDPs with Python bindings
GNU General Public License v3.0
647 stars 98 forks source link

Added Eigen3 version 3.3 as a REQUIRED component in Cmake #17

Closed alecive closed 6 years ago

alecive commented 7 years ago

Right now, Eigen 3.3 is required, but cmake does not look for 3.3. I just changed the CMakeLists.txt in order to fix this.

alecive commented 7 years ago

Also, I saw that test/CMakeLists.txt does not require a specific Eigen3 version. Shall I add version 3.3 also in that file?

Svalorzen commented 7 years ago

This change, and also putting it in test makes sense. At this point though, it seems (as travis points out) that linking to Eigen 3.3 directly won't work, as the beta packages currently available on Ubuntu 16.04 do not really report being version 3.3. I guess this is because they are still in beta stage.

Thanks for pointing this out though, I'll keep this open so when a stable package of 3.3 really gets released I'll merge this. Feel free to add the change for test/CmakeLists.txt too.

alecive commented 7 years ago

@Svalorzen I added the dependency also in test/CMakeLists.txt. I agree that we should wait for Eigen3 3.3 on Ubuntu 16.04, but the problem is that this is supposed to fix also compilation errors.

My setup is the following: I have Ubuntu 14.04 (and I cannot upgrade to 16.04 for the time being), Eigen3 v3.2 installed from repositories in /usr/local, and Eigen3 v3.3 locally installed in my code folder. By explicitly requiring 3.2, my cmake picks the 3.2 version installed in my system, and then I get a compilation error (because obviously we should link to v3.3).

So the trade-off is between either waiting for the official Ubuntu release of 3.3, or having a repo that potentially does not compile on some machines.

Obviously, the choice is totally yours, but I just wanted to give you the full picture. I will just stick with my fork for the time being :smile:

Svalorzen commented 7 years ago

I understand your pain. Maybe we could require some intermediate version? The one on my own machine reports finding Eigen 3.2.92; the one installed on Travis is 3.2.92.

Maybe if the default one on Ubuntu 14.04 is lower than say, 3.2.90, we could just force the version to be higher than that. At the moment I don't have a 14.04 machine, if you want to check on your own and see if it can be done you're welcome to try!

alecive commented 7 years ago

For me is totally fine to stick with this configuration for the time being. Let's just hope that 3.3 will be on the Ubuntu repositories soon!

Svalorzen commented 6 years ago

Hey @alecive, not sure if you're still working with this.

In any case, I was just looking at it again, and I noticed that in theory I never require the EXACT version match, so any version which is at least 3.2 would suffice.

However, CMake does not by default try to sort or order packages, so the first it finds which satisfies the constraint is good, which in your case is a problem since it goes to the 3.2.

I've just found out the

set(CMAKE_FIND_PACKAGE_SORT_ORDER NATURAL)
set(CMAKE_FIND_PACKAGE_SORT_DIRECTION DEC)

commands, which should require CMake to first try out the best packages first (so in your case 3.3) without requiring moving the minimum version to 3.3.

Would it be possible for you to try this out? Just add those two lines at the beginning of the main CMakeLists.txt and see if it works.

alecive commented 6 years ago

I don't understand it completely. My main gripe is with this one:

any version which is at least 3.2 would suffice.

For me, 3.2 does not work!

Svalorzen commented 6 years ago

What I meant is, for CMake it does not matter whether 3.2.92 is chosen, or 3.3, since the code currently only requires at least 3.2. I cannot put 3.3 otherwise the default Eigen package for Ubuntu will not get accepted, but it is not directly preventing you from using the 3.3 version.

What those two lines do is that they force CMake to use the highest version available. So if on your system both the 3.2 and 3.3 are available, CMake will default to the 3.3, which is the right choice.

alecive commented 6 years ago

I cannot put 3.3 otherwise the default Eigen package for Ubuntu will not get accepted, but it is not directly preventing you from using the 3.3 version.

The main reason behind my PR is that this repo does not compile with the 3.2.0-8 version available in the 14.04 repos for Eigen. So, the default Eigen package for ubuntu should not get accepted, since it does not work!

If you are saying that 3.2.92 works, then put 3.2.92 as the minimum. But there should be a minimum version number that is bigger than 3.2, since 3.2.0-8 does not work.

Svalorzen commented 6 years ago

Alright, I guess that's simpler. If you want to modify this PR, let me know, otherwise I can do this myself.

alecive commented 6 years ago

I've been trying to compile the repo from scratch but now I can't since it requires cmake 3.9. Is it really needed?

alecive commented 6 years ago

Ok nevermind, I saw that you fixed the problem yourself.

Svalorzen commented 6 years ago

Ah, thanks anyway for the try =)

It is needed unfortunately, as I use the new features to enable link time optimization in the compilers without having to do it myself manually.