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

Reduced required CMake version #498

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

themarpe
Copy link

@themarpe themarpe commented Dec 7, 2021

  • I've checked this Git style guide. [No]
  • I've checked this CMake style guide. [Yes]
  • My change will work with CMake 3.2 (minimum requirement for Hunter). [Yes]
  • I will try to keep this pull request as small as possible and will try not to mix unrelated features. [Yes]

Closes: #367

As stated in the issue, the change f43d5f3 bumped required CMake version to 3.12. This reverts it by using previous behavior for version less than 3.12 and new behavior otherwise.

(PR just combines the removed and added bits from the f43d5f3 commit)

Tested on our project (min 3.10.x), which was using an old Hunter version before the version bump

Question: should Hunter have its CI run with latest and minimum supported CMake version to keep this behavior going forward? There might be some projects that require higher versions of CMake and those could still be left as exceptions, but this change limited any use of Hunter, so might make sense just for the base Hunter to run the CI with minimum CMake version as well

@rbsheth
Copy link
Member

rbsheth commented Dec 7, 2021

Interesting thought. @craffael in #493 suggests bumping the CMake version in CI to the latest, and you suggest the opposite 😄

Not sure what is best...

@themarpe
Copy link
Author

themarpe commented Dec 7, 2021

@rbsheth I suggest both actually hah:)
Check our usecase: https://github.com/luxonis/depthai-core/actions/runs/1546463976 (under the build matrix)
We build 2 times - against the "minimum supported" version and latest.

I this applies in this case as well, as I otherwise don't see any good way of making sure that some changes don't bump the version and break (increase) minimum accepted CMake version.

@craffael
Copy link

What about making it possible to increase the CMake version used by CI for individual packages? Most packages are fine with an older version of cmake, but as @rbsheth mentioned, #493 requires at least 3.20.

Also I wonder how important it is that hunter supports cmake versions prior to 3.12. Personally I never had any problems switching to a newer version of cmake and the upgrade was always very smooth. But maybe that's not everyones experience?

@themarpe
Copy link
Author

What about making it possible to increase the CMake version used by CI for individual packages?

That's an option, requires more work though. For Hunter usage only (eg custom packages, etc...) the CI should atleast test the minimum documented version it supports

Also I wonder how important it is that hunter supports cmake versions prior to 3.12. Personally I never had any problems switching to a newer version of cmake and the upgrade was always very smooth. But maybe that's not everyones experience?

As mentioned in the issue, Ubuntu18.04 and its derivatives still top out at 3.10, which is still quite a portion of users. (depending on the field, eg ROS, etc...) (By top out I mean one cannot do apt install cmake to update it - has to manually install, which is a roadblock for most systems and they stay at eg 3.10)

Now if CMake was more like pip and one could do cmake install -U cmake to update it, then yeah, newest version all the way. This is IMO a tad unfortunate for CMake as it takes way to long to phase out older versions, so getting updates into mainline really trickles down to users with a lot of delay.

@rbsheth where would be the best way to add a CI for minimum verision CMake for Hunter only usecase?

Anyway, I think this PR is okay to merge in and I can do the CI PR afterwards?

@craffael
Copy link

As mentioned in the issue, Ubuntu18.04 and its derivatives still top out at 3.10, which is still quite a portion of users. (depending on the field, eg ROS, etc...) (By top out I mean one cannot do apt install cmake to update it - has to manually install, which is a roadblock for most systems and they stay at eg 3.10)

For ubuntu I was quite successful with the approach described here and which uses another apt repository: https://askubuntu.com/questions/355565/how-do-i-install-the-latest-version-of-cmake-from-the-command-line (top answer)

@themarpe
Copy link
Author

@craffael I agree, there are options to do it, is just not as straight forward as it should be such that CIs and existing OSes would be using the latest CMake - they are still on older versions.
Also the above top answer AFAIK creates alot of issues on a ROS system, as its whole build system is tied to CMake and removing it before installing the new one, removes all of the dependents as well AFAIK.

Preferably, CMake would employ a similar technique as pip, where installing a new version could be done easily and preferably in userspace. That said, I don't know the downsides of this approach and neither have a dug into it any more than necessary, so this is more of a personal opinion.

@rbsheth
Copy link
Member

rbsheth commented Dec 15, 2021

The CMake version is controlled by the polly CI setup script here: https://github.com/cpp-pm/polly/blob/master/bin/install-ci-dependencies.py#L202

It would be nice to have a minimum version of CMake supported (like 3.10) but to have some packages work and not others is going to be strange. Maybe we can just test 3.10 as an optional CI check on Ubuntu or something, so the data point is there.

@themarpe
Copy link
Author

but to have some packages work and not others is going to be strange.

I agree

Maybe we can just test 3.10 as an optional CI check on Ubuntu or something, so the data point is there.

Yes, instead of testing all packages, we just test some basic Hunter functionality, like adding some simple CMake library just to test that it goes through, using the minimum version.

Hah maybe we just grep for cmake_minimum_version and make sure nothing in core Hunter is set to higher than minimum version :D (with maybe added exception, for cases that are optional, etc...)

@themarpe
Copy link
Author

@rbsheth regardless the CI checking, I think this PR does the intended job of making sure core Hunter still works with lower CMake versions, up for merging it?

Copy link
Member

@rbsheth rbsheth left a comment

Choose a reason for hiding this comment

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

Yup

@rbsheth rbsheth merged commit 6171c32 into cpp-pm:master Dec 15, 2021
@rbsheth
Copy link
Member

rbsheth commented Dec 15, 2021

Released: https://github.com/cpp-pm/hunter/releases/tag/v0.23.319

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undocumented bump of minimum required CMake version to 3.12 in f43d5f3
3 participants