From c40ae7a7edddaec197ac88f6791fe08debc984b8 Mon Sep 17 00:00:00 2001 From: xinghuayu007 <1450306854@qq.com> Date: Wed, 5 Nov 2025 03:23:29 +0000 Subject: [PATCH 1/4] [GLUTEN-11027] Use clang-tidy to check cpp files --- cpp/run-clang-tidy.py | 218 +++++++++++++++++++++++++++++++++++++++ dev/builddeps-veloxbe.sh | 1 + 2 files changed, 219 insertions(+) create mode 100644 cpp/run-clang-tidy.py diff --git a/cpp/run-clang-tidy.py b/cpp/run-clang-tidy.py new file mode 100644 index 00000000000..9bf3fa6fa43 --- /dev/null +++ b/cpp/run-clang-tidy.py @@ -0,0 +1,218 @@ +#!/usr/bin/env python3 +# 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. + +from __future__ import print_function +import argparse +import os +import regex +import subprocess +import sys + + +class string(str): + def extract(self, rexp): + return regex.match(rexp, self).group(1) + + def json(self): + return json.loads(self, object_hook=attrdict) + + +CODE_CHECKS = """* + -abseil-* + -android-* + -cert-err58-cpp + -clang-analyzer-osx-* + -cppcoreguidelines-avoid-c-arrays + -cppcoreguidelines-avoid-magic-numbers + -cppcoreguidelines-pro-bounds-array-to-pointer-decay + -cppcoreguidelines-pro-bounds-pointer-arithmetic + -cppcoreguidelines-pro-type-reinterpret-cast + -cppcoreguidelines-pro-type-vararg + -fuchsia-* + -google-* + -hicpp-avoid-c-arrays + -hicpp-deprecated-headers + -hicpp-no-array-decay + -hicpp-use-equals-default + -hicpp-vararg + -llvmlibc-* + -llvm-header-guard + -llvm-include-order + -mpi-* + -misc-non-private-member-variables-in-classes + -misc-no-recursion + -misc-unused-parameters + -modernize-avoid-c-arrays + -modernize-deprecated-headers + -modernize-use-nodiscard + -modernize-use-trailing-return-type + -objc-* + -openmp-* + -readability-avoid-const-params-in-decls + -readability-convert-member-functions-to-static + -readability-magic-numbers + -zircon-* +""" + + +def run(command, compressed=False, **kwargs): + if "input" in kwargs: + input = kwargs["input"] + + if type(input) == list: + input = "\n".join(input) + "\n" + + kwargs["input"] = input.encode("utf-8") + reply = subprocess.run( + command, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, **kwargs + ) + + if compressed: + stdout = gzip.decompress(reply.stdout) + else: + stdout = reply.stdout + + stdout = ( + string(stdout.decode("utf-8", errors="ignore").strip()) + if stdout is not None + else "" + ) + stderr = ( + string(reply.stderr.decode("utf-8").strip()) if reply.stderr is not None else "" + ) + + if stderr != "": + print(stderr, file=sys.stderr) + + return reply.returncode, stdout, stderr + + +def check_list(check_string): + return ",".join([c.strip() for c in check_string.strip().splitlines() if c.strip()]) + + +CODE_CHECKS = check_list(CODE_CHECKS) + + +def check_output_has_warnings(output): + if not output: + return False + return bool(regex.search(r"(?i)\b(warning|error):", output)) + + +def get_modified_cpp_files(commit="HEAD"): + cmd = f"git diff-tree --no-commit-id --name-status -r {commit}" + status, stdout, stderr = run(cmd) + if status != 0: + print("Error running git diff-tree:", stderr, file=sys.stderr) + return [] + + files = [] + for line in stdout.splitlines(): + parts = line.strip().split("\t") + if len(parts) < 2: + continue + change_type, path = parts[0], parts[1] + if change_type in ("A", "M") and path.endswith((".cpp", ".cc", ".h")): + files.append(path) + + return files + + +def ensure_compile_db(build_dir): + path = os.path.join(build_dir, "compile_commands.json") + return os.path.exists(path) + + +def tidy(args): + clang_tidy_bin = args.clang_tidy or "clang-tidy" + build_dir = args.build_dir or "cpp/build" + project_root = args.project_root or "/app/dps-ssc-core-gluten/incubator-gluten" + + files = get_modified_cpp_files() + if not files: + print("No modified .cc/.cpp/.h files in the latest commit. Nothing to do.") + return 0 + + if not ensure_compile_db(build_dir): + print("No compile_commands.json found in '{}'.".format(build_dir)) + return 1 + + fix = "--fix" if args.fix == "fix" else "" + header_filter = "^(?!.*build).*".format(regex.escape(project_root)) + + cmd = ( + "xargs {clang} -p={build}/releases --format-style=file --header-filter='{hf}' " + "--checks='{checks}' {fix} --quiet".format( + clang=clang_tidy_bin, + build=build_dir, + hf=header_filter, + checks=CODE_CHECKS, + fix=fix, + ) + ) + + print("Running clang-tidy on {} file(s)...".format(len(files))) + status, stdout, stderr = run(cmd, input=files) + + combined_output = "" + if stdout: + combined_output += stdout + "\n" + if stderr: + combined_output += stderr + + if check_output_has_warnings(combined_output): + print("clang-tidy found warnings/errors.") + # Print a reasonably sized portion to avoid flooding logs; print all if small + if len(combined_output) > 20000: + print(combined_output[:20000]) + print("... (output truncated)") + else: + print(combined_output) + return 1 + + # Also check the return code (status) - clang-tidy may return non-zero for internal errors + if status != 0: + print("clang-tidy returned non-zero exit code:", status) + if combined_output: + print(combined_output) + return 1 + + print("clang-tidy completed successfully (no warnings).") + return 0 + + +def parse_args(): + parser = argparse.ArgumentParser( + description="Run clang-tidy with project settings (compatible with older Python versions)" + ) + parser.add_argument("--fix", help="Apply automatic fixes (use 'fix' to enable)") + parser.add_argument("--clang-tidy", help="Path to clang-tidy binary") + parser.add_argument( + "--build-dir", help="Path to build directory with compile_commands.json" + ) + parser.add_argument( + "--project-root", help="Root path of the source project (for header-filter)" + ) + return parser.parse_args() + + +def main(): + return tidy(parse_args()) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/dev/builddeps-veloxbe.sh b/dev/builddeps-veloxbe.sh index 10ca369e147..39aa412d25d 100755 --- a/dev/builddeps-veloxbe.sh +++ b/dev/builddeps-veloxbe.sh @@ -243,6 +243,7 @@ function build_gluten_cpp { -DENABLE_HDFS=$ENABLE_HDFS \ -DENABLE_ABFS=$ENABLE_ABFS \ -DENABLE_GPU=$ENABLE_GPU \ + -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \ -DENABLE_ENHANCED_FEATURES=$ENABLE_ENHANCED_FEATURES" if [ $OS == 'Darwin' ]; then From fce6f47729def426d033d0bcfea0df73fe1f901b Mon Sep 17 00:00:00 2001 From: xinghuayu007 <1450306854@qq.com> Date: Mon, 10 Nov 2025 09:21:32 +0000 Subject: [PATCH 2/4] put clang-tidy check into ci/cd --- .github/workflows/cpp_clang_tidy.yml | 100 +++++++++++++++++++++++++++ cpp/core/memory/ArrowMemoryPool.h | 1 + dev/check.py | 4 +- {cpp => dev}/run-clang-tidy.py | 50 +++----------- 4 files changed, 113 insertions(+), 42 deletions(-) create mode 100644 .github/workflows/cpp_clang_tidy.yml rename {cpp => dev}/run-clang-tidy.py (75%) mode change 100644 => 100755 diff --git a/.github/workflows/cpp_clang_tidy.yml b/.github/workflows/cpp_clang_tidy.yml new file mode 100644 index 00000000000..d90fb77cf7c --- /dev/null +++ b/.github/workflows/cpp_clang_tidy.yml @@ -0,0 +1,100 @@ +# 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. + +name: Clang Tidy Check + +on: + pull_request: + paths: + - '.github/workflows/cpp_clang_tidy.yml' + - 'cpp/**' + +env: + ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true + MVN_CMD: 'mvn -ntp' + WGET_CMD: 'wget -nv' + SETUP: 'source .github/workflows/util/setup-helper.sh' + CCACHE_DIR: "${{ github.workspace }}/.ccache" + # spark.sql.ansi.enabled defaults to false. + SPARK_ANSI_SQL_MODE: false + +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: true + +jobs: + build-native-lib-centos-7: + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@v4 + - name: Get Ccache + uses: actions/cache/restore@v4 + with: + path: '${{ env.CCACHE_DIR }}' + key: ccache-centos7-release-default-${{github.sha}} + restore-keys: | + ccache-centos7-release-default + - name: Build Gluten native libraries + run: | + docker pull apache/gluten:vcpkg-centos-7 + docker run -v $GITHUB_WORKSPACE:/work -w /work apache/gluten:vcpkg-centos-7 bash -c " + set -e + yum install tzdata -y + df -a + cd /work + export CCACHE_DIR=/work/.ccache + mkdir -p /work/.ccache + bash dev/ci-velox-buildstatic-centos-7.sh + ls ./cpp/build/ + mv ./cpp/build/compile_commands.json ./cpp/build/releases/ + ccache -s + mkdir -p /work/.m2/repository/org/apache/arrow/ + cp -r /root/.m2/repository/org/apache/arrow/* /work/.m2/repository/org/apache/arrow/ + " + + - name: "Save ccache" + uses: actions/cache/save@v4 + id: ccache + with: + path: '${{ env.CCACHE_DIR }}' + key: ccache-centos7-release-default-${{github.sha}} + - uses: actions/upload-artifact@v4 + with: + name: velox-native-lib-centos-7-${{github.sha}} + path: ./cpp/build/ + if-no-files-found: error + - uses: actions/upload-artifact@v4 + with: + name: arrow-jars-centos-7-${{github.sha}} + path: .m2/repository/org/apache/arrow/ + if-no-files-found: error + + clang-tidy-check: + needs: build-native-lib-centos-7 + runs-on: ubuntu-22.04 + container: apache/gluten:centos-8-jdk8 + steps: + - uses: actions/checkout@v4 + with: fetch-depth: 0 + - uses: actions/download-artifact@v4 + with: + name: velox-native-lib-centos-7-${{github.sha}} + path: ./cpp/build/releases + - name: Check Clang Tidy + run: | + pip3 install regex + cd $GITHUB_WORKSPACE/ + python3 dev/check.py tidy commit --fix + diff --git a/cpp/core/memory/ArrowMemoryPool.h b/cpp/core/memory/ArrowMemoryPool.h index 9f3592ceec1..de089bd7db5 100644 --- a/cpp/core/memory/ArrowMemoryPool.h +++ b/cpp/core/memory/ArrowMemoryPool.h @@ -54,6 +54,7 @@ class ArrowMemoryPool final : public arrow::MemoryPool { private: std::unique_ptr allocator_; + ArrowMemoryPoolReleaser releaser_; }; diff --git a/dev/check.py b/dev/check.py index e237ae250a0..f1ec051c90d 100755 --- a/dev/check.py +++ b/dev/check.py @@ -168,7 +168,7 @@ def header_command(commit, files, fix): def tidy_command(commit, files, fix): - files = [file for file in files if regex.match(r".*\.cpp$", file)] + files = [file for file in files if regex.match(r".*\.(cc|cpp|h)$", file)] if not files: return 0 @@ -177,7 +177,7 @@ def tidy_command(commit, files, fix): fix = "--fix" if fix == "fix" else "" status, stdout, stderr = util.run( - f"{SCRIPTS}/run-clang-tidy.py {commit} {fix} -", input=files + f"{SCRIPTS}/run-clang-tidy.py {commit} {fix} ", input=files ) if stdout != "": diff --git a/cpp/run-clang-tidy.py b/dev/run-clang-tidy.py old mode 100644 new mode 100755 similarity index 75% rename from cpp/run-clang-tidy.py rename to dev/run-clang-tidy.py index 9bf3fa6fa43..6a3ff9deeb1 --- a/cpp/run-clang-tidy.py +++ b/dev/run-clang-tidy.py @@ -113,53 +113,25 @@ def check_output_has_warnings(output): return bool(regex.search(r"(?i)\b(warning|error):", output)) -def get_modified_cpp_files(commit="HEAD"): - cmd = f"git diff-tree --no-commit-id --name-status -r {commit}" - status, stdout, stderr = run(cmd) - if status != 0: - print("Error running git diff-tree:", stderr, file=sys.stderr) - return [] - - files = [] - for line in stdout.splitlines(): - parts = line.strip().split("\t") - if len(parts) < 2: - continue - change_type, path = parts[0], parts[1] - if change_type in ("A", "M") and path.endswith((".cpp", ".cc", ".h")): - files.append(path) - - return files - - def ensure_compile_db(build_dir): path = os.path.join(build_dir, "compile_commands.json") return os.path.exists(path) def tidy(args): - clang_tidy_bin = args.clang_tidy or "clang-tidy" - build_dir = args.build_dir or "cpp/build" - project_root = args.project_root or "/app/dps-ssc-core-gluten/incubator-gluten" - - files = get_modified_cpp_files() - if not files: - print("No modified .cc/.cpp/.h files in the latest commit. Nothing to do.") - return 0 + build_dir = "cpp/build/releases" if not ensure_compile_db(build_dir): print("No compile_commands.json found in '{}'.".format(build_dir)) return 1 fix = "--fix" if args.fix == "fix" else "" - header_filter = "^(?!.*build).*".format(regex.escape(project_root)) + files = args.files cmd = ( - "xargs {clang} -p={build}/releases --format-style=file --header-filter='{hf}' " + "xargs clang-tidy -p={build}/releases --format-style=file " "--checks='{checks}' {fix} --quiet".format( - clang=clang_tidy_bin, build=build_dir, - hf=header_filter, checks=CODE_CHECKS, fix=fix, ) @@ -199,19 +171,17 @@ def parse_args(): parser = argparse.ArgumentParser( description="Run clang-tidy with project settings (compatible with older Python versions)" ) - parser.add_argument("--fix", help="Apply automatic fixes (use 'fix' to enable)") - parser.add_argument("--clang-tidy", help="Path to clang-tidy binary") - parser.add_argument( - "--build-dir", help="Path to build directory with compile_commands.json" - ) - parser.add_argument( - "--project-root", help="Root path of the source project (for header-filter)" - ) + parser.add_argument("--fix", action="store_const", default="show", const="fix") + parser.add_argument("--commit", default="", help="Commit for check") + parser.add_argument("files", metavar="FILES", nargs="*", help="files to process") return parser.parse_args() def main(): - return tidy(parse_args()) + args = parse_args() + if not args.files: + args.files = [line.strip() for line in sys.stdin if line.strip()] + return tidy(args) if __name__ == "__main__": From 532a59d74d63e6919a09970d453479cfb71bd140 Mon Sep 17 00:00:00 2001 From: xinghuayu007 <1450306854@qq.com> Date: Wed, 19 Nov 2025 03:33:43 +0000 Subject: [PATCH 3/4] update --- .github/workflows/cpp_clang_tidy.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/cpp_clang_tidy.yml b/.github/workflows/cpp_clang_tidy.yml index d90fb77cf7c..af18e4f9fa1 100644 --- a/.github/workflows/cpp_clang_tidy.yml +++ b/.github/workflows/cpp_clang_tidy.yml @@ -59,7 +59,6 @@ jobs: bash dev/ci-velox-buildstatic-centos-7.sh ls ./cpp/build/ mv ./cpp/build/compile_commands.json ./cpp/build/releases/ - ccache -s mkdir -p /work/.m2/repository/org/apache/arrow/ cp -r /root/.m2/repository/org/apache/arrow/* /work/.m2/repository/org/apache/arrow/ " From c7c6f0858e83a60b8041a60f72030dd69e594842 Mon Sep 17 00:00:00 2001 From: xinghuayu007 <1450306854@qq.com> Date: Wed, 19 Nov 2025 03:39:21 +0000 Subject: [PATCH 4/4] update --- cpp/core/memory/ArrowMemoryPool.h | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/core/memory/ArrowMemoryPool.h b/cpp/core/memory/ArrowMemoryPool.h index de089bd7db5..9f3592ceec1 100644 --- a/cpp/core/memory/ArrowMemoryPool.h +++ b/cpp/core/memory/ArrowMemoryPool.h @@ -54,7 +54,6 @@ class ArrowMemoryPool final : public arrow::MemoryPool { private: std::unique_ptr allocator_; - ArrowMemoryPoolReleaser releaser_; };