Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix eigen includes #7

Merged
merged 8 commits into from
Feb 3, 2018
Merged

Conversation

patrikhuber
Copy link
Contributor

Hi!
The correct include for Eigen is Eigen/Core, without the eigen3 :-)

Thank you for making this library available open source!

@patrikhuber
Copy link
Contributor Author

patrikhuber commented Jan 28, 2018

Hmm travis fails now with Eigen/Core: No such file or directory, but to be honest I don't really know where the Eigen include path is set. I've checked the CMake files and the travis file and couldn't find anything. But every project I've seen so far, including the official Eigen documentation, includes Eigen with #include "Eigen/Core" and not eigen3/Eigen/Core.

If you include the repo as a submodule, there's no `nlohmann/` directory. The file is in the directory `src/` (https://github.com/nlohmann/json/blob/develop/src/json.hpp), and should be included via `#include "json.hpp"`, see the nlohmann/json readme: https://github.com/nlohmann/json#integration
@patrikhuber
Copy link
Contributor Author

Regarding the json include, I've opened nlohmann/json#942 upstream, I'm curious about their comment.

@Dobiasd
Copy link
Owner

Dobiasd commented Jan 29, 2018

Nice, thanks a lot. 👍
I guess line 42 in .travis.yml needs to be changed from cd eigen to cd eigen-git-mirror. But that of course would only fix the problem introduced with the commit Change Eigen to official Eigen GitHub mirror, not the include-directory related one that already existed before.

@Dobiasd
Copy link
Owner

Dobiasd commented Jan 29, 2018

OK, now we are back to the include error. It seems like doing make install with eigen simply copies the files to ./local/include/eigen3/Eigen. So if one does not want to set ./local/include/eigen3 as an additional include directory for the compiler it is #include <eigen3/Eigen/Core> and not #include <eigen3/Eigen/Core>. Or am I missing something?

@patrikhuber
Copy link
Contributor Author

patrikhuber commented Jan 29, 2018

It looks like you're correct.
But I have never seen anyone or any project include Eigen with <eigen3/Eigen/Core>. And the official documentation also always includes it as Eigen/Core. So I can only conclude that it is expected that people add eigen3/ to their include path?

@Dobiasd
Copy link
Owner

Dobiasd commented Jan 29, 2018

Probably, yeah. At least the documentation states:

The directory in which you placed Eigen's source code must be in the include path.
...
On Linux or Mac OS X, another option is to symlink or copy the Eigen folder into /usr/local/include/.

@patrikhuber
Copy link
Contributor Author

Ah yes, good find! Slightly annoying though I guess that the headers don't just get copied to /usr/local/include/Eigen for a system-wide install ;-)

@Dobiasd
Copy link
Owner

Dobiasd commented Jan 29, 2018

What do you think about adding ln /usr/local/include/Eigen ./local/include/eigen3/Eigen to the install instructions and to travis.yml in order so have the includes in C++ as in the documentation?

@patrikhuber
Copy link
Contributor Author

Sorry for the late reply! I think for travis, it's an acceptable fix. But I wouldn't advise to people in the readme to symlink stuff in /usr/local/include.
An alternative I guess would be to use find_package(Eigen3)? Something along the lines how I did it? But that requires a bit more modifications so maybe there's an easier solution for your repo.

Regarding nlohmann/json, it actually seems like there were some people discussing this issue in parallel and they've just merged a fix with proper include directories to the develop branch, and if I read correctly it'll be released tomorrow I think.

@Dobiasd
Copy link
Owner

Dobiasd commented Feb 2, 2018

Well, the symlink thing is what the Eigen documentation suggests. ;-)
Currently I would prefer it over the submodule solution, even though I too find it a bit weird.

The json fix seems to be merged into master, right?

So how do you think we should continue with this PR here?
Do you want to add the symlink stuff to it or go back to #include <eigen3/Eigen/... or try out the find_package solution?

@patrikhuber
Copy link
Contributor Author

The json fix seems to be merged into master, right?

Yep they released 3.1.0 today :-)
I was just going to deal with this PR tonight or tomorrow morning actually ;-)

Agree I wouldn't go for the Eigen submodule. What I meant was actually not that, but just using find_package from Eigen. We would have to copy the FindEigen.cmake from the Eigen/cmake repository to this repository, and then, the CMakeLists can find Eigen properly whether it's installed in the system, or e.g. via a EIGEN3_INCLUDE_DIR hint.
But... let's try the symlink for now, maybe while trying it, I have a good idea whether the find_package can work easily too.

@patrikhuber
Copy link
Contributor Author

Ok - so, the CMakeLists.txt of fdeep is quite rudimentary. What should sort-of ideally be done is to add a CMake target for the library, and then use find_package(...) and specify all the dependencies for that target in CMake (e.g. Eigen, fplus, nlohmann/json). Then, you could also use target_compile_options instead of add_compile_options and target_include_directories etc. And then this CMake target can be exported to be consumed by others. This is commonly done, also for header-only libraries (CMake supports that since 3.x or something). For my eos library linked above, I've done all that, except for exporting the target (this is on my todo list... I need to read what's the best practice to do that...).
And then basically users have two choices: For the people who do not want to have anything to do with CMake, they can just add all the include paths that fdeep needs to their include paths, and be happy. And for people using CMake already, they can import the fdeep target and just do target_link_libraries(my-app fdeep::fdeep) and be even happier.

But for the sake of this PR, let's fix it with the symlink and update nlohmann/json. I'll push shortly.

This should allow the Eigen headers to be found with e.g. `#include "Eigen/Core"`.
@patrikhuber
Copy link
Contributor Author

I think we're ready to merge here.
The nlohmann/json problem fixed itself upstream so no changes needed anymore in fdeep.

@Dobiasd Dobiasd merged commit 105aa72 into Dobiasd:master Feb 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants