MantisBT - CMake
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0014864CMakeModulespublic2014-04-07 05:152015-04-06 09:07
ReporterDaniele E. Domenichelli 
Assigned ToDaniele E. Domenichelli 
PrioritynormalSeverityminorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product VersionCMake 2.8.12.1 
Target VersionFixed in VersionCMake 3.1 
Summary0014864: CheckTypeSize changes CMAKE_MINIMUM_REQUIRED_VERSION
DescriptionIncluding CheckTypeSize will change CMAKE_MINIMUM_REQUIRED_VERSION to 2.6
Steps To ReproduceThis simple example:

  cmake_minimum_required(VERSION 2.8.12)
  message("${CMAKE_CURRENT_LIST_LINE} - ${CMAKE_MINIMUM_REQUIRED_VERSION}")
  include(CheckTypeSize)
  message("${CMAKE_CURRENT_LIST_LINE} - ${CMAKE_MINIMUM_REQUIRED_VERSION}")

will produce the following output:

  2 - 2.8.12
  4 - 2.6
Additional Information
TagsNo tags attached.
Relationships
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2014-04-07 05:15Daniele E. DomenichelliNew Issue
2014-04-07 05:32Daniele E. DomenichelliNote Added: 0035645
2014-04-07 08:47Brad KingNote Added: 0035646
2014-04-07 10:06Daniele E. DomenichelliNote Added: 0035647
2014-04-07 10:17Brad KingNote Added: 0035648
2014-04-07 10:26Daniele E. DomenichelliNote Added: 0035649
2014-04-07 10:29Daniele E. DomenichelliAssigned To => Daniele E. Domenichelli
2014-04-07 10:29Daniele E. DomenichelliStatusnew => assigned
2014-04-07 10:31Brad KingNote Added: 0035650
2014-04-07 10:33Daniele E. DomenichelliNote Added: 0035651
2014-04-07 10:37Brad KingNote Added: 0035652
2014-05-07 11:17Daniele E. DomenichelliNote Added: 0035840
2014-05-07 11:34Brad KingNote Added: 0035841
2014-05-13 05:10Daniele E. DomenichelliNote Added: 0035865
2014-05-13 08:39Brad KingNote Added: 0035868
2014-05-13 08:49Daniele E. DomenichelliNote Added: 0035869
2014-05-13 10:29Brad KingNote Added: 0035870
2014-05-16 06:18Daniele E. DomenichelliNote Added: 0035888
2014-05-16 08:44Brad KingNote Added: 0035889
2014-05-16 12:20Daniele E. DomenichelliNote Added: 0035892
2014-10-29 04:32Daniele E. DomenichelliNote Added: 0037095
2014-10-29 04:32Daniele E. DomenichelliStatusassigned => resolved
2014-10-29 04:32Daniele E. DomenichelliFixed in Version => CMake 3.1
2014-10-29 04:32Daniele E. DomenichelliResolutionopen => fixed
2015-04-06 09:07Robert MaynardNote Added: 0038432
2015-04-06 09:07Robert MaynardStatusresolved => closed

Notes
(0035645)
Daniele E. Domenichelli   
2014-04-07 05:32   
include() creates a new policy scope, but not a new scope, therefore even though CheckTypeSize calls:

  cmake_policy(PUSH)
  cmake_minimum_required(VERSION 2.6 FATAL_ERROR)

  [...]

  cmake_policy(POP)

When the flow returns to the calling file, the policies are restored, but the variables are not, and therefore CMAKE_MINIMUM_REQUIRED_VERSION is not restored.

Perhaps cmake_policy(PUSH/POP) should store CMAKE_MINIMUM_REQUIRED_VERSION together with the version.

Also there are another bunch of modules calling cmake_minimum_required, these should probably be fixed as well.
(0035646)
Brad King   
2014-04-07 08:47   
Re 0014864:0035645: The modules should use cmake_policy(VERSION) to set the policy version instead of cmake_minimum_required. I think all of them that do this now can be updated to set the policy version to 3.0.
(0035647)
Daniele E. Domenichelli   
2014-04-07 10:06   
Re 0014864:0035646: Some modules use it to check that if some feature is available: for example FindCUDA.cmake says:


   # We need to have at least this version to support the VERSION_LESS argument to 'if' (2.6.2) and unset (2.6.3)
  cmake_policy(PUSH)
  cmake_minimum_required(VERSION 2.6.3)
  cmake_policy(POP)

How should we handle these? Perhaps it could be replaced with something like this:

  if("${CMAKE_MINIMUM_REQUIRED_VERSION}" VERSION_LESS "2.6.3")
    cmake_policy(PUSH)
    cmake_minimum_required(VERSION 2.6.3)
    cmake_policy(POP)
  endif()

so that cmake_minimum_required is called only if we are requiring a smaller version. But perhaps this check can just be removed...

Anyway it would be nice to be able to call cmake_minimum_required() in modules always get the higher version, i.e.

  cmake_minimum_required(VERSION 2.6)
    # CMAKE_MINIMUM_REQUIRED_VERSION = 2.6
  cmake_minimum_required(VERSION 2.8)
    # CMAKE_MINIMUM_REQUIRED_VERSION = 2.8
  cmake_minimum_required(VERSION 2.6)
    # CMAKE_MINIMUM_REQUIRED_VERSION = 2.8 (unchanged)
    # cmake_policies = 2.6

So that you can use cmake_minimum_required in modules to ensure that the features used in your module are available.

Perhaps also something like

  cmake_minimum_required(VERSION <version> NO_POLICY)

that does the same as:

  cmake_policy(PUSH)
  cmake_minimum_required(VERSION <version>)
  cmake_policy(POP)
(0035648)
Brad King   
2014-04-07 10:17   
Re 0014864:0035647: Modules that come with CMake should never call cmake_minimum_required for any reason AFAIK. We know the version of CMake is high enough because the module comes with CMake. Some authors call cmake_minimum_required to try to accommodate users that copy the modules out into their own projects. This issue demonstrates why that should not be done.
(0035649)
Daniele E. Domenichelli   
2014-04-07 10:26   
Actually I just realized that the find modules should not be problem because they are included with find_package that (if I understand it correctly) create a nested variable scope.
(0035650)
Brad King   
2014-04-07 10:31   
Re 0014864:0035649: No, find_package does not create a nested variable scope for find modules. That is only done for the package version files.
(0035651)
Daniele E. Domenichelli   
2014-04-07 10:33   
Re 0014864:0035648: So, is it ok if I change all the occurrences where it is used for the policies to:

  cmake_policy(PUSH)
  cmake_policy(VERSION 3.0) # Or perhaps ${CMAKE_VERSION}
  [...]
  cmake_policy(POP)

And remove all the occurrences where it is used for checking some feature?
(0035652)
Brad King   
2014-04-07 10:37   
Re 0014864:0035651: Yes. Use "cmake_policy(VERSION 3.0)" for now and I will review it in 'next' to see if it should be something else.

Thanks for working on this.
(0035840)
Daniele E. Domenichelli   
2014-05-07 11:17   
Sorry for the delay.
I just pushed a topic bug_0014864

http://cmake.org/gitweb?p=stage/cmake.git;a=commitdiff;h=a125514d [^]

Can you please review it?

I fixed all the occurrences of cmake_minimum_required that found in Modules, but I don't know if I fixed them in the correct way.

The only occurrence left is in Modules/CMakeFindPackageMode.cmake

  # require the current version. If we don't do this, Platforms/CYGWIN.cmake complains because
  # it doesn't know whether it should set WIN32 or not:
  cmake_minimum_required(VERSION ${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION}.${CMAKE_PATCH_VERSION} )

I'm not sure if this module if it is used only by cmake --find-package or if it can be included in other files.
(0035841)
Brad King   
2014-05-07 11:34   
Re 0014864:0035840: CMakeFindPackageMode is not for inclusion in projects.

This issue is about CMake module code that could be included while processing other projects. In general this is only Check and Find modules. We should not need to change other modules.

The appearances in CMakeLists.txt files need to stay. They could be updated to use

 cmake_minimum_required(VERSION ${CMAKE_VERSION})

because they come with CMake and should always work correctly with the current version. The SystemInformation.cmake should not be changed either.
(0035865)
Daniele E. Domenichelli   
2014-05-13 05:10   
Updated and split in 3 separate commits as they are different cases:

http://cmake.org/gitweb?p=stage/cmake.git;a=commitdiff;h=e6af7fa9 [^]
http://cmake.org/gitweb?p=stage/cmake.git;a=commitdiff;h=81d0703d [^]
http://cmake.org/gitweb?p=stage/cmake.git;a=commitdiff;h=2b0baa65 [^]

I'm not sure about the generated CMakeLists.txt, should they have ${CMAKE_VERSION} as well or ${CMAKE_MINUMUM_REQUIRED_VERSION} (i.e use the same as current project)?
(0035868)
Brad King   
2014-05-13 08:39   
Re 0014864:0035865: For generated CMakeLists.txt files the content of them is still something that comes with CMake, so it should be kept up to date. The generated files are not to be directly included by a project either. I think ${CMAKE_VERSION} is correct in those too.
(0035869)
Daniele E. Domenichelli   
2014-05-13 08:49   
Ok, updated and squashed the last 2 commits:

http://cmake.org/gitweb?p=stage/cmake.git;a=commitdiff;h=e6af7fa9 [^]
http://cmake.org/gitweb?p=stage/cmake.git;a=commitdiff;h=6beb2acf [^]
(0035870)
Brad King   
2014-05-13 10:29   
Re 0014864:0035869: Thanks. Please revise the commit messages to explain the purpose of each change. A reader should not have to follow the issue number to this issue tracker entry to understand the commit. Please use a more descriptive topic name for the same reason.
(0035888)
Daniele E. Domenichelli   
2014-05-16 06:18   
Updated. The topic name is now "preserve_cmake_minimum_required_version"

Do not change minimum required version in modules
http://cmake.org/gitweb?p=stage/cmake.git;a=commitdiff;h=27dd68a2 [^]

Keep cmake_minimum_required calls in sync with current version
http://cmake.org/gitweb?p=stage/cmake.git;a=commitdiff;h=548ba5a0 [^]
(0035889)
Brad King   
2014-05-16 08:44   
Re 0014864:0035888: Thanks. Please merge to 'next' for testing.
(0035892)
Daniele E. Domenichelli   
2014-05-16 12:20   
Done. Thanks!
(0037095)
Daniele E. Domenichelli   
2014-10-29 04:32   
Fixed in http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=d01320d4 [^]
(0038432)
Robert Maynard   
2015-04-06 09:07   
Closing resolved issues that have not been updated in more than 4 months.