[GitHub] [arrow] kou commented on a change in pull request #7620: ARROW-9013: [C++] Validate CMake options

[GitBox <gi...@apache.org>]

kou commented on a change in pull request #7620:
URL: https://github.com/apache/arrow/pull/7620#discussion_r449264750



##########
File path: cpp/CMakeLists.txt
##########
@@ -18,6 +18,37 @@
 cmake_minimum_required(VERSION 3.2)
 message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
 
+cmake_policy(SET CMP0025 NEW)
+# IN_LIST if() operator
+cmake_policy(SET CMP0057 NEW)
+
+# Compatibility with CMake 3.1
+if(POLICY CMP0054)
+  # http://www.cmake.org/cmake/help/v3.1/policy/CMP0054.html

Review comment:
       ```suggestion
     # https://www.cmake.org/cmake/help/v3.1/policy/CMP0054.html
   ```

##########
File path: cpp/CMakeLists.txt
##########
@@ -18,6 +18,37 @@
 cmake_minimum_required(VERSION 3.2)
 message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
 
+cmake_policy(SET CMP0025 NEW)
+# IN_LIST if() operator
+cmake_policy(SET CMP0057 NEW)

Review comment:
       Could you add `if(POLICY CMP0057)` for CMake 3.2?
   https://cmake.org/cmake/help/latest/policy/CMP0057.html says "This policy was introduced in CMake version 3.3.".

##########
File path: cpp/CMakeLists.txt
##########
@@ -18,6 +18,37 @@
 cmake_minimum_required(VERSION 3.2)
 message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
 
+cmake_policy(SET CMP0025 NEW)
+# IN_LIST if() operator
+cmake_policy(SET CMP0057 NEW)
+
+# Compatibility with CMake 3.1
+if(POLICY CMP0054)
+  # http://www.cmake.org/cmake/help/v3.1/policy/CMP0054.html

Review comment:
       Could you also add the one-line summary in the page ("Only interpret if() arguments as variables or keywords when unquoted.") to here?
   It's difficult to know what is the policy for only with URL...

##########
File path: cpp/cmake_modules/DefineOptions.cmake
##########
@@ -390,6 +390,25 @@ that have not been built" OFF)
          ON)
 endif()
 
+macro(validate_config)
+  foreach(category ${ARROW_OPTION_CATEGORIES})
+    set(option_names ${ARROW_${category}_OPTION_NAMES})
+
+    foreach(name ${option_names})
+      set(possible_values ${${name}_OPTION_POSSIBLE_VALUES})
+      set(value "${${name}}")
+      if(possible_values)
+        if(NOT ("${value}" IN_LIST possible_values))

Review comment:
       Could you skip this check if `NOT CMAKE_VERSION VERSION_LESS "3.3"` because `IN_LIST` doesn't exist in CMake 3.2?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org