GUDHI / gudhi-devel

The GUDHI library is a generic open source C++ library, with a Python interface, for Topological Data Analysis (TDA) and Higher Dimensional Geometry Understanding.
https://gudhi.inria.fr/
MIT License
245 stars 65 forks source link

Use Cgal and Eigen3 cmake target #1049

Closed VincentRouvreau closed 1 month ago

VincentRouvreau commented 2 months ago

This PR is required by future CGAL 6.0

questions :question:

* [ ] Do we update CGAL_VERSION_NR and EIGEN_VERSION_AT_LEAST check in code ?

mglisse commented 2 months ago

Remove project from each CMake (just keep the main one) as not mandatory and it does not add any functionality

Does it not even help if we have tests with the same name in 2 subprojects, or something like that? Anyway, whatever you prefer.

Do we update CGAL_VERSION_NR and EIGEN_VERSION_AT_LEAST check in code ?

I would say no. The code still works with older versions, it is only the cmake stuff that breaks.

find_package(CGAL 5.1.0) does not work with CGAL master (but 6.0.X). Do we open an issue on CGAL side ?

Yes please. I remember discussing this issue a while ago, but I don't know with whom... If they don't want to change it before the 6.0 release, we will have to handle that differently in gudhi.

VincentRouvreau commented 2 months ago

Does it not even help if we have tests with the same name in 2 subprojects, or something like that? Anyway, whatever you prefer.

From what I read here:

To start a project, we use the project() command to set the project name. This call is required with every project and should be called soon after cmake_minimum_required(). As we will see later, this command can also be used to specify other project level information such as the language or version number.

As all of our code managed by CMake is C++, with the same gudhi version number, cmake minimal version is the same for all the library (easier to maintain),... I prefer to keep one project.

VincentRouvreau commented 1 month ago

Sorry about that bad merge... I start again a new PR from 96f7508 and I close this one. Forget the tensorflow proposal, it will come on a new PR