You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/07/02 16:32:50 UTC
[GitHub] [arrow] pitrou opened a new pull request #7620: ARROW-9013: [C++] Validate CMake options
pitrou opened a new pull request #7620:
URL: https://github.com/apache/arrow/pull/7620
Disallow passing invalid values to enum-style CMake options
(such as `-DARROW_SIMD_LEVEL=xyzzy`)
----------------------------------------------------------------
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
[GitHub] [arrow] kou closed pull request #7620: ARROW-9013: [C++] Validate CMake options
Posted by GitBox <gi...@apache.org>.
kou closed pull request #7620:
URL: https://github.com/apache/arrow/pull/7620
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #7620: ARROW-9013: [C++] Validate CMake options
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7620:
URL: https://github.com/apache/arrow/pull/7620#issuecomment-653114885
https://issues.apache.org/jira/browse/ARROW-9013
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on pull request #7620: ARROW-9013: [C++] Validate CMake options
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7620:
URL: https://github.com/apache/arrow/pull/7620#issuecomment-654331202
The "AMD64 MacOS 10.15 Python 3.7" CI failure looks unrelated.
----------------------------------------------------------------
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
[GitHub] [arrow] kou commented on pull request #7620: ARROW-9013: [C++] Validate CMake options
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #7620:
URL: https://github.com/apache/arrow/pull/7620#issuecomment-654445290
Thanks!
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on pull request #7620: ARROW-9013: [C++] Validate CMake options
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7620:
URL: https://github.com/apache/arrow/pull/7620#issuecomment-654273536
Thanks for the review @kou . I believe I've addressed all of your comments.
----------------------------------------------------------------
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
[GitHub] [arrow] kou commented on a change in pull request #7620: ARROW-9013: [C++] Validate CMake options
Posted by 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