search

mutationpp / Mutationpp

The MUlticomponent Thermodynamic And Transport library for IONized gases in C++
GNU Lesser General Public License v3.0
108 stars 58 forks source link

Remove vendored dependencies #125

Open rdbisme opened 4 years ago

rdbisme commented 4 years ago

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 commented 4 years ago

This is relevant: https://gitlab.kitware.com/cmake/cmake/-/issues/20511

jbscoggi commented 4 years ago

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 commented 4 years ago

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: https://github.com/catchorg/Catch2/issues/1079. And it does not need to get propagated since it's a private dependency, only needed when testing.

rdbisme commented 4 years ago

This is also related: https://stackoverflow.com/questions/30987311/exporting-an-imported-library