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 2021/06/23 02:57:13 UTC
[GitHub] [arrow] kou commented on a change in pull request #10571: ARROW-8459: [Dev][Archery] Use a more recent cmake-format
kou commented on a change in pull request #10571:
URL: https://github.com/apache/arrow/pull/10571#discussion_r656721780
##########
File path: dev/archery/archery/utils/lint.py
##########
@@ -90,20 +91,75 @@ def cpp_linter(src, build_dir, clang_format=True, cpplint=True,
class CMakeFormat(Command):
- def __init__(self, cmake_format_bin):
- self.bin = cmake_format_bin
+
+ # TODO(kszucs): check cmake_format version
+ def __init__(self, paths, cmake_format_bin=None):
+ self.check_version()
+ self.bin = default_bin(cmake_format_bin, "cmake-format")
+ self.paths = paths
+
+ @classmethod
+ def from_patterns(cls, base_path, include_patterns, exclude_patterns):
+ paths = {
+ str(path.as_posix())
+ for pattern in include_patterns
+ for path in base_path.glob(pattern)
+ }
+ for pattern in exclude_patterns:
+ pattern = (base_path / pattern).as_posix()
+ paths -= set(fnmatch.filter(paths, str(pattern)))
+ return cls(paths)
+
+ @staticmethod
+ def check_version():
+ try:
+ # cmake_format is part of the cmakelang package
+ import cmakelang
+ except ImportError:
+ raise ImportError(
+ "Please install archery using: `pip install dev/archery[lint]`"
+ )
+ # pin a specific version of cmake_format, must be updated in setup.py
+ if cmakelang.__version__ != "0.6.13":
+ raise LintValidationException(
+ "Wrong version of cmake_format is detected. "
+ "Please reinstall it using `pip install dev/archery[lint]`"
+ )
+
+ def check(self):
+ return self.run("-l", "error", "--check", *self.paths, check=False)
+
+ def fix(self):
+ return self.run("--in-place", *self.paths, check=False)
def cmake_linter(src, fix=False):
- """ Run cmake-format.py on all CMakeFiles.txt """
+ """
+ Run cmake-format on all CMakeFiles.txt
+ """
logger.info("Running cmake-format linters")
- if not fix:
- logger.warn("run-cmake-format modifies files, regardless of --fix")
+ cmake_format = CMakeFormat.from_patterns(
+ src.path,
+ include_patterns=[
+ 'ci/**/*.cmake',
+ 'cpp/CMakeLists.txt',
+ 'cpp/src/**/CMakeLists.txt',
+ 'cpp/cmake_modules/*.cmake',
+ 'go/**/CMakeLists.txt',
+ 'java/**/CMakeLists.txt',
+ 'matlab/**/CMakeLists.txt',
+ ],
+ exclude_patterns=[
+ 'cpp/cmake_modules/FindNumPy.cmake',
+ 'cpp/cmake_modules/FindPythonLibsNew.cmake',
+ 'cpp/cmake_modules/UseCython.cmake',
+ 'cpp/src/arrow/util/config.h.cmake',
+ ]
Review comment:
`cpp/cmake_modules/*.cmake` in the list are imported CMake files: https://github.com/apache/arrow/pull/9045#discussion_r553939866
We can't use cmake-format for `cpp/src/arrow/util/config.h.cmake` because it's not valid CMake file. It has `@...@` to replace variables by CMake.
##########
File path: cpp/cmake_modules/FindArrow.cmake
##########
@@ -399,33 +432,31 @@ endif()
set(ARROW_ABI_VERSION ${ARROW_SO_VERSION})
-mark_as_advanced(ARROW_ABI_VERSION
- ARROW_CONFIG_SUFFIXES
- ARROW_FULL_SO_VERSION
- ARROW_IMPORT_LIB
- ARROW_INCLUDE_DIR
- ARROW_LIBS
- ARROW_LIB_DIR
- ARROW_SEARCH_LIB_PATH_SUFFIXES
- ARROW_SHARED_IMP_LIB
- ARROW_SHARED_LIB
- ARROW_SO_VERSION
- ARROW_STATIC_LIB
- ARROW_VERSION
- ARROW_VERSION_MAJOR
- ARROW_VERSION_MINOR
- ARROW_VERSION_PATCH)
+mark_as_advanced(
+ ARROW_ABI_VERSION
+ ARROW_CONFIG_SUFFIXES
+ ARROW_FULL_SO_VERSION
+ ARROW_IMPORT_LIB
+ ARROW_INCLUDE_DIR
+ ARROW_LIBS
+ ARROW_LIB_DIR
+ ARROW_SEARCH_LIB_PATH_SUFFIXES
+ ARROW_SHARED_IMP_LIB
+ ARROW_SHARED_LIB
+ ARROW_SO_VERSION
+ ARROW_STATIC_LIB
+ ARROW_VERSION
+ ARROW_VERSION_MAJOR
+ ARROW_VERSION_MINOR
+ ARROW_VERSION_PATCH)
-find_package_handle_standard_args(Arrow REQUIRED_VARS
- # The first required variable is shown
- # in the found message. So this list is
- # not sorted alphabetically.
- ARROW_INCLUDE_DIR
- ARROW_LIB_DIR
- ARROW_FULL_SO_VERSION
- ARROW_SO_VERSION
- VERSION_VAR
- ARROW_VERSION)
+find_package_handle_standard_args(
+ Arrow
+ REQUIRED_VARS # The first required variable is shown
+ # in the found message. So this list is
+ # not sorted alphabetically.
+ ARROW_INCLUDE_DIR ARROW_LIB_DIR ARROW_FULL_SO_VERSION ARROW_SO_VERSION
Review comment:
```suggestion
Arrow
# The first required variable is shown in the found message. So this list is
# not sorted alphabetically.
REQUIRED_VARS ARROW_INCLUDE_DIR ARROW_LIB_DIR ARROW_FULL_SO_VERSION ARROW_SO_VERSION
```
##########
File path: dev/archery/archery/utils/lint.py
##########
@@ -90,20 +91,75 @@ def cpp_linter(src, build_dir, clang_format=True, cpplint=True,
class CMakeFormat(Command):
- def __init__(self, cmake_format_bin):
- self.bin = cmake_format_bin
+
+ # TODO(kszucs): check cmake_format version
+ def __init__(self, paths, cmake_format_bin=None):
+ self.check_version()
+ self.bin = default_bin(cmake_format_bin, "cmake-format")
+ self.paths = paths
+
+ @classmethod
+ def from_patterns(cls, base_path, include_patterns, exclude_patterns):
+ paths = {
+ str(path.as_posix())
+ for pattern in include_patterns
+ for path in base_path.glob(pattern)
+ }
+ for pattern in exclude_patterns:
+ pattern = (base_path / pattern).as_posix()
+ paths -= set(fnmatch.filter(paths, str(pattern)))
+ return cls(paths)
+
+ @staticmethod
+ def check_version():
+ try:
+ # cmake_format is part of the cmakelang package
+ import cmakelang
+ except ImportError:
+ raise ImportError(
+ "Please install archery using: `pip install dev/archery[lint]`"
+ )
+ # pin a specific version of cmake_format, must be updated in setup.py
+ if cmakelang.__version__ != "0.6.13":
+ raise LintValidationException(
+ "Wrong version of cmake_format is detected. "
+ "Please reinstall it using `pip install dev/archery[lint]`"
+ )
+
+ def check(self):
+ return self.run("-l", "error", "--check", *self.paths, check=False)
+
+ def fix(self):
+ return self.run("--in-place", *self.paths, check=False)
def cmake_linter(src, fix=False):
- """ Run cmake-format.py on all CMakeFiles.txt """
+ """
+ Run cmake-format on all CMakeFiles.txt
+ """
logger.info("Running cmake-format linters")
- if not fix:
- logger.warn("run-cmake-format modifies files, regardless of --fix")
+ cmake_format = CMakeFormat.from_patterns(
+ src.path,
+ include_patterns=[
+ 'ci/**/*.cmake',
+ 'cpp/CMakeLists.txt',
+ 'cpp/src/**/CMakeLists.txt',
+ 'cpp/cmake_modules/*.cmake',
+ 'go/**/CMakeLists.txt',
+ 'java/**/CMakeLists.txt',
+ 'matlab/**/CMakeLists.txt',
+ ],
Review comment:
We have only `python/CMakeLists.txt` in `python/`.
`python/cmake_modules/` is a symbolic link to `cpp/cmake_modules/`.
##########
File path: cpp/cmake_modules/DefineOptions.cmake
##########
@@ -104,33 +112,36 @@ if("${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}")
define_option(ARROW_USE_PRECOMPILED_HEADERS "Use precompiled headers when compiling"
OFF)
- define_option_string(ARROW_SIMD_LEVEL
- "Compile-time SIMD optimization level"
- "SSE4_2" # default to SSE4.2
- "NONE"
- "SSE4_2"
- "AVX2"
- "AVX512")
-
- define_option_string(ARROW_RUNTIME_SIMD_LEVEL
- "Max runtime SIMD optimization level"
- "MAX" # default to max supported by compiler
- "NONE"
- "SSE4_2"
- "AVX2"
- "AVX512"
- "MAX")
+ define_option_string(
Review comment:
Could you try `32` or something?
I don't expect formatter so much but I want to reduce diff to look history easily later.
##########
File path: cpp/cmake_modules/FindLLVMAlt.cmake
##########
@@ -57,23 +58,19 @@ if(LLVM_FOUND)
add_library(LLVM::LLVM_INTERFACE INTERFACE IMPORTED)
- set_target_properties(LLVM::LLVM_INTERFACE
- PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
- "${LLVM_INCLUDE_DIRS}"
- INTERFACE_COMPILE_FLAGS
- "${LLVM_DEFINITIONS}"
- INTERFACE_LINK_LIBRARIES
- "${LLVM_LIBS}")
+ set_target_properties(
+ LLVM::LLVM_INTERFACE
+ PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${LLVM_INCLUDE_DIRS}"
+ INTERFACE_COMPILE_FLAGS "${LLVM_DEFINITIONS}"
+ INTERFACE_LINK_LIBRARIES "${LLVM_LIBS}")
endif()
mark_as_advanced(CLANG_EXECUTABLE LLVM_LINK_EXECUTABLE)
-find_package_handle_standard_args(LLVMAlt
- REQUIRED_VARS # The first variable is used for display.
- LLVM_PACKAGE_VERSION
- CLANG_EXECUTABLE
- LLVM_FOUND
- LLVM_LINK_EXECUTABLE)
+find_package_handle_standard_args(
+ LLVMAlt
+ REQUIRED_VARS # The first variable is used for display.
+ LLVM_PACKAGE_VERSION CLANG_EXECUTABLE LLVM_FOUND LLVM_LINK_EXECUTABLE)
Review comment:
```suggestion
LLVMAlt
# The first variable is used for display.
REQUIRED_VARS LLVM_PACKAGE_VERSION CLANG_EXECUTABLE LLVM_FOUND LLVM_LINK_EXECUTABLE)
```
--
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