jrl-umi3218 / jrl-cmakemodules

CMake utility toolbox
https://jrl-cmakemodules.readthedocs.io/en/master/
Other
56 stars 46 forks source link

require CMake >= 3.10, fix #338 #620

Closed nim65s closed 10 months ago

nim65s commented 10 months ago

Hi,

CMake 3.27 now show the following warning:

CMake Deprecation Warning at CMakeLists.txt:27 (cmake_minimum_required):
Compatibility with CMake < 3.5 will be removed from a future version of
CMake.

Update the VERSION argument <min> value or use a ...<max> suffix to tell
CMake that the project does not need compatibility with older versions.

So I guess this is time to enforce #338, where 3.10 was the latest consensual version in the discussion.

gergondet commented 10 months ago

Sorry about the comment after the fact but this came to me after reviewing the PR, I think the changes around policy manipulation must be reverted.

While we check that CMAKE_VERSION is greater than 3.10 we don't check that cmake_minimum_required was called with at least 3.10 (in fact I don't think there's a simple way to check for this?).

This means that we can have a project with cmake_minimum_required(VERSION 3.1) that is configured with CMake >= 3.10 and they will break if the submodule is updated because some policies will be unset (e.g. CMP0057 introduced in 3.3)

nim65s commented 10 months ago

true :/

nim65s commented 10 months ago

I'll try to just add cmake_minimum_required(VERSION 3.10) in base, to see how this behave

nim65s commented 10 months ago

So in eigenpy, with latest jrl-cmakemodule master, + cmake_minimum_required(VERSION 3.1) is failing:


CMake Error at cmake/base.cmake:265 (if):
  if given arguments:

    "NOT" "eigen3 >= 3.0.5" "IN_LIST" "_PKG_CONFIG_REQUIRES"

  Unknown arguments specified
Call Stack (most recent call first):
  cmake/package-config.cmake:82 (_add_to_list_if_not_present)
  CMakeLists.txt:102 (add_project_dependency)

Adding cmake_minimum_required(VERSION 3.10) in base.cmake fix this issue. I'll open another PR for this.

gergondet commented 10 months ago

What happens the other way around? e.g. cmake_minimum_required(VERSION 3.20) in the main project but cmake_minimum_required(VERSION 3.10) inside jrl-cmakemodules?

nim65s commented 10 months ago

In that case, eigenpy is configuring / building fine, but I have no idea about how policies between 3.10 and 3.20 are treated. The doc is not clear about what happen on multiple calls of this function https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html

https://cmake.org/cmake/help/latest/command/cmake_policy.html#command:cmake_policy hints that we can GET to check this, I'll try

nim65s commented 10 months ago
cmake_minimum_required(VERSION 3.1)

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.1: ${fifthy} ${seventy} ${ninety}")

cmake_minimum_required(VERSION 3.10)

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.10: ${fifthy} ${seventy} ${ninety}")

cmake_minimum_required(VERSION 3.20)

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.20: ${fifthy} ${seventy} ${ninety}")

cmake_minimum_required(VERSION 3.10)

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.10: ${fifthy} ${seventy} ${ninety}")

project(something)
-- after 3.1:   
-- after 3.10:  NEW 
-- after 3.20:  NEW NEW
-- after 3.10:  NEW

So #621 is a bad idea.

nim65s commented 10 months ago

A simple additional check should work:

cmake_minimum_required(VERSION 3.1)

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.1: ${fifthy} ${seventy} ${ninety}")

if(CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.10)
  cmake_minimum_required(VERSION 3.10)
endif()

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.10: ${fifthy} ${seventy} ${ninety}")

if(CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.20)
  cmake_minimum_required(VERSION 3.20)
endif()

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.20: ${fifthy} ${seventy} ${ninety}")

if(CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.10)
  cmake_minimum_required(VERSION 3.10)
endif()

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.10: ${fifthy} ${seventy} ${ninety}")

project(something)

-- after 3.1:
-- after 3.10: NEW -- after 3.20: NEW NEW -- after 3.10: NEW NEW

gergondet commented 10 months ago

I tried locally and I was getting a different result. It turns out that whether cmake_minimum_required is called in an included file or not changes the result of this. I am not sure how all this interacts with macros.

Another edge case that I can think of is if someone is explicitly relying on some OLD policy since jrl-cmakemodules was careful about not affecting the global project policy before (I'm not aware of any project that would be affected but that's something to consider)

I think the path that would lead to the least breakage would be to keep the policy updates that we were doing before.

Furthermore, it's easier to track the breakage if the maintainer upgrades the minimum required rather than if it breaks because jrl-cmakemodules are updated.

nim65s commented 10 months ago

cmake_minimum_required and cmake_policy are changing CMAKE_MINIMUM_REQUIRED_VERSION, so the scope of that call is important: in a macro it should change the top level, but in a function this is going to be only local.

On the scope of the included file… I'm not sure, but it might be better to check for CMAKE_MINIMUM_REQUIRED_VERSION and ask the user to bump it to 3.10.

For people explicitely relying on OLD policy, this is not an issue I think, as this looks to be handled correctly:

cmake_minimum_required(VERSION 3.1)

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.1: 50-${fifty} 70-${seventy} 90-${ninety}")

if(CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.10)
  cmake_minimum_required(VERSION 3.10)
endif()

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.10: 50-${fifty} 70-${seventy} 90-${ninety}")

if(CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.20)
  cmake_minimum_required(VERSION 3.20)
endif()

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.20: 50-${fifty} 70-${seventy} 90-${ninety}")

if(CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.10)
  cmake_minimum_required(VERSION 3.10)
endif()

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.10: 50-${fifty} 70-${seventy} 90-${ninety}")

cmake_policy(SET CMP0050 OLD)

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after OLD: 50-${fifty} 70-${seventy} 90-${ninety}")

if(CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.10)
  cmake_minimum_required(VERSION 3.10)
endif()

cmake_policy(GET CMP0050 fifty)
cmake_policy(GET CMP0070 seventy)
cmake_policy(GET CMP0090 ninety)

message(STATUS "after 3.10: 50-${fifty} 70-${seventy} 90-${ninety}")

project(something)
-- after 3.1: 50-NEW 70- 90-
-- after 3.10: 50-NEW 70-NEW 90-
-- after 3.20: 50-NEW 70-NEW 90-NEW
-- after 3.10: 50-NEW 70-NEW 90-NEW
CMake Deprecation Warning at CMakeLists.txt:43 (cmake_policy):
  The OLD behavior for policy CMP0050 will be removed from a future version
  of CMake.

  The cmake-policies(7) manual explains that the OLD behaviors of all
  policies are deprecated and that a policy should be set to OLD only under
  specific short-term circumstances.  Projects should be ported to the NEW
  behavior and not rely on setting a policy to OLD.

-- after OLD: 50-OLD 70-NEW 90-NEW
-- after 3.10: 50-OLD 70-NEW 90-NEW