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

Bump cmake_minimum_required to 3.21 #2532

Merged
merged 4 commits into from Nov 14, 2021
Merged

Bump cmake_minimum_required to 3.21 #2532

merged 4 commits into from Nov 14, 2021

Conversation

ihnorton
Copy link
Member

TYPE: IMPROVEMENT
DESC: Bump cmake_minimum_required to 3.13

@ihnorton ihnorton marked this pull request as draft October 12, 2021 19:12
@ihnorton ihnorton marked this pull request as ready for review October 13, 2021 13:34
@ihnorton ihnorton changed the title WIP Bump cmake_minimum_required to 3.13 Bump cmake_minimum_required to 3.13 Oct 13, 2021
@Shelnutt2
Copy link
Member

@ihnorton centos/rhel 7 has cmake 3.12. Do we need 3.13 or would 3.12 be acceptable too?

Copy link
Member

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

This is fine. I tried to find a concise on-line representation of 'where cmake commonly is' and that is difficult so I didn't post anything. The closest may be the various tabs in the repology site entry and while no single one is fully answering it (given the widespread use of cmake and repology's insistence on covering largely irrelevant entries (AIX, anyone?)) the badges one may come closest.

So 3.13 should be fine.

@dhoke4tdb
Copy link
Contributor

dhoke4tdb commented Oct 13, 2021

Related to Seth's question, visual studio 2017 is version cmake version 3.12.18081601-MSVC_2, also a 3.12 derivative.
Are we leaving vs2017 behind now? (or forcing use of later cmake obtained external to VS installation?)

@eric-hughes-tiledb
Copy link
Contributor

forcing use of later cmake obtained external to [VS 2017, ... or whatever else]

It's not part of this PR to decide this, but I'd like to have some estimate about what kind of actual cost this would be of requiring even-more recent versions of CMake. A clearly-labeled requirement that the user "must use an updated version of CMake", combined with an explicit bail-out in our CMakeLists.txt (not just relying on the version command), might be acceptable as long as we're not too aggressive about pushing the required version up.

I'm constantly fighting with our build, and a good part of that is that we're unable to use more recent CMake features and so we've got a build that's stuck in the past in a bad way. For example, 3.20 has a full preset facility (begun in 3.19), which allows specification of a large number of configuration parameters that are otherwise passed in disparate ways. Being able to lock all these down would isolate our build from variation from environmental defaults and other sources. There's a feature in 3.18 that would directly address a problem I'm having this week with the superbuild:

The find_program(), find_library(), find_path() and find_file() commands gained a new REQUIRED option that will stop processing with an error message if nothing is found.
Also a small-looking but significant change would allow a lot of cleanup in the build:
Since CMake 3.19, an INTERFACE library target may optionally contain source files

There's a lot of this kind of thing happening lately. I'm all in favor of being as aggressive as possible about CMake versions. Unlike compilers, there's no ABI back-compatibility issues.

Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

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

Wholeheartedly approve.

@eddelbuettel
Copy link
Member

I had overlooked the CentOS 7 concern, my bad, Those systems are (sadly) quite common. Now, in order to build there (given our use of C++17) we likely require the devtoolset anyway. Can a newer cmake be made available for those users? Could we (worst case) even provide an rpm ourselves?

@ihnorton
Copy link
Member Author

PR to bump rtools40 cmake version: r-windows/rtools-packages#229

@ihnorton ihnorton changed the title Bump cmake_minimum_required to 3.13 Bump cmake_minimum_required to 3.21 Oct 19, 2021
@ihnorton ihnorton closed this Oct 19, 2021
@ihnorton ihnorton reopened this Oct 19, 2021
@ihnorton ihnorton requested review from Shelnutt2 and removed request for Shelnutt2 October 19, 2021 14:39
@ihnorton
Copy link
Member Author

Ok, this is g2g and passing with cmake 3.21, should we choose to make the jump to lightspeed.

@Shelnutt2
Copy link
Member

@ihnorton the manylinux image test is failing because it is on cmake 3.20. Can we upgrade to the latest manylinux, say 2021-11-07-28723f3 which has cmake 3.21.4?

@ihnorton
Copy link
Member Author

Sure, don't see why not.

@Shelnutt2 Shelnutt2 deleted the ihn/cmake-3.13 branch November 14, 2021 18:54
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.

None yet

5 participants