require CMake >= 3.10, fix #338 #620

nim65s · 2023-09-16T14:40:22Z

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

Other than the small requested change it looks good to me. Thanks @nim65s for taking this on :)

base.cmake

gergondet · 2023-09-19T08:44:03Z

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 · 2023-09-19T08:49:57Z

true :/

nim65s · 2023-09-19T08:50:52Z

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

nim65s · 2023-09-19T08:53:32Z

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 · 2023-09-19T08:55:21Z

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 · 2023-09-19T09:00:37Z

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 · 2023-09-19T09:06:02Z

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 · 2023-09-19T09:08:53Z

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 · 2023-09-19T09:12:21Z

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 · 2023-09-19T13:56:13Z

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

fix ffd8d44#diff-61fcbab488c8e9c131b5c70aab7aae76ddf3743bf5a26a72dd0130ac6ea93c94L632 introduced in #620

nim65s added 5 commits September 16, 2023 16:39

advertise CMake >= support, fix #338

dc7efae

remove conditions about CMAKE_VERSION < 3.10

5f61443

cxx11: deprecate and use cxx-standard

5331803

CMake >= 3.10 → all policies <= 71 are NEW by default

ffd8d44

enforce CMake >= 3.10

abf5159

nim65s changed the title ~~advertise CMake >= 3.10 support, fix #338~~ require CMake >= 3.10, fix #338 Sep 16, 2023

gergondet approved these changes Sep 19, 2023

View reviewed changes

base.cmake Outdated Show resolved Hide resolved

merge variable definitions

b005b0a

nim65s merged commit 9924c7e into master Sep 19, 2023
2 checks passed

nim65s deleted the cmake-3.10 branch September 19, 2023 07:42

nim65s mentioned this pull request Sep 19, 2023

Apply CMake >= 3.10 policies #621

Merged

nim65s added a commit that referenced this pull request Oct 30, 2023

findboost: fix logic

a0ee57d

fix ffd8d44#diff-61fcbab488c8e9c131b5c70aab7aae76ddf3743bf5a26a72dd0130ac6ea93c94L632 introduced in #620

nim65s mentioned this pull request Oct 30, 2023

findboost: fix logic #629

Merged

nim65s mentioned this pull request Aug 6, 2024

Add symmetries publisher and publish empty messages on no detections agimus-project/happypose_ros#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

require CMake >= 3.10, fix #338 #620

require CMake >= 3.10, fix #338 #620

nim65s commented Sep 16, 2023 •

edited

Loading

gergondet left a comment

gergondet commented Sep 19, 2023

nim65s commented Sep 19, 2023

nim65s commented Sep 19, 2023

nim65s commented Sep 19, 2023

gergondet commented Sep 19, 2023

nim65s commented Sep 19, 2023

nim65s commented Sep 19, 2023

nim65s commented Sep 19, 2023

gergondet commented Sep 19, 2023

nim65s commented Sep 19, 2023

require CMake >= 3.10, fix #338 #620

require CMake >= 3.10, fix #338 #620

Conversation

nim65s commented Sep 16, 2023 • edited Loading

gergondet left a comment

Choose a reason for hiding this comment

gergondet commented Sep 19, 2023

nim65s commented Sep 19, 2023

nim65s commented Sep 19, 2023

nim65s commented Sep 19, 2023

gergondet commented Sep 19, 2023

nim65s commented Sep 19, 2023

nim65s commented Sep 19, 2023

nim65s commented Sep 19, 2023

gergondet commented Sep 19, 2023

nim65s commented Sep 19, 2023

nim65s commented Sep 16, 2023 •

edited

Loading