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

Update minimum cmake version (#4076) #4080

Closed
wants to merge 1 commit into from

Conversation

Nauroze
Copy link

@Nauroze Nauroze commented Jul 23, 2023

Resolves issue #4076
This commit updates the minimum cmake version 3.16, since 3.5 and below will be deprecated.

  • A side not to those new to cmake versioning, it goes as follows 3.1, 3.2... 3.5... 3.9, 3.10, 3.11 etc. Therefore keep in mind that 3.5 is actually lower than 3.16.

@coveralls
Copy link

coveralls commented Jul 23, 2023

Coverage Status

coverage: 100.0%. remained the same when pulling e68ffb6 on Nauroze:update-min-cmake-version into 5d27543 on nlohmann:develop.

CMakeLists.txt Outdated
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.1)
cmake_minimum_required(VERSION 3.16)
Copy link
Owner

Choose a reason for hiding this comment

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

Please set to the lowest still supported version which would be 3.5.

Copy link

Choose a reason for hiding this comment

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

Another option would be to update the minimum and set the maximum that guarantees that the CMake script still behaves as expected. So far it looks like the script works well with CMake 3.27 so setting:

cmake_minimum_required(VERSION 3.5 ... 3.27)

May guarantee that this script works until CMake 3.27 gets deprecated.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is necessary as this project does not rely on any fancy CMake feature.

Copy link

Choose a reason for hiding this comment

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

It would be better to use the range notation. The minimum version doesn't even need to change if you don't want. (i.e. 3.1...3.27 should work. As an aside: I'm not certain but I believe there shouldn't be spaces between the two versions since the syntax was added in a way to be backwards compatible with ancient versions of CMake from before the range feature as added.) The advantage to using the range notation is that you're not turning on unnecessary compatibility features and that cleans up and improves the ability to vendor the library with add_subdirectory.

Copy link
Author

Choose a reason for hiding this comment

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

I suppose I should follow the owner's (@nlohmann) request of using the minimum supported of 3.5 instead of a range. It seems the test scripts use cmake 3.13, so that too should resolve it. Let me know if 3.13 is preferred otherwise I will edit the pull request with 3.5 as requested.

Copy link
Owner

Choose a reason for hiding this comment

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

What happens when the range is given as 3.1...3.27 and I am running CMake 3.50?

Copy link

Choose a reason for hiding this comment

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

If you are running CMake 3.50 the script will work as long as CMake 3.27 is still supported (like CMake 3.5 is supported by CMake 3.27).

Basically the range notation means: "I require at least CMake 3.1 and I verified that still works with CMake 3.27. So if you are using CMake 3.50, please use the CMake 3.27 compatibility mode."

Copy link

Choose a reason for hiding this comment

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

Not using the range notation is the same as using 3.1...3.1 as the range notation.

Copy link

Choose a reason for hiding this comment

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

Yeah, to provide a little more technical detail: the range notation just means that cmake_policy introduced between CMake versions min and max are to be set to NEW instead of OLD. The reason for the new warning about < 3.5 support being dropped is not that Kitware cares that you're writing scripts compatible with < 3.5 but rather that they're removing the OLD policies and the script might break if it hasn't been validated to work with the policies set to NEW. I can't think of any reason for a project since the feature was introduced to not use the range notation.

Copy link
Author

Choose a reason for hiding this comment

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

I have revised the commit on this PR to the range notation. I have tested it with cmake version 3.5 and cmake version 3.27 installed on my system. No additional warnings seen for either one.

Here's a capture of the output:

3.27

/workspaces/json (update-min-cmake-version) $ cmake --version
cmake version 3.27.0

CMake suite maintained and supported by Kitware (kitware.com/cmake).
/workspaces/json (update-min-cmake-version) $ cd build/
/workspaces/json/build (update-min-cmake-version) $ cmake ..
-- The CXX compiler identification is GNU 9.4.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Using the multi-header code from /workspaces/json/include/
-- Found Git: /usr/local/bin/git (found version "2.41.0")
-- Operating system: Linux-5.15.0-1041-azure; Distributor ID: Ubuntu; Description: Ubuntu 20.04.6 LTS; Release: 20.04; Codename: focal; Linux codespaces-2fc995 5.15.0-1041-azure #48-Ubuntu SMP Tue Jun 20 20:34:08 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
-- Compiler: c++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0; Copyright (C) 2019 Free Software Foundation, Inc.; This is free software; see the source for copying conditions. There is NO; warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
-- Testing standards: 11 14 17 20 [23]
-- Looking for C++ include sys/types.h
-- Looking for C++ include sys/types.h - found
-- Looking for C++ include stdint.h
-- Looking for C++ include stdint.h - found
-- Looking for C++ include stddef.h
-- Looking for C++ include stddef.h - found
-- Check size of size_t
-- Check size of size_t - done
-- Configuring done (2.4s)
-- Generating done (0.2s)
-- Build files have been written to: /workspaces/json/build

3.5

/workspaces/json (update-min-cmake-version) $ sudo cmake --version
cmake version 3.5.0

CMake suite maintained and supported by Kitware (kitware.com/cmake).
/workspaces/json (update-min-cmake-version) $ cd build/
/workspaces/json/build (update-min-cmake-version) $ sudo cmake ..
-- The CXX compiler identification is GNU 9.4.0
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Using the multi-header code from /workspaces/json/include/
-- Configuring done
-- Generating done
-- Build files have been written to: /workspaces/json/build

@Nauroze Nauroze force-pushed the update-min-cmake-version branch 2 times, most recently from 5e3b4e0 to 1c29614 Compare July 24, 2023 23:21
@Nauroze Nauroze changed the title Update minimum cmake version to 3.16 (#4076) Update minimum cmake version (#4076) Jul 24, 2023
@jmigual
Copy link

jmigual commented Aug 5, 2023

So @nlohmann, I think this change is good enough and supports this CMake build script until CMake 3.27 is deprecated. Can this be merged?

@Nauroze Nauroze requested a review from nlohmann August 6, 2023 01:30
Copy link
Contributor

@past-due past-due left a comment

Choose a reason for hiding this comment

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

👍

@t-b
Copy link
Contributor

t-b commented Aug 9, 2023

I also think this PR is a pure improvement.

@craigscott-crascit
Copy link
Contributor

The changes in this PR are insufficient, there are still various CMakeLists.txt files in the tests area which need updating as well. The shift all the way up to 3.27 also looks a bit risky to me to take as a single jump all at once. It would take a deeper analysis of all the policies between 3.1 and 3.27 to be sure none of them adversely affect the project.

I just open #4112 before seeing this PR here. My PR only lifts the upper range to CMake 3.14, but I did go through the project's CMake logic and I believe the policy changes are fine between 3.1 and 3.14. I also updated the tests area. Therefore, I recommend closing this PR here in favour of #4112, which is a safer, more complete change.

@nlohmann
Copy link
Owner

@Nauroze What do you think of #4112?

@Nauroze
Copy link
Author

Nauroze commented Sep 24, 2023

@Nauroze What do you think of #4112?

@nlohmann Yeah I'm good with that, it's more thorough as it is changing the CMakeLists.txt in the test directories as well.

Something to keep in mind: Since it uses 3.14 we have to look at changing this again in a few years if any future deprecation warnings come out. 3.27 would be a change for the long run. But I do understand picking 3.14 was a conservative choice and keeping it safe.

@nlohmann
Copy link
Owner

Closed in favor of #4112.

@nlohmann nlohmann closed this Sep 25, 2023
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.

8 participants