pcl icon indicating copy to clipboard operation
pcl copied to clipboard

Remove findGLEW and FindOpenMP as they are already present in CMake. Update minimum required cmake to 3.16.3

Open larshg opened this issue 1 year ago • 7 comments

larshg avatar Aug 09 '24 08:08 larshg

Regarding GLEW, this would effectively revert commit https://github.com/PointCloudLibrary/pcl/commit/b4cba32aee2bc4802ed9e35e26a65c185303b8f6 , right?

Regarding OpenMP: Our FindOpenMP.cmake says that it can be removed when CMake 3.11 is required (you added FindOpenMP.cmake in commit https://github.com/PointCloudLibrary/pcl/commit/6d6718c2ee7683fa7f091220e5d8e8ae35c8bcac ). The fix on the CMake side seems to be https://gitlab.kitware.com/cmake/cmake/-/commit/ffa6f8752b6928190c61ccb32f9c2776de422ad2 ? Then we should require CMake 3.11 in CMakeLists.txt and PCLConfig.cmake.in

mvieth avatar Aug 11 '24 09:08 mvieth

Should we move the cmake all the way to 3.16.3, as thats what our lowest CI uses, ie. Ubuntu 20.04 -> https://dev.azure.com/PointCloudLibrary/pcl/_build/results?buildId=24643&view=logs&j=f37cc87d-ec62-5c45-cc90-507356bb1dc7&t=fd8b7a93-6a1a-5896-63be-1e40cca607f5&l=8975 ?

larshg avatar Aug 12 '24 08:08 larshg

So every CMake version >= 3.11 and < 3.16 would be untested, but there is no immediate reason why they would not work, right? I think I would slightly prefer to set the requirement to 3.11. But if you think 3.16 is better, I am also fine with that.

mvieth avatar Aug 14 '24 14:08 mvieth

So every CMake version >= 3.11 and < 3.16 would be untested, but there is no immediate reason why they would not work, right? I think I would slightly prefer to set the requirement to 3.11. But if you think 3.16 is better, I am also fine with that.

Yes, I think we have made jumps in CMake versions before. And currently on CIs: Ubuntu 20.04: 3.16.3-1ubuntu1.20.04.1 Ubuntu 22.04: 3.22.1-1ubuntu1.22.04.2 Ubuntu 24.04: 3.30.2-0kitware1ubuntu24.04.1 Mac 12: 3.30.1 Mac 13: 3.30.2 Windows x86&x64: 3.30.2

So if we only require 3.11, but don't test that it actually works with that version is kind of unfortunate? I would actually argue that we should always raise the Cmake version to the lowest that we test with on CI's.

We could try to keep it compatible with 3.11, carefully checking each time that we don't use functionality that's not <3.16 compliant, but our CI's won't catch it.

larshg avatar Aug 14 '24 18:08 larshg

okay, let's raise the requirement to CMake 3.16 then.

mvieth avatar Aug 17 '24 12:08 mvieth

I think searching for GLEW in CONFIG mode is not the right way? At least on Ubuntu, there is neither a GLEWConfig.cmake nor a GLEW-config.cmake in any Ubuntu version. The CI jobs report that GLEW could not be found.

mvieth avatar Aug 23 '24 18:08 mvieth

I think searching for GLEW in CONFIG mode is not the right way? At least on Ubuntu, there is neither a GLEWConfig.cmake nor a GLEW-config.cmake in any Ubuntu version. The CI jobs report that GLEW could not be found.

Yeah, surely looks like that :sad:

I have reverted to use the non-config version.

larshg avatar Aug 26 '24 06:08 larshg

I am unsure about removing all the ${<NAME>_ROOT} and $ENV{<NAME>_ROOT} hints: how does CMake determine what <NAME> is supposed to be? I guess this is related to https://cmake.org/cmake/help/latest/policy/CMP0074.html ? Does this only work for find_package calls, or also for find_path and find_file?

By the way, could you force-push or close and reopen the PR, so that the CI checks run again? The logs from the last run have already been deleted by Azure.

mvieth avatar Sep 14 '24 13:09 mvieth

I am unsure about removing all the ${<NAME>_ROOT} and $ENV{<NAME>_ROOT} hints: how does CMake determine what <NAME> is supposed to be? I guess this is related to https://cmake.org/cmake/help/latest/policy/CMP0074.html ? Does this only work for find_package calls, or also for find_path and find_file?

By the way, could you force-push or close and reopen the PR, so that the CI checks run again? The logs from the last run have already been deleted by Azure.

I think the name is defered from the find_package(FLANN) - then its FLANN_ROOT etc. Also according to: Package roots are maintained as a stack so nested calls to all find_* commands inside find modules and config packages I assume that it also includes find_path and find_file. Where it still uses the NAME_ROOT as search paths.

larshg avatar Sep 16 '24 18:09 larshg