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/22 15:06:22 UTC
[GitHub] [arrow] pitrou commented on a change in pull request #10571: ARROW-8459: [Dev][Archery] Use a more recent cmake-format
pitrou commented on a change in pull request #10571:
URL: https://github.com/apache/arrow/pull/10571#discussion_r656313383
##########
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 cmake files in `python` as well, no?
##########
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
Review comment:
I don't know if you can do it in this PR?
##########
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:
Do you know the reasons for these exclusions?
##########
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
Review comment:
Wait, it seems done already below. Perhaps just remove this TODO?
--
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