Skip to content

Commit

Permalink
ARROW-2981: [C++] improve clang-tidy usability
Browse files Browse the repository at this point in the history
- adds a docker-compose service for running clang-tidy
- docker-compose runs as root, so the files touched by clang-tidy and clang-format were owned by root. They are now passed back to the user
- clang-format is run after clang-tidy because the latter munges formatting

I ran clang-tidy then cleaned up the build errors [in this branch](apache/arrow@f92830f...bkietz:clang-tidy-run-example) to give an idea of what things are changed and what things can go wrong.

Author: Benjamin Kietzman <[email protected]>

Closes apache#4293 from bkietz/2981-Support-scripts-documentation-for-runnin and squashes the following commits:

63ac52c <Benjamin Kietzman> refactor clang-tidy: don't modify sources
31e4755 <Benjamin Kietzman> run code-modifying linters with lint_user
fa5af80 <Benjamin Kietzman> add description of producing HeaderFilterRegex
3ecc91d <Benjamin Kietzman> built-in clang-format didn't match ninja format
2193dab <Benjamin Kietzman> mention clang-tidy in integration.rst
0724b55 <Benjamin Kietzman> update clang-tidy's header regex to sort-of match lint_exclusions.txt
df3a121 <Benjamin Kietzman> clang-tidy can run clang-format automatically
ed2b231 <Benjamin Kietzman> maintain ownership when running clang-{format,tidy}
a87dd04 <Benjamin Kietzman> adding docker-compose endpoint for clang-tidy
  • Loading branch information
bkietz authored and fsaintjacques committed Jun 14, 2019
1 parent f8cd263 commit 72b5531
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 5 deletions.
4 changes: 3 additions & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
# under the License.
---
Checks: 'clang-diagnostic-*,clang-analyzer-*,-clang-analyzer-alpha*,google-*,modernize-*,readability-*'
HeaderFilterRegex: 'arrow/.*'
# produce HeaderFilterRegex from cpp/build-support/lint_exclusions.txt with:
# echo -n '^('; sed -e 's/*/\.*/g' cpp/build-support/lint_exclusions.txt | tr '\n' '|'; echo ')$'
HeaderFilterRegex: '^(.*codegen.*|.*_generated.*|.*windows_compatibility.h|.*pyarrow_api.h|.*pyarrow_lib.h|.*python/config.h|.*python/platform.h|.*thirdparty/ae/.*|.*vendored/.*|.*RcppExports.cpp.*|)$'
AnalyzeTemporaryDtors: true
CheckOptions:
- key: google-readability-braces-around-statements.ShortStatementLines
Expand Down
12 changes: 9 additions & 3 deletions cpp/build-support/run_clang_tidy.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,13 @@ def _check_all(cmd, filenames):
help="If specified, only print errors")
arguments = parser.parse_args()

exclude_globs = []
if arguments.exclude_globs:
for line in open(arguments.exclude_globs):
exclude_globs.append(line.strip())

linted_filenames = []
for path in lintutils.get_sources(arguments.source_dir):
for path in lintutils.get_sources(arguments.source_dir, exclude_globs):
linted_filenames.append(path)

if not arguments.quiet:
Expand All @@ -111,8 +116,9 @@ def _check_all(cmd, filenames):
cmd.append('-fix')
results = lintutils.run_parallel(
[cmd + some for some in lintutils.chunk(linted_filenames, 16)])
for result in results:
result.check_returncode()
for returncode, stdout, stderr in results:
if returncode != 0:
sys.exit(returncode)

else:
_check_all(cmd, linted_filenames)
25 changes: 25 additions & 0 deletions dev/lint/run_clang_tidy.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#!/bin/bash
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

set -ex

mkdir -p /build/lint
pushd /build/lint
cmake -GNinja /arrow/cpp
ninja check-clang-tidy
popd
10 changes: 10 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,16 @@ services:
command: arrow/dev/lint/run_clang_format.sh
volumes: *ubuntu-volumes

clang-tidy:
# Usage:
# docker-compose build cpp
# docker-compose build python
# docker-compose build lint
# docker-compose run clang-tidy
image: arrow:lint
command: arrow/dev/lint/run_clang_tidy.sh
volumes: *ubuntu-volumes

docs:
# Usage:
# docker-compose build cpp
Expand Down
3 changes: 2 additions & 1 deletion docs/source/developers/integration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ build mount is used for caching and sharing state between staged images.
- *rust*: Builds the rust project
- *lint*: Run various lint on the C++ sources
- *iwyu*: Run include-what-you-use on the C++ sources
- *clang-format*: Run clang-format on the C++ sources
- *clang-format*: Run clang-format on the C++ sources, modifying in place
- *clang-tidy*: Run clang-tidy on the C++ sources, outputting recommendations
- *docs*: Builds this documentation

You can build and run a service by using the `build` and `run` docker-compose
Expand Down

0 comments on commit 72b5531

Please sign in to comment.