From 0e0e12aa22126dcec8a699ee880fd8a0e4a9fdcf Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Mon, 21 Sep 2020 14:12:55 -0700 Subject: [PATCH] Using comp_db for clang-tidy --- .clang_complete | 20 --- .gitignore | 3 + bazel/grpc_deps.bzl | 11 ++ .../grpc_clang_tidy/Dockerfile.template | 2 +- .../test/sanity/Dockerfile.template | 2 +- tools/bazel.rc | 3 + tools/distrib/clang_tidy_code.sh | 26 +++- tools/distrib/gen_compilation_database.py | 135 ++++++++++++++++++ tools/distrib/run_clang_tidy.py | 23 +-- tools/dockerfile/grpc_clang_tidy/Dockerfile | 2 +- .../clang_tidy_all_the_things.sh | 8 +- tools/dockerfile/test/sanity/Dockerfile | 2 +- .../run_tests/sanity/check_bazel_workspace.py | 8 +- tools/run_tests/sanity/sanity_tests.yaml | 3 + 14 files changed, 204 insertions(+), 44 deletions(-) delete mode 100644 .clang_complete create mode 100755 tools/distrib/gen_compilation_database.py diff --git a/.clang_complete b/.clang_complete deleted file mode 100644 index 2769e899722..00000000000 --- a/.clang_complete +++ /dev/null @@ -1,20 +0,0 @@ --Wall --Wc++-compat --I. --Igens --Iinclude --Isrc/core/ext/upb-generated --Ithird_party/abseil-cpp --Ithird_party/address_sorting/include --Ithird_party/benchmark/include --Ithird_party/boringssl-with-bazel/src/include --Ithird_party/cares --Ithird_party/cares/cares --Ithird_party/googletest --Ithird_party/googletest/googlemock/include --Ithird_party/googletest/googletest/include --Ithird_party/googletest/include --Ithird_party/protobuf/src --Ithird_party/re2 --Ithird_party/upb --Ithird_party/zlib diff --git a/.gitignore b/.gitignore index c0ec88139f2..cff0222ef8e 100644 --- a/.gitignore +++ b/.gitignore @@ -151,3 +151,6 @@ BenchmarkDotNet.Artifacts/ # pyenv config .python-version + +# clang JSON compilation database file +compile_commands.json diff --git a/bazel/grpc_deps.bzl b/bazel/grpc_deps.bzl index ed8786ed684..75d1b4dbbe3 100644 --- a/bazel/grpc_deps.bzl +++ b/bazel/grpc_deps.bzl @@ -269,6 +269,17 @@ def grpc_deps(): sha256 = "97e70364e9249702246c0e9444bccdc4b847bed1eb03c5a3ece4f83dfe6abc44", ) + if "bazel_compdb" not in native.existing_rules(): + http_archive( + name = "bazel_compdb", + sha256 = "bcecfd622c4ef272fd4ba42726a52e140b961c4eac23025f18b346c968a8cfb4", + strip_prefix = "bazel-compilation-database-0.4.5", + urls = [ + "https://storage.googleapis.com/grpc-bazel-mirror/github.com/grailbio/bazel-compilation-database/archive/0.4.5.tar.gz", + "https://github.com/grailbio/bazel-compilation-database/archive/0.4.5.tar.gz", + ], + ) + if "io_opencensus_cpp" not in native.existing_rules(): http_archive( name = "io_opencensus_cpp", diff --git a/templates/tools/dockerfile/grpc_clang_tidy/Dockerfile.template b/templates/tools/dockerfile/grpc_clang_tidy/Dockerfile.template index e56f76d63cb..4d5c83fda04 100644 --- a/templates/tools/dockerfile/grpc_clang_tidy/Dockerfile.template +++ b/templates/tools/dockerfile/grpc_clang_tidy/Dockerfile.template @@ -20,7 +20,7 @@ # This is because clang-tidy 7.0 started treating compiler errors as tidy errors # and there are a couple of files which are not properly compiled via tidy so it # should be using 6.0 version until all compilation errors are addressed. - RUN apt-get update && apt-get install -y clang-tidy-6.0 + RUN apt-get update && apt-get install -y clang-tidy-6.0 jq ENV CLANG_TIDY=clang-tidy-6.0 ADD clang_tidy_all_the_things.sh / diff --git a/templates/tools/dockerfile/test/sanity/Dockerfile.template b/templates/tools/dockerfile/test/sanity/Dockerfile.template index 453505d7c9e..314f3ab625f 100644 --- a/templates/tools/dockerfile/test/sanity/Dockerfile.template +++ b/templates/tools/dockerfile/test/sanity/Dockerfile.template @@ -40,7 +40,7 @@ # This is because clang-tidy 7.0 started treating compiler errors as tidy errors # and there are a couple of files which are not properly compiled via tidy so it # should be using 6.0 version until all compilation errors are addressed. - RUN apt-get install -y clang-tidy-6.0 + RUN apt-get install -y clang-tidy-6.0 jq ENV CLANG_TIDY=clang-tidy-6.0 diff --git a/tools/bazel.rc b/tools/bazel.rc index 02795e434dc..7b687484846 100644 --- a/tools/bazel.rc +++ b/tools/bazel.rc @@ -103,3 +103,6 @@ build:mutrace --copt=-O3 build:mutrace --copt=-fno-omit-frame-pointer build:mutrace --copt=-DNDEBUG build:mutrace --linkopt=-rdynamic + +# Compile database generation config +build:compdb --build_tag_filters=-nocompdb diff --git a/tools/distrib/clang_tidy_code.sh b/tools/distrib/clang_tidy_code.sh index 9262b6bd3e1..10056d6d2a2 100755 --- a/tools/distrib/clang_tidy_code.sh +++ b/tools/distrib/clang_tidy_code.sh @@ -21,6 +21,22 @@ set -ex cd $(dirname $0)/../.. REPO_ROOT=$(pwd) +# grep targets with manual tag, which is not included in a result of bazel build using ... +# let's get a list of them using query command and pass it to gen_compilation_database.py +export MANUAL_TARGETS=$(bazel query 'attr("tags", "manual", tests(//test/cpp/...))' | grep -v _on_ios) + +# generate a clang compilation database +tools/distrib/gen_compilation_database.py \ + --include_headers \ + --ignore_system_headers \ + --dedup_targets \ + "//:*" \ + "//src/core/..." \ + "//src/compiler/..." \ + "//test/core/..." \ + "//test/cpp/..." \ + $MANUAL_TARGETS + if [ "$CLANG_TIDY_SKIP_DOCKER" == "" ] then # build clang-tidy docker image @@ -29,7 +45,15 @@ then # run clang-tidy against the checked out codebase # when modifying the checked-out files, the current user will be impersonated # so that the updated files don't end up being owned by "root". - docker run -e TEST="$TEST" -e CHANGED_FILES="$CHANGED_FILES" -e CLANG_TIDY_ROOT="/local-code" --rm=true -v "${REPO_ROOT}":/local-code --user "$(id -u):$(id -g)" -t grpc_clang_tidy /clang_tidy_all_the_things.sh "$@" + docker run \ + -e TEST="$TEST" \ + -e CHANGED_FILES="$CHANGED_FILES" \ + -e CLANG_TIDY_ROOT="/local-code" \ + --rm=true \ + -v "${REPO_ROOT}":/local-code \ + -v "${HOME/.cache/bazel}":"${HOME/.cache/bazel}" \ + --user "$(id -u):$(id -g)" \ + -t grpc_clang_tidy /clang_tidy_all_the_things.sh "$@" else CLANG_TIDY_ROOT="${REPO_ROOT}" tools/dockerfile/grpc_clang_tidy/clang_tidy_all_the_things.sh "$@" fi diff --git a/tools/distrib/gen_compilation_database.py b/tools/distrib/gen_compilation_database.py new file mode 100755 index 00000000000..d8d2761b8e4 --- /dev/null +++ b/tools/distrib/gen_compilation_database.py @@ -0,0 +1,135 @@ +#!/usr/bin/env python3 + +# Copyright 2020 gRPC authors. +# +# Licensed 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. + +# This is based on the script on the Envoy project +# https://github.com/envoyproxy/envoy/blob/master/tools/gen_compilation_database.py + +import argparse +import glob +import json +import logging +import os +import re +import shlex +import subprocess +from pathlib import Path + +RE_INCLUDE_SYSTEM = re.compile("\s*-I\s+/usr/[^ ]+") + + +# This method is equivalent to https://github.com/grailbio/bazel-compilation-database/blob/master/generate.sh +def generateCompilationDatabase(args): + # We need to download all remote outputs for generated source code. + # This option lives here to override those specified in bazelrc. + bazel_options = shlex.split(os.environ.get("BAZEL_BUILD_OPTIONS", "")) + [ + "--config=compdb", + "--remote_download_outputs=all", + ] + + subprocess.check_call(["bazel", "build"] + bazel_options + [ + "--aspects=@bazel_compdb//:aspects.bzl%compilation_database_aspect", + "--output_groups=compdb_files,header_files" + ] + args.bazel_targets) + + execroot = subprocess.check_output(["bazel", "info", "execution_root"] + + bazel_options).decode().strip() + + compdb = [] + for compdb_file in Path(execroot).glob("**/*.compile_commands.json"): + compdb.extend( + json.loads( + "[" + + compdb_file.read_text().replace("__EXEC_ROOT__", execroot) + + "]")) + + if args.dedup_targets: + compdb_map = {target["file"]: target for target in compdb} + compdb = list(compdb_map.values()) + + return compdb + + +def isHeader(filename): + for ext in (".h", ".hh", ".hpp", ".hxx"): + if filename.endswith(ext): + return True + return False + + +def isCompileTarget(target, args): + filename = target["file"] + if not args.include_headers and isHeader(filename): + return False + if not args.include_genfiles: + if filename.startswith("bazel-out/"): + return False + if not args.include_external: + if filename.startswith("external/"): + return False + return True + + +def modifyCompileCommand(target, args): + cc, options = target["command"].split(" ", 1) + + # Workaround for bazel added C++11 options, those doesn't affect build itself but + # clang-tidy will misinterpret them. + options = options.replace("-std=c++0x ", "") + options = options.replace("-std=c++11 ", "") + + if args.vscode: + # Visual Studio Code doesn't seem to like "-iquote". Replace it with + # old-style "-I". + options = options.replace("-iquote ", "-I ") + + if args.ignore_system_headers: + # Remove all include options for /usr/* directories + options = RE_INCLUDE_SYSTEM.sub("", options) + + if isHeader(target["file"]): + options += " -Wno-pragma-once-outside-header -Wno-unused-const-variable" + options += " -Wno-unused-function" + if not target["file"].startswith("external/"): + # *.h file is treated as C header by default while our headers files are all C++11. + options = "-x c++ -std=c++11 -fexceptions " + options + + target["command"] = " ".join([cc, options]) + return target + + +def fixCompilationDatabase(args, db): + db = [ + modifyCompileCommand(target, args) + for target in db + if isCompileTarget(target, args) + ] + + with open("compile_commands.json", "w") as db_file: + json.dump(db, db_file, indent=2) + + +if __name__ == "__main__": + parser = argparse.ArgumentParser( + description='Generate JSON compilation database') + parser.add_argument('--include_external', action='store_true') + parser.add_argument('--include_genfiles', action='store_true') + parser.add_argument('--include_headers', action='store_true') + parser.add_argument('--vscode', action='store_true') + parser.add_argument('--ignore_system_headers', action='store_true') + parser.add_argument('--dedup_targets', action='store_true') + parser.add_argument('bazel_targets', nargs='*', default=["//..."]) + args = parser.parse_args() + fixCompilationDatabase(args, generateCompilationDatabase(args)) diff --git a/tools/distrib/run_clang_tidy.py b/tools/distrib/run_clang_tidy.py index 6e54a7593db..6cdddcc5781 100755 --- a/tools/distrib/run_clang_tidy.py +++ b/tools/distrib/run_clang_tidy.py @@ -24,17 +24,6 @@ sys.path.append( 'python_utils')) import jobset -extra_args = [ - '-x', - 'c++', - '-std=c++11', -] -with open('.clang_complete') as f: - for line in f: - line = line.strip() - if line.startswith('-I'): - extra_args.append(line) - clang_tidy = os.environ.get('CLANG_TIDY', 'clang-tidy') argp = argparse.ArgumentParser(description='Run clang-tidy against core') @@ -50,17 +39,19 @@ args = argp.parse_args() cmdline = [ clang_tidy, -] + ['--extra-arg-before=%s' % arg for arg in extra_args] +] if args.fix: cmdline.append('--fix') jobs = [] for filename in args.files: - jobs.append(jobset.JobSpec( - cmdline + [filename], - shortname=filename, - )) #verbose_success=True)) + jobs.append( + jobset.JobSpec( + cmdline + [filename], + shortname=filename, + timeout_seconds=15 * 60, + )) num_fails, res_set = jobset.run(jobs, maxjobs=args.jobs) sys.exit(num_fails) diff --git a/tools/dockerfile/grpc_clang_tidy/Dockerfile b/tools/dockerfile/grpc_clang_tidy/Dockerfile index 494dc6423a9..54eb6a52e06 100644 --- a/tools/dockerfile/grpc_clang_tidy/Dockerfile +++ b/tools/dockerfile/grpc_clang_tidy/Dockerfile @@ -65,7 +65,7 @@ RUN mkdir /var/local/jenkins # This is because clang-tidy 7.0 started treating compiler errors as tidy errors # and there are a couple of files which are not properly compiled via tidy so it # should be using 6.0 version until all compilation errors are addressed. -RUN apt-get update && apt-get install -y clang-tidy-6.0 +RUN apt-get update && apt-get install -y clang-tidy-6.0 jq ENV CLANG_TIDY=clang-tidy-6.0 ADD clang_tidy_all_the_things.sh / diff --git a/tools/dockerfile/grpc_clang_tidy/clang_tidy_all_the_things.sh b/tools/dockerfile/grpc_clang_tidy/clang_tidy_all_the_things.sh index cad2b2b3732..947ffbafb71 100755 --- a/tools/dockerfile/grpc_clang_tidy/clang_tidy_all_the_things.sh +++ b/tools/dockerfile/grpc_clang_tidy/clang_tidy_all_the_things.sh @@ -20,5 +20,9 @@ CLANG_TIDY=${CLANG_TIDY:-clang-tidy} cd ${CLANG_TIDY_ROOT} -find src/core src/cpp test/core test/cpp ! -path 'src/core/ext/upb-generated/*' ! -path 'src/core/ext/upbdefs-generated/*' -name '*.h' -or -name '*.cc' -print0 \ - | xargs -0 tools/distrib/run_clang_tidy.py "$@" +# run clang tidy for all source files +cat compile_commands.json | jq -r '.[].file' \ + | grep -E "(^src/core/|^src/cpp/|^test/core/|^test/cpp/)" \ + | grep -v -E "/upb-generated/|/upbdefs-generated/" \ + | sort \ + | xargs tools/distrib/run_clang_tidy.py "$@" diff --git a/tools/dockerfile/test/sanity/Dockerfile b/tools/dockerfile/test/sanity/Dockerfile index 7a75aba3655..87ce4d0d0f9 100644 --- a/tools/dockerfile/test/sanity/Dockerfile +++ b/tools/dockerfile/test/sanity/Dockerfile @@ -88,7 +88,7 @@ RUN apt-get install -y clang clang-format # This is because clang-tidy 7.0 started treating compiler errors as tidy errors # and there are a couple of files which are not properly compiled via tidy so it # should be using 6.0 version until all compilation errors are addressed. -RUN apt-get install -y clang-tidy-6.0 +RUN apt-get install -y clang-tidy-6.0 jq ENV CLANG_TIDY=clang-tidy-6.0 diff --git a/tools/run_tests/sanity/check_bazel_workspace.py b/tools/run_tests/sanity/check_bazel_workspace.py index 69afd0860e3..b784a15eec9 100755 --- a/tools/run_tests/sanity/check_bazel_workspace.py +++ b/tools/run_tests/sanity/check_bazel_workspace.py @@ -35,6 +35,7 @@ git_submodule_hashes = { _BAZEL_SKYLIB_DEP_NAME = 'bazel_skylib' _BAZEL_TOOLCHAINS_DEP_NAME = 'bazel_toolchains' +_BAZEL_COMPDB_DEP_NAME = 'bazel_compdb' _TWISTED_TWISTED_DEP_NAME = 'com_github_twisted_twisted' _YAML_PYYAML_DEP_NAME = 'com_github_yaml_pyyaml' _TWISTED_INCREMENTAL_DEP_NAME = 'com_github_twisted_incremental' @@ -56,6 +57,7 @@ _GRPC_DEP_NAMES = [ 'envoy_api', _BAZEL_SKYLIB_DEP_NAME, _BAZEL_TOOLCHAINS_DEP_NAME, + _BAZEL_COMPDB_DEP_NAME, _TWISTED_TWISTED_DEP_NAME, _YAML_PYYAML_DEP_NAME, _TWISTED_INCREMENTAL_DEP_NAME, @@ -75,6 +77,7 @@ _GRPC_BAZEL_ONLY_DEPS = [ 'io_opencensus_cpp', _BAZEL_SKYLIB_DEP_NAME, _BAZEL_TOOLCHAINS_DEP_NAME, + _BAZEL_COMPDB_DEP_NAME, _TWISTED_TWISTED_DEP_NAME, _YAML_PYYAML_DEP_NAME, _TWISTED_INCREMENTAL_DEP_NAME, @@ -149,7 +152,10 @@ build_rules = { exec(bazel_file) in build_rules for name in _GRPC_DEP_NAMES: assert name in names_and_urls.keys() -assert len(_GRPC_DEP_NAMES) == len(names_and_urls.keys()) +if len(_GRPC_DEP_NAMES) != len(names_and_urls.keys()): + assert False, "Diff: " + (str(set(_GRPC_DEP_NAMES) - set(names_and_urls)) + + "," + + str(set(names_and_urls) - set(_GRPC_DEP_NAMES))) # There are some "bazel-only" deps that are exceptions to this sanity check, # we don't require that there is a corresponding git module for these. diff --git a/tools/run_tests/sanity/sanity_tests.yaml b/tools/run_tests/sanity/sanity_tests.yaml index b94e7b3a739..ba2a219b48f 100644 --- a/tools/run_tests/sanity/sanity_tests.yaml +++ b/tools/run_tests/sanity/sanity_tests.yaml @@ -24,6 +24,9 @@ - script: tools/distrib/check_pytype.sh - script: tools/distrib/clang_format_code.sh - script: tools/distrib/clang_tidy_code.sh + # ClangTidy needs to run exclusively because it runs bazel making changes + # and it needs many CPUs to parse all source files to be diagnosed. + cpu_cost: 1000 - script: tools/distrib/pylint_code.sh - script: tools/distrib/python/check_grpcio_tools.py - script: tools/distrib/yapf_code.sh --diff