View Issue Details Jump to Notes ] Print ]
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
TagsNo tags attached.
Attached Files

 Relationships

  Notes
(0035645)
Daniele E. Domenichelli (developer)
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 (manager)
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 (developer)
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 (manager)
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 (developer)
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 (manager)
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 (developer)
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 (manager)
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 (developer)
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 (manager)
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 (developer)
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 (manager)
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 (developer)
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 (manager)
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 (developer)
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 (manager)
2014-05-16 08:44

Re 0014864:0035888: Thanks. Please merge to 'next' for testing.
(0035892)
Daniele E. Domenichelli (developer)
2014-05-16 12:20

Done. Thanks!
(0037095)
Daniele E. Domenichelli (developer)
2014-10-29 04:32

Fixed in http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=d01320d4 [^]
(0038432)
Robert Maynard (manager)
2015-04-06 09:07

Closing resolved issues that have not been updated in more than 4 months.

 Issue History
Date Modified Username Field Change
2014-04-07 05:15 Daniele E. Domenichelli New Issue
2014-04-07 05:32 Daniele E. Domenichelli Note Added: 0035645
2014-04-07 08:47 Brad King Note Added: 0035646
2014-04-07 10:06 Daniele E. Domenichelli Note Added: 0035647
2014-04-07 10:17 Brad King Note Added: 0035648
2014-04-07 10:26 Daniele E. Domenichelli Note Added: 0035649
2014-04-07 10:29 Daniele E. Domenichelli Assigned To => Daniele E. Domenichelli
2014-04-07 10:29 Daniele E. Domenichelli Status new => assigned
2014-04-07 10:31 Brad King Note Added: 0035650
2014-04-07 10:33 Daniele E. Domenichelli Note Added: 0035651
2014-04-07 10:37 Brad King Note Added: 0035652
2014-05-07 11:17 Daniele E. Domenichelli Note Added: 0035840
2014-05-07 11:34 Brad King Note Added: 0035841
2014-05-13 05:10 Daniele E. Domenichelli Note Added: 0035865
2014-05-13 08:39 Brad King Note Added: 0035868
2014-05-13 08:49 Daniele E. Domenichelli Note Added: 0035869
2014-05-13 10:29 Brad King Note Added: 0035870
2014-05-16 06:18 Daniele E. Domenichelli Note Added: 0035888
2014-05-16 08:44 Brad King Note Added: 0035889
2014-05-16 12:20 Daniele E. Domenichelli Note Added: 0035892
2014-10-29 04:32 Daniele E. Domenichelli Note Added: 0037095
2014-10-29 04:32 Daniele E. Domenichelli Status assigned => resolved
2014-10-29 04:32 Daniele E. Domenichelli Fixed in Version => CMake 3.1
2014-10-29 04:32 Daniele E. Domenichelli Resolution open => fixed
2015-04-06 09:07 Robert Maynard Note Added: 0038432
2015-04-06 09:07 Robert Maynard Status resolved => closed


Copyright © 2000 - 2018 MantisBT Team