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

Remove vendored dependencies #125

Open
rdbisme opened this issue Apr 23, 2020 · 4 comments
Open

Remove vendored dependencies #125

rdbisme opened this issue Apr 23, 2020 · 4 comments

Comments

@rdbisme
Copy link
Collaborator

rdbisme commented Apr 23, 2020

After a private conversation with @ggange, I think that Mutation++ should not vendor the dependencies.

I explain why: you are someone that wants to couple mutation++ with your own solver code. Let'assume you are using cmake also for your solver.

  1. You compile mutation++ using the instructions provided in the documentation, but you don't have Eigen installed system-wide. Then the current situation will use the one vendored in third_party. So far so good.

  2. The problems start to appear when you then find_package(mutation++) in your code. Since Eigen is a PUBLIC dependency of mutation++, the find_dependency in the configure file of mutation++ will try to find Eigen without luck, and it will complain.

  3. The hacky solution then would be to add the third_party directory into CMAKE_MODULE_PREFIX and writing a FindEigen3.cmake (taking it maybe from this repository) or install Eigen system wide, with potential version mismatch problems.

IMHO what should happen is that whoever wants to build mutation++ should have Eigen installed separately, hopefully with your package manager. That means if you can build mutation++ you can also build whatever depends on it. Further advantages are that we do not have to ship the FindEigen3.cmake since modern installations of Eigen will ship the EigenConfig.cmake so it will be captured by cmake automatically and natively.

The need for Catch2 is less urgent, since basically nobody uses it apart from CI (and I think tests fails in new versions...)

What do you think?

@rdbisme
Copy link
Collaborator Author

rdbisme commented Apr 25, 2020

@jbscoggi
Copy link
Member

Hi @rubendibattista and @ggange,

Would it make more since just to use git submodules and the download/installation to our build config (if Eigen is not found)? I agree that there should be a better way to handle this then the way we are now.

As for Catch2, personally, I would prefer just to stick the single header (which has it's copyright info) in the tests directory and that's it. I think this is the simplest way forward IMO.

@rdbisme
Copy link
Collaborator Author

rdbisme commented Apr 26, 2020

Ehi @jbscoggi, that would not solve the problem with dependency propagation when coupling mutation++ within other software. It's basically what we have now, only fancier. :)

Instead of doing that, what about just, you know, asking users to apt-get install libeigen3-dev (or equivalent)?

I think I might be able to persist the installation information of eigen doing some cmake shenanigans, but... is it really worth it? Wouldn't it just be easier to ask people to install the dependencies? Eigen is basically shipped by all the package managers.

Catch2 it's a less pressing need since mutation++ just basically use it in CI, but the version currently vendored is way out of date. For example the Approx matcher has been updated in recenter versions to have a correct behavior: catchorg/Catch2#1079. And it does not need to get propagated since it's a private dependency, only needed when testing.

@rdbisme
Copy link
Collaborator Author

rdbisme commented Apr 26, 2020

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

No branches or pull requests

2 participants