CMake find_package with a minimal CGAL version number does not detect properly 6.X CGAL version

VincentRouvreau commented 1 month ago

Issue Details

CMake find_package(CGAL 5.1.0) (with a minimal cgal version number) does not seem to behave as expected with master version.

Source Code

Here is a CMake file to reproduce the behaviour:

cmake_minimum_required(VERSION 3.8)
project(minimal_cgal_version)

find_package(CGAL 5.1.0)

if (TARGET CGAL::CGAL)
  message("++ CGAL version: ${CGAL_VERSION}. Includes found in ${CGAL_INCLUDE_DIRS}")
endif ()

With CGAL 5.1.5, it works fine:

++ CGAL version: 5.1.5. Includes found in /home/gailuron/workspace/contrib/CGAL-5.1.5/include

(tested with 5.5.3 also).

But, with 6.0-I-207 (master from some days ago):

CMake Warning at src/cmake/modules/GUDHI_third_party_libraries.cmake:30 (find_package):
  Could not find a configuration file for package "CGAL" that is compatible
  with requested version "5.1.0".

  The following configuration files were considered but not accepted:

    /home/gailuron/workspace/contrib/CGAL-6.0-I-207/build/CGALConfig.cmake, version: 6.0

Call Stack (most recent call first):
  CMakeLists.txt:20 (include)

Environment

Operating system (Windows/Mac/Linux, 32/64 bits): Linux 64 bits
Compiler: g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
Release or debug mode: Release mode
Specific flags used (if any):
CGAL version: 5.1.5, 5.5.3 and 6.0-I-207 (master from some days ago)
Boost version: 1.84.0
Other libraries versions if used (Eigen, TBB, etc.): cmake 3.29.0 hcfe8598_0 conda-forge eigen 3.4.0 h00ab1b0_0 conda-forge gmp 6.3.0 h59595ed_1 conda-forge mpfr 4.2.1 h9458935_0 conda-forge

lrineau commented 1 month ago

That is an history of PR #4697 and, in particular commit 252b58d39fb3a997365b584b84a43bc75d7b655f, it seems that at that time we decided that, if the CMake user-code was asking for CGAL compatible with version 5.1, then a major version change like 6.0 would not be compatible. Do you agree it was a reasonable choice?

In recent CMake versions, find_package can specify a version range. I do not know if our file Installation/lib/cmake/CGAL/CGALConfigVersion.cmake is ready for that. I am pretty sure it is not tested, and thus I would guess it is broken.

I am open for a discussion, here. @VincentRouvreau, @afabri, @sloriot, others, please comment.

VincentRouvreau commented 1 month ago

Will the major changes for CGAL 6.0 invalidate the CMake code above? From a CGAL user point of view, if you keep PACKAGE_VERSION_COMPATIBLE=FALSE when major version is not the same, I do not see any CMake mechanism to make find_package(CGAL 5.1.0) work as I am expecting to. For instance, multiple find_package does not work:

find_package(CGAL 5.1.0...<6.0.0)
if (NOT TARGET CGAL::CGAL)
  find_package(CGAL 6.0.0)
endif()

But I can still do:

find_package(CGAL) # No expected version
if (NOT TARGET CGAL::CGAL)
  # Do some tests to check the version number
endif()

lrineau commented 1 month ago

Hi @VincentRouvreau @mglisse,

After a discussion with @sloriot, we decided that we will change that file CGALConfigVersion.cmake for master/CGAL-6.0. Whatever we decide for the policy in CGALConfigVersion.cmake, CGAL tries to be reasonably backward-compatible, with a few breaking changes for any new version (except for bug-fixes versions). Regardless of the policy we adopt, it will only approximate the actual situation. Therefore, we should avoid creating unnecessary obstacles for CGAL users.

VincentRouvreau commented 1 month ago

Hi @VincentRouvreau @mglisse,

After a discussion with @sloriot, we decided that we will change that file CGALConfigVersion.cmake for master/CGAL-6.0. Whatever we decide for the policy in CGALConfigVersion.cmake, CGAL tries to be reasonably backward-compatible, with a few breaking changes for any new version (except for bug-fixes versions). Regardless of the policy we adopt, it will only approximate the actual situation. Therefore, we should avoid creating unnecessary obstacles for CGAL users.

Thanks for the fix proposal !

lrineau commented 1 month ago

@VincentRouvreau A pull-request fixing that issue is being tested: https://github.com/CGAL/cgal/pull/8221.

CGAL / cgal