Skip to content

Commit

Permalink
ARROW-6835: [Archery][CMake] Restore ARROW_LINT_ONLY cmake option
Browse files Browse the repository at this point in the history
Closes #5616 from fsaintjacques/ARROW-6835-restore-arrow-lint-only and squashes the following commits:

55bcb89 <François Saint-Jacques> Don't fail hard if lint binary is missing
ffff4c0 <François Saint-Jacques> ARROW-6835:  Restore ARROW_LINT_ONLY cmake option

Authored-by: François Saint-Jacques <fsaintjacques@gmail.com>
Signed-off-by: Wes McKinney <wesm+git@apache.org>
  • Loading branch information
fsaintjacques authored and wesm committed Oct 11, 2019
1 parent d1848c8 commit d5ba83e
Show file tree
Hide file tree
Showing 9 changed files with 80 additions and 31 deletions.
11 changes: 8 additions & 3 deletions cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,14 @@ if(${CLANG_FORMAT_FOUND})
${ARROW_LINT_QUIET})
endif()

add_custom_target(lint_cpp_cli ${PYTHON_EXECUTABLE} ${BUILD_SUPPORT_DIR}/lint_cpp_cli.py
${CMAKE_CURRENT_SOURCE_DIR}/src)

if(ARROW_LINT_ONLY)
message("ARROW_LINT_ONLY was specified, this is only a partial build directory")
return()
endif()

#
# "make clang-tidy" and "make check-clang-tidy" targets
#
Expand Down Expand Up @@ -251,9 +259,6 @@ if(UNIX)
add_custom_target(iwyu ${BUILD_SUPPORT_DIR}/iwyu/iwyu.sh)
endif(UNIX)

add_custom_target(lint_cpp_cli ${PYTHON_EXECUTABLE} ${BUILD_SUPPORT_DIR}/lint_cpp_cli.py
${CMAKE_CURRENT_SOURCE_DIR}/src)

#
# Set up various options
#
Expand Down
12 changes: 11 additions & 1 deletion dev/archery/archery/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -196,26 +196,36 @@ def build(ctx, src, build_dir, force, targets, **kwargs):
callback=validate_arrow_sources,
help="Specify Arrow source directory")
@click.option("--with-clang-format", default=True, type=BOOL,
show_default=True,
help="Ensure formatting of C++ files.")
@click.option("--with-cpplint", default=True, type=BOOL,
show_default=True,
help="Ensure linting of C++ files with cpplint.")
@click.option("--with-clang-tidy", default=False, type=BOOL,
show_default=True,
help="Lint C++ with clang-tidy.")
@click.option("--with-iwyu", default=False, type=BOOL,
show_default=True,
help="Lint C++ with Include-What-You-Use (iwyu).")
@click.option("--with-flake8", default=True, type=BOOL,
show_default=True,
help="Lint python files with flake8.")
@click.option("--with-cmake-format", default=True, type=BOOL,
show_default=True,
help="Lint CMakeFiles.txt files with cmake-format.py.")
@click.option("--with-rat", default=True, type=BOOL,
show_default=True,
help="Lint files for license violation via apache-rat.")
@click.option("--with-r", default=True, type=BOOL,
show_default=True,
help="Lint r files.")
@click.option("--with-rust", default=True, type=BOOL,
show_default=True,
help="Lint rust files.")
@click.option("--with-docker", default=True, type=BOOL,
show_default=True,
help="Lint docker images with hadolint.")
@click.option("--fix", type=BOOL, default=False,
@click.option("--fix", is_flag=True, type=BOOL, default=False,
help="Toggle fixing the lint errors if the linter supports it.")
@click.pass_context
def lint(ctx, src, **kwargs):
Expand Down
9 changes: 8 additions & 1 deletion dev/archery/archery/lang/cpp.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ def __init__(self,
with_parquet=False, with_gandiva=False, with_plasma=False,
with_flight=False,
# extras
with_lint_only=False,
cmake_extras=None):
self.cc = cc
self.cxx = cxx
Expand All @@ -59,6 +60,8 @@ def __init__(self,
self.with_plasma = with_plasma
self.with_flight = with_flight

self.with_lint_only = with_lint_only

self.cmake_extras = cmake_extras

def _gen_defs(self):
Expand All @@ -68,7 +71,9 @@ def _gen_defs(self):
yield ("CMAKE_EXPORT_COMPILE_COMMANDS", truthifier(True))
yield ("CMAKE_BUILD_TYPE", or_else(self.build_type, "debug"))

yield ("BUILD_WARNING_LEVEL", or_else(self.warn_level, "production"))
if not self.with_lint_only:
yield ("BUILD_WARNING_LEVEL",
or_else(self.warn_level, "production"))

# if not ctx.quiet:
# yield ("ARROW_VERBOSE_THIRDPARTY_BUILD", "ON")
Expand All @@ -90,6 +95,8 @@ def _gen_defs(self):
yield ("ARROW_PLASMA", truthifier(self.with_plasma))
yield ("ARROW_FLIGHT", truthifier(self.with_flight))

yield ("ARROW_LINT_ONLY", truthifier(self.with_lint_only))

# Detect custom conda toolchain
if self.use_conda:
for d, v in [('CMAKE_AR', 'AR'), ('CMAKE_RANLIB', 'RANLIB')]:
Expand Down
2 changes: 1 addition & 1 deletion dev/archery/archery/lang/java.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

class Java(Command):
def __init__(self, java_bin=None):
self.bin = default_bin(java_bin, "JAVA", "java")
self.bin = default_bin(java_bin, "java")


class Jar(CommandStackMixin, Java):
Expand Down
2 changes: 1 addition & 1 deletion dev/archery/archery/lang/rust.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@

class Cargo(Command):
def __init__(self, cargo_bin=None):
self.bin = default_bin(cargo_bin, "CARGO", "cargo")
self.bin = default_bin(cargo_bin, "cargo")
2 changes: 1 addition & 1 deletion dev/archery/archery/utils/cmake.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

class CMake(Command):
def __init__(self, cmake_bin=None):
self.bin = default_bin(cmake_bin, "CMAKE", "cmake")
self.bin = default_bin(cmake_bin, "cmake")

@staticmethod
def default_generator():
Expand Down
27 changes: 13 additions & 14 deletions dev/archery/archery/utils/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,17 @@
# under the License.

import os
import shlex
import shutil
import subprocess

from .logger import logger, ctx


def find_exec(executable):
exec_exists = os.path.exists(executable)
executable = executable if exec_exists else shutil.which(executable)

if executable is None:
raise FileNotFoundError(executable)

return executable


def default_bin(name, env, default):
return name if name else os.environ.get(env, default)
def default_bin(name, default):
assert(default)
env_name = "ARCHERY_%s_BIN".format(default.upper())
return name if name else os.environ.get(env_name, default)


# Decorator running a command and returning stdout
Expand Down Expand Up @@ -65,7 +58,7 @@ class Command:

def run(self, *argv, **kwargs):
assert(hasattr(self, "bin"))
invocation = [find_exec(self.bin)]
invocation = shlex.split(self.bin)
invocation.extend(argv)

for key in ["stdout", "stderr"]:
Expand All @@ -80,6 +73,12 @@ def run(self, *argv, **kwargs):
logger.debug(f"Executing `{invocation}`")
return subprocess.run(invocation, **kwargs)

@property
def available(self):
""" Indicate if the command binary is found in PATH. """
binary = shlex.split(self.bin)[0]
return shutil.which(binary) is not None

def __call__(self, *argv, **kwargs):
return self.run(*argv, **kwargs)

Expand All @@ -92,4 +91,4 @@ def run(self, *argv, **kwargs):

class Bash(Command):
def __init__(self, bash_bin=None):
self.bin = default_bin(bash_bin, "BASH", "bash")
self.bin = default_bin(bash_bin, "bash")
2 changes: 1 addition & 1 deletion dev/archery/archery/utils/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def wrapper(self, *argv, **kwargs):

class Git(Command):
def __init__(self, git_bin=None):
self.bin = default_bin(git_bin, "GIT", "git")
self.bin = default_bin(git_bin, "git")

def run_cmd(self, cmd, *argv, git_dir=None, **kwargs):
""" Inject flags before sub-command in argv. """
Expand Down
44 changes: 36 additions & 8 deletions dev/archery/archery/utils/lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import os

from .command import Bash, Command, default_bin
from .cmake import CMake
from .git import git
from .logger import logger
from ..lang.cpp import CppCMakeDefinition, CppConfiguration
Expand Down Expand Up @@ -49,11 +50,19 @@ def cpp_linter(src, build_dir, clang_format=True, cpplint=True,
""" Run clang-format, cpplint and clang-tidy on cpp/ codebase. """
logger.info("Running C++ linters")

cmake = CMake()
if not cmake.available:
logger.error("cpp linter requested but cmake binary not found.")
return

# A cmake build directory is required to populate `compile_commands.json`
# which in turn is required by clang-tidy. It also provides a convenient
# way to hide clang-format/clang-tidy invocation via the Generate
# (ninja/make) targets.
cmake_args = {"with_python": False}

# ARROW_LINT_ONLY exits early but ignore building compile_command.json
lint_only = not (iwyu or clang_tidy)
cmake_args = {"with_python": False, "with_lint_only": lint_only}
cmake_def = CppCMakeDefinition(src.cpp, CppConfiguration(**cmake_args))

build = cmake_def.build(build_dir)
Expand All @@ -80,23 +89,29 @@ def __init__(self, cmake_format_bin):
def cmake_linter(src, fix=False):
""" Run cmake-format.py on all CMakeFiles.txt """
logger.info("Running cmake-format linters")

if not fix:
logger.warn("run-cmake-format modifies files, irregardless of --fix")

arrow_cmake_format = os.path.join(src.path, "run-cmake-format.py")
cmake_format = CMakeFormat(cmake_format_bin=arrow_cmake_format)
yield LintResult.from_cmd(cmake_format("--check"))


class Flake8(Command):
def __init__(self, flake8_bin=None):
self.bin = default_bin(flake8_bin, "FLAKE8", "flake8")
self.bin = default_bin(flake8_bin, "flake8")


def python_linter(src):
""" Run flake8 linter on python/pyarrow, and dev/. """
logger.info("Running python linters")
flake8 = Flake8()

if not flake8.available:
logger.error("python linter requested but flake8 binary not found.")
return

yield LintResult.from_cmd(flake8(src.pyarrow, src.dev, check=False))
config = os.path.join(src.pyarrow, ".flake8.cython")
yield LintResult.from_cmd(flake8("--config=" + config, src.pyarrow,
Expand Down Expand Up @@ -137,14 +152,20 @@ def r_linter(src):
def rust_linter(src):
""" Run Rust linter. """
logger.info("Running rust linter")
yield LintResult.from_cmd(Cargo().run("+stable", "fmt", "--all", "--",
"--check", cwd=src.rust,
check=False))
cargo = Cargo()

if not cargo.available:
logger.error("rust linter requested but cargo executable not found.")
return

yield LintResult.from_cmd(cargo.run("+stable", "fmt", "--all", "--",
"--check", cwd=src.rust,
check=False))


class Hadolint(Command):
def __init__(self, hadolint_bin=None):
self.bin = default_bin(hadolint_bin, "HADOLINT", "hadolint")
self.bin = default_bin(hadolint_bin, "hadolint")


def is_docker_image(path):
Expand All @@ -161,10 +182,17 @@ def docker_linter(src):
""" Run Hadolint docker linter. """
logger.info("Running docker linter")

hadolint = Hadolint()

if not hadolint.available:
logger.error(
"hadolint linter requested but hadolint binary not found.")
return

for path in git.ls_files(git_dir=src.path):
if is_docker_image(path):
yield LintResult.from_cmd(Hadolint().run(path, check=False,
cwd=src.path))
yield LintResult.from_cmd(hadolint.run(path, check=False,
cwd=src.path))


def linter(src, with_clang_format=True, with_cpplint=True,
Expand Down

0 comments on commit d5ba83e

Please sign in to comment.