-
Notifications
You must be signed in to change notification settings - Fork 1.7k
updated minimum required cmake version to 3.16 #1514
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
updated minimum required cmake version to 3.16 #1514
Conversation
|
@gullradriel HackRF is over 10 years old, and there are folks who use it on some old systems, so we would rather not increase the required CMake version more than is necessary. We plan to merge #1016 which requires CMake 3.8. Would that be acceptable? |
Of course. As long as it's >= 3.5 it should remove the warning. I just made a choice because I had to make one for Mayhem and for that PR. I'm taking anything that is removing the warning ;-) Do you want me to edit the PR and reduce the version number to 3.8 or are you going to do it in another PR ? |
| #top dir cmake project for libhackrf + tools | ||
|
|
||
| cmake_minimum_required(VERSION 2.8.12) | ||
| cmake_minimum_required(VERSION 3.16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| cmake_minimum_required(VERSION 3.16) | |
| cmake_minimum_required(VERSION 2.8...4.0) |
If upstream wants to preserve compatibility this is how. Although if it's not tested with the lower end, there can be incompatible changes creeping in.
On the other hand for the users with very old CMake, wouldn't they also be using a very old version of this project? If they are building from source it is also trivial to get a modern cmake:
$ pip install cmake|
After further investigation I've chosen to bump our minimum CMake version to 3.10 rather than 3.16, because:
The bump was done in PR #1584, which also adds CI matrix testing across CMake versions to catch future issues. We may bump further if needed for some of the improvements in #1578, which I'm reviewing at the moment. I'll close this PR as it's redundant now, but thanks for chasing this issue. |
|
@martinling did you check my comment in #1514 (comment). The |
|
@LecrisUT I've seen that discussion and I'm aware of that option, but we'd need a lot more thorough testing to be sure if we want to enable all the new policies introduced between 3.10 and the current 4.x releases. Setting that kind of compatibility range would make CMake's behaviour differ more between versions, more so than we're really set up to test for effectively. The matrix testing isn't a guarantee that everything is fine. It only tests on three pretty recent systems/toolchains, it only does one kind of build on each, and it doesn't actually test the resulting binaries with hardware. Our best chance of not breaking things is to avoid changing behaviour unnecessarily. |
For the most parts the policies have a breaking failure at build-time. There are a few recent ones that are a bit trickier like the The tricky situation to test and where you should be careful is in the Config.cmake files since the user is in control of it unless you override it inside those files (which you should). Of course to be certain, you should check the policies documents https://cmake.org/cmake/help/latest/manual/cmake-policies.7.html. At the very least I do not find anything that would silently affect the binaries. But ultimately up to your discretion. As long as you keep up with the deprecation messages in a timely manner, that would be sufficient for downstream packaging. |
|
For me, what it comes down to is that if someone submitted a PR which was just: if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.11.0)
cmake_policy(SET CMP0072 NEW)
endif()
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.12.0)
cmake_policy(SET CMP0073 NEW)
cmake_policy(SET CMP0074 NEW)
cmake_policy(SET CMP0075 NEW)
endif()...and so on, for 125 policy changes across 23 CMake versions, without any justification for why we need those differences in behaviour, then we'd clearly reject that PR. But as I understand it, that's what setting |
Yes, that is correct, with the caveat that 1. you can still set them to I could also recommend having fallback switches if you want to experiment and non-commit to the overall policies, Qt has quite a complex setup that they do that for. But also if you are committed to looking into these, consider various modern CMake designs using |
Latest cmake versions are giving a warning on CMakeLists files that are using a required version under 3.5.
That PR is fixing the warning by requiring a minimum version of 3.16 anywhere it was checked. I choosed 3.16 as it's the one used in a lots of other open source projects, and it's not too recent.
As an example, we checked that Ubuntu latest stable is using at least version 3.22, and Ubuntu noble based build container is having version 3.28.3
'firmware' cmake warnings:
'host' cmake warnings: