From 230e16ffdcc557c2fdb33f60c4cddbc957be39ff Mon Sep 17 00:00:00 2001 From: phlax Date: Mon, 11 Sep 2023 21:02:34 +0100 Subject: [PATCH] clang/tools: Use python distributed versions (#29455) Signed-off-by: Ryan Northey --- tools/base/requirements.in | 2 + tools/base/requirements.txt | 23 ++++++++- tools/clang-format/BUILD | 12 +++++ tools/clang-format/clang_format.bzl | 60 ++++++++++++++++++++++ tools/code_format/BUILD | 2 + tools/code_format/check_format.py | 79 ++++++++--------------------- tools/local_fix_format.sh | 9 +++- 7 files changed, 126 insertions(+), 61 deletions(-) create mode 100644 tools/clang-format/BUILD create mode 100644 tools/clang-format/clang_format.bzl diff --git a/tools/base/requirements.in b/tools/base/requirements.in index dac380272b35..e34f513c3053 100644 --- a/tools/base/requirements.in +++ b/tools/base/requirements.in @@ -2,6 +2,8 @@ abstracts>=0.0.12 aio.api.bazel aiohttp>=3.8.1 cffi>=1.15.0 +clang-format==14.0.6 +clang-tidy==14.0.6 colorama coloredlogs cryptography>=41.0.1 diff --git a/tools/base/requirements.txt b/tools/base/requirements.txt index 4bb9504c318a..ac6349030d50 100644 --- a/tools/base/requirements.txt +++ b/tools/base/requirements.txt @@ -368,6 +368,27 @@ charset-normalizer==3.2.0 \ # via # aiohttp # requests +clang-format==14.0.6 \ + --hash=sha256:13f2d6d4a2af004a783c65f0921afa8f0384bffcdaf500b6c2cb542edeb0b4a5 \ + --hash=sha256:810c649ab97d208cd418c897d50ab6e958eb8d96854527edd80d0dd21a75e914 \ + --hash=sha256:aaf4edecc46a24f0b572b82cf5827e292ad1c137903427627c4d5f671668cc2b \ + --hash=sha256:bd400c47665dd19afc03f98e747f78ed828abab99c6a1b07e137b35c1cd3cc26 \ + --hash=sha256:c93580945f75de7e01996f1fb3cf67e4dc424f1c864e237c85614fb99a48c7a4 \ + --hash=sha256:d5c96b500d7f8b5d2db5b75ac035be387512850ad589cdc3019666b861382136 \ + --hash=sha256:d780c04334bca80f2b60d25bf53c37bd0618520ee295a7888a11f25bde114ac4 \ + --hash=sha256:d7c1c5e404c58e55f0170f01b3c5611dce6c119e62b5d1020347e0ad97d5a047 \ + --hash=sha256:dbfd60528eb3bb7d7cfe8576faa70845fbf93601f815ef75163d36606e87f388 + # via -r requirements.in +clang-tidy==14.0.6 \ + --hash=sha256:02bce40a56cc344e20d2f63bef6b85acf9837954559e0091804d6e748dfc0359 \ + --hash=sha256:173a757415108095b541eb9a2d0c222d41f5624e7bb5b98772476957228ce2c7 \ + --hash=sha256:4635f6553f9e3eb7a81fec29d15e4e70b49c1780f31a17550c11007fc9bba4b3 \ + --hash=sha256:5b56edb6b7215eb79fede7ab8a4f9b94454bdfe1091d026acc1afdc7696abb68 \ + --hash=sha256:7f75eb4839dc996dea494a07814b3a70200be75bc7d9acb54d3d5916f24bcd8d \ + --hash=sha256:c9ffcb91f17ee920fdd7a83f30484f3cb4c183f7b490d092373e4a6f2c82729d \ + --hash=sha256:d595b8e9a155d63b6b9dec0afa62725590626c9f0e945c3d9e448a28e0082b39 \ + --hash=sha256:fef62fb706adccef94128761ca0796973a196e2d60fb938a312cfa2bc59730bd + # via -r requirements.in colorama==0.4.6 \ --hash=sha256:08695f5cb7ed6e0531a20572697297273c47b8cae5a63ffc6d6ed5c201be6e44 \ --hash=sha256:4f1d9991f5acc0ca119f9d443620b77f9d6b33703e51011c16baf57afb285fc6 @@ -1235,7 +1256,7 @@ sphinxcontrib-httpdomain==1.8.1 \ --hash=sha256:21eefe1270e4d9de8d717cc89ee92cc4871b8736774393bafc5e38a6bb77b1d5 \ --hash=sha256:6c2dfe6ca282d75f66df333869bb0ce7331c01b475db6809ff9d107b7cdfe04b # via envoy-docs-sphinx-runner -sphinxcontrib-jquery @ https://github.com/sphinx-contrib/jquery/archive/refs/tags/v3.0.0.zip \ +sphinxcontrib.jquery @ https://github.com/sphinx-contrib/jquery/archive/refs/tags/v3.0.0.zip \ --hash=sha256:562ad9ac0ac3d8f04a363eb3507ae4b2b856aa04aabab6df7543530fafb849ca # via # -r requirements.in diff --git a/tools/clang-format/BUILD b/tools/clang-format/BUILD new file mode 100644 index 000000000000..604edb37e7c9 --- /dev/null +++ b/tools/clang-format/BUILD @@ -0,0 +1,12 @@ +load("@base_pip3//:requirements.bzl", "requirement") +load("//bazel:envoy_build_system.bzl", "envoy_package") +load(":clang_format.bzl", "clang_format") + +licenses(["notice"]) # Apache 2 + +envoy_package() + +clang_format( + name = "clang-format", + target = requirement("clang-format"), +) diff --git a/tools/clang-format/clang_format.bzl b/tools/clang-format/clang_format.bzl new file mode 100644 index 000000000000..c21dc465ffd0 --- /dev/null +++ b/tools/clang-format/clang_format.bzl @@ -0,0 +1,60 @@ +# +# This fishes the clang-format binary out of the related python package. +# +# This is useful as using the binary through the python entry_point adds a lot of overhead. +# +# ```starlark +# +# load("@base_pip3//:requirements.bzl", "requirement") +# +# clang_format( +# name = "clang-format", +# target = requirement("clang-format"), +# ) +# +# ``` +# +# The exposed binary can also be run directly: +# +# ```console +# +# $ bazel run //tools/clang-format -- --version +# +# ``` +# + +def _clang_format_impl(ctx): + clang_bin = None + for file in ctx.attr.target[DefaultInfo].data_runfiles.files.to_list(): + if file.basename == "clang-format" and file.dirname.split("/").pop() == "bin": + clang_bin = file + break + + if not clang_bin: + fail("Unable to find clang-format file in package") + + output_file = ctx.actions.declare_file("clang-format") + args = ctx.actions.args() + args.add(clang_bin.path) + args.add(output_file.path) + ctx.actions.run( + outputs = [output_file], + inputs = [clang_bin], + arguments = [args], + executable = "cp", + mnemonic = "ClangFormatGetter", + ) + return [DefaultInfo( + executable = output_file, + files = depset([output_file]), + )] + +clang_format = rule( + implementation = _clang_format_impl, + attrs = { + "target": attr.label( + allow_files = True, + ), + }, + executable = True, +) diff --git a/tools/code_format/BUILD b/tools/code_format/BUILD index 0e3d75117729..73b9872299c7 100644 --- a/tools/code_format/BUILD +++ b/tools/code_format/BUILD @@ -22,11 +22,13 @@ py_binary( srcs = ["check_format.py"], args = [ "--path=%s" % PATH, + "--clang_format_path=$(location //tools/clang-format)", "--buildifier_path=$(location @com_github_bazelbuild_buildtools//buildifier)", "--buildozer_path=$(location @com_github_bazelbuild_buildtools//buildozer)", ], data = [ ":config.yaml", + "//tools/clang-format", "@com_github_bazelbuild_buildtools//buildifier", "@com_github_bazelbuild_buildtools//buildozer", ], diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index e6a89940fc81..327fb0303111 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -55,7 +55,10 @@ def buildozer_path(self) -> str: @cached_property def clang_format_path(self) -> str: """Path to the clang-format binary.""" - return os.getenv("CLANG_FORMAT", "clang-format-14") + path = ( + os.path.join(self.source_path, self.args.clang_format_path) + if self.source_path else self.args.clang_format_path) + return path @cached_property def config(self) -> Dict: @@ -128,7 +131,6 @@ def __init__(self, args): self.source_path = os.getcwd() if self.args.path: os.chdir(self.args.path) - os.environ["BAZEL_EXECROOT"] = self.source_path self._include_dir_order = self.args.include_dir_order @property @@ -217,6 +219,8 @@ def args(self): nargs="+", default=[], help="exclude paths from bazel_tools check.") + parser.add_argument( + "--clang_format_path", type=str, help="Path to clang-format executable.") parser.add_argument("--buildifier_path", type=str, help="Path to buildifier executable.") parser.add_argument("--buildozer_path", type=str, help="Path to buildozer executable.") parser.add_argument( @@ -322,55 +326,6 @@ def executable_by_others(self, executable): st = os.stat(os.path.expandvars(executable)) return bool(st.st_mode & stat.S_IXOTH) - # Check whether all needed external tools (clang-format, buildifier, buildozer) are - # available. - def check_tools(self): - error_messages = [] - clang_format_abs_path = self.look_path(self.config.clang_format_path) - - if clang_format_abs_path: - if not self.executable_by_others(clang_format_abs_path): - error_messages.append( - "command {} exists, but cannot be executed by other " - "users".format(self.config.clang_format_path)) - else: - error_messages.append( - "Command {} not found. If you have clang-format in version 12.x.x " - "installed, but the binary name is different or it's not available in " - "PATH, please use CLANG_FORMAT environment variable to specify the path. " - "Examples:\n" - " export CLANG_FORMAT=clang-format-14.0.0\n" - " export CLANG_FORMAT=/opt/bin/clang-format-14\n" - " export CLANG_FORMAT=/usr/local/opt/llvm@14/bin/clang-format".format( - self.config.clang_format_path)) - - def check_bazel_tool(name, path, var): - bazel_tool_abs_path = self.look_path(path) - if bazel_tool_abs_path: - if not self.executable_by_others(bazel_tool_abs_path): - error_messages.append( - "command {} exists, but cannot be executed by other " - "users".format(path)) - elif self.path_exists(path): - if not self.executable_by_others(path): - error_messages.append( - "command {} exists, but cannot be executed by other " - "users".format(path)) - else: - error_messages.append( - "Command {} not found. If you have {} installed, but the binary " - "name is different or it's not available in $GOPATH/bin, please use " - "{} environment variable to specify the path. Example:\n" - " export {}=`which {}`\n" - "If you don't have {} installed, you can install it by:\n" - " go install github.com/bazelbuild/buildtools/{}@latest".format( - path, name, var, var, name, name, name)) - - check_bazel_tool('buildifier', self.config.buildifier_path, 'BUILDIFIER_BIN') - check_bazel_tool('buildozer', self.config.buildozer_path, 'BUILDOZER_BIN') - - return error_messages - def check_namespace(self, file_path): for excluded_path in self.namespace_check_excluded_paths: if file_path.startswith(excluded_path): @@ -905,7 +860,6 @@ def fix_source_path(self, file_path): def check_source_path(self, file_path): error_messages = self.check_file_contents(file_path, self.check_source_line) - if not file_path.endswith(self.config.suffixes["proto"]): error_messages += self.check_namespace(file_path) command = ( @@ -914,8 +868,7 @@ def check_source_path(self, file_path): file_path)) error_messages += self.execute_command( command, "header_order.py check failed", file_path) - command = ("%s %s | diff %s -" % (self.config.clang_format_path, file_path, file_path)) - error_messages += self.execute_command(command, "clang-format check failed", file_path) + error_messages.extend(self.clang_format(file_path, check=True)) return error_messages # Example target outputs are: @@ -946,11 +899,19 @@ def fix_header_order(self, file_path): return ["header_order.py rewrite error: %s" % (file_path)] return [] - def clang_format(self, file_path): - command = "%s -i %s" % (self.config.clang_format_path, file_path) - if os.system(command) != 0: - return ["clang-format rewrite error: %s" % (file_path)] - return [] + def clang_format(self, file_path, check=False): + result = [] + command = ( + f"{self.config.clang_format_path} {file_path} | diff {file_path} -" + if check else f"{self.config.clang_format_path} -i {file_path}") + + if check: + result = self.execute_command(command, "clang-format check failed", file_path) + else: + if os.system(command) != 0: + result = [f"clang-format rewrite error: {file_path}"] + + return result def check_format(self, file_path, fail_on_diff=False): error_messages = [] diff --git a/tools/local_fix_format.sh b/tools/local_fix_format.sh index b392610cc48a..832d80ae8b0d 100755 --- a/tools/local_fix_format.sh +++ b/tools/local_fix_format.sh @@ -43,6 +43,10 @@ if [[ $# -gt 0 && "$1" == "-skip-bazel" ]]; then shift use_bazel=0 + CLANG_FORMAT_BIN="$(command -v clang-format)" || { + echo "Local clang-format not found, exiting" >&2 + exit 1 + } BUILDIFIER_BIN="$(command -v buildifier)" || { echo "Local buildifier not found, exiting" >&2 exit 1 @@ -72,7 +76,10 @@ format_some () { ./tools/spelling/check_spelling_pedantic.py fix "$@" else for arg in "$@"; do - ./tools/code_format/check_format.py --buildozer_path "$BUILDOZER_BIN" --buildifier_path "$BUILDIFIER_BIN" fix "$arg" + ./tools/code_format/check_format.py \ + --clang_format_path "$CLANG_FORMAT_BIN" \ + --buildozer_path "$BUILDOZER_BIN" \ + --buildifier_path "$BUILDIFIER_BIN" fix "$arg" ./tools/spelling/check_spelling_pedantic.py fix "$arg" done fi