View Issue Details [ Jump to Notes ] | [ Print ] | ||||||||
ID | Project | Category | View Status | Date Submitted | Last Update | ||||
0014864 | CMake | Modules | public | 2014-04-07 05:15 | 2015-04-06 09:07 | ||||
Reporter | Daniele E. Domenichelli | ||||||||
Assigned To | Daniele E. Domenichelli | ||||||||
Priority | normal | Severity | minor | Reproducibility | always | ||||
Status | closed | Resolution | fixed | ||||||
Platform | OS | OS Version | |||||||
Product Version | CMake 2.8.12.1 | ||||||||
Target Version | Fixed in Version | CMake 3.1 | |||||||
Summary | 0014864: CheckTypeSize changes CMAKE_MINIMUM_REQUIRED_VERSION | ||||||||
Description | Including CheckTypeSize will change CMAKE_MINIMUM_REQUIRED_VERSION to 2.6 | ||||||||
Steps To Reproduce | This 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 | ||||||||
Tags | No tags attached. | ||||||||
Attached Files | |||||||||
Relationships | |
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. |
Notes |
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 |
Issue History |
Copyright © 2000 - 2018 MantisBT Team |