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 findGLEW and FindOpenMP as they are already present in CMake. Update minimum required cmake to 3.16.3 #6100

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

larshg
Copy link
Contributor

@larshg larshg commented Aug 9, 2024

No description provided.

@larshg larshg marked this pull request as ready for review August 10, 2024 10:48
@larshg larshg requested a review from mvieth August 10, 2024 10:48
@mvieth
Copy link
Member

mvieth commented Aug 11, 2024

Regarding GLEW, this would effectively revert commit b4cba32 , right?

Regarding OpenMP: Our FindOpenMP.cmake says that it can be removed when CMake 3.11 is required (you added FindOpenMP.cmake in commit 6d6718c ). 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

@larshg
Copy link
Contributor Author

larshg commented Aug 12, 2024

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 ?

@mvieth
Copy link
Member

mvieth commented Aug 14, 2024

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.

@larshg
Copy link
Contributor Author

larshg commented Aug 14, 2024

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.

@mvieth
Copy link
Member

mvieth commented Aug 17, 2024

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

@larshg larshg changed the title Remove findGLEW and FindOpenMP as they are already present in CMake. Remove findGLEW and FindOpenMP as they are already present in CMake. Update minimum required cmake to 3.16.3 Aug 21, 2024
@mvieth
Copy link
Member

mvieth commented Aug 23, 2024

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 mvieth mentioned this pull request Aug 25, 2024
@larshg
Copy link
Contributor Author

larshg commented Aug 26, 2024

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.

@mvieth
Copy link
Member

mvieth commented Sep 14, 2024

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.

@larshg
Copy link
Contributor Author

larshg commented Sep 16, 2024

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.

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! Looks good then.

@larshg larshg merged commit d231dde into PointCloudLibrary:master Sep 19, 2024
13 checks passed
@larshg larshg deleted the RemoveFindScripts branch September 19, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants