This is an automated email from the ASF dual-hosted git repository.
zhouyuan pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gluten.git
The following commit(s) were added to refs/heads/main by this push:
new b58e654ae1 [VL] Run clang-tidy in the native build job and narrow its
check list (#12729)
b58e654ae1 is described below
commit b58e654ae1967ef2441cce4d737856b2b801c572
Author: Philo He <[email protected]>
AuthorDate: Wed Aug 12 16:14:51 2026 +0800
[VL] Run clang-tidy in the native build job and narrow its check list
(#12729)
* Initial
* Fake change
* Fix
* Revert "Fake change"
This reverts commit a2d750534ceb5ec7e48f85b0d9acd05e7df9f285.
* Fix comment
---
.github/workflows/cpp_clang_tidy.yml | 83 -----------------------------
.github/workflows/velox_backend_x86.yml | 34 ++++++++++++
cpp/.clang-tidy | 24 +++++++++
dev/docker/Dockerfile.centos9-dynamic-build | 2 +
dev/run-clang-tidy.py | 59 ++++----------------
5 files changed, 71 insertions(+), 131 deletions(-)
diff --git a/.github/workflows/cpp_clang_tidy.yml
b/.github/workflows/cpp_clang_tidy.yml
deleted file mode 100644
index 759173f6a8..0000000000
--- a/.github/workflows/cpp_clang_tidy.yml
+++ /dev/null
@@ -1,83 +0,0 @@
-# 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:
- workflow_run:
- workflows: ["Velox Backend (x86)"]
- types:
- - completed
-
-env:
- ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true
-
-concurrency:
- group: clang-tidy-${{ github.event.workflow_run.id }}
- cancel-in-progress: true
-
-jobs:
- clang-tidy-check:
- if: ${{ github.event.workflow_run.conclusion == 'success' }}
- runs-on: ubuntu-22.04
- container: apache/gluten:centos-8-jdk8
- steps:
- - uses: actions/checkout@v7
- with:
- ref: ${{ github.event.workflow_run.head_sha }}
- fetch-depth: 0
- # Workspaces bind-mounted into containers are often owned by a different
UID than the
- # container user; Git 2.35+ refuses operations unless the directory is
marked safe.
- - name: Configure Git safe directory (container)
- run: git config --global --add safe.directory "$GITHUB_WORKSPACE"
- - name: Detect C++ file changes
- id: filter
- run: |
- HEAD_SHA="${{ github.event.workflow_run.head_sha }}"
- git remote -v && git fetch origin main:main
- # Finds the common ancestor for PR targeting the main branch only.
- # We may need to update the base branch if PR is not targeting the
main branch.
- BASE_SHA=$(git merge-base main $HEAD_SHA)
- echo "BASE_SHA: $BASE_SHA"
- echo "HEAD_SHA: $HEAD_SHA"
- if git diff --name-only $BASE_SHA $HEAD_SHA | grep -E
'^cpp/.*\.(cc|h)$'; then
- echo "cpp_changed=true" >> $GITHUB_OUTPUT
- else
- echo "cpp_changed=false" >> $GITHUB_OUTPUT
- fi
- - name: Download artifact from in-progress workflow
- if: steps.filter.outputs.cpp_changed == 'true'
- uses: dawidd6/action-download-artifact@v6
- with:
- github_token: ${{secrets.GITHUB_TOKEN}}
- workflow: .github/workflows/velox_backend_x86.yml
- name: velox-native-lib-centos-7-${{
github.event.workflow_run.head_sha }}
- commit: ${{ github.event.workflow_run.head_sha }}
- workflow_conclusion: ""
- check_artifacts: true
- search_artifacts: true
- if_no_artifact_found: fail
- - name: Check Clang Tidy
- if: steps.filter.outputs.cpp_changed == 'true'
- run: |
- pip3 install regex
- yum install glibc-devel glibc-headers
- cd $GITHUB_WORKSPACE/
- yum install clang llvm llvm-devel -y
- dnf install clang-tools-extra -y
- clang-tidy --version
- sed -i "s|/work|$(pwd)|g" cpp/build/compile_commands.json
- python3 dev/check.py tidy commit --fix
-
diff --git a/.github/workflows/velox_backend_x86.yml
b/.github/workflows/velox_backend_x86.yml
index 577fcc3a1a..981adf8c19 100644
--- a/.github/workflows/velox_backend_x86.yml
+++ b/.github/workflows/velox_backend_x86.yml
@@ -1026,6 +1026,10 @@ jobs:
container: apache/gluten:centos-9-jdk8
steps:
- uses: actions/checkout@v7
+ with:
+ # 'check.py tidy commit' diffs HEAD^..HEAD, which a depth-1 clone
lacks: the
+ # diff comes back empty and the check passes without inspecting
anything.
+ fetch-depth: 2
- name: Install Stash restore dependencies # the stash action needs gh
and jq, absent from this container
run: |
case "$(uname -m)" in
@@ -1057,6 +1061,36 @@ jobs:
df -a
bash dev/ci-velox-buildshared-centos-9.sh
ccache -s
+ # Runs in this job because clang-tidy needs the compilation database and
header tree
+ # the build just produced; only 'cpp/build/' is uploaded, so shipping
+ # 'compile_commands.json' to another job leaves its '-I' paths dangling.
+ - name: Check Clang Tidy
+ # Advisory until observed green: clang-tidy re-parses a database
written by
+ # gcc-toolset-12, so toolchain-level diagnostics are still possible.
+ continue-on-error: true
+ env:
+ # Make the GCC-generated database clang-safe: point clang at the
libstdc++ the
+ # build used, and mirror the '-Wno-' flags cpp/CMakeLists.txt adds
on its Clang
+ # branch, which the database lacks while still carrying '-Werror'.
+ CLANG_TIDY_EXTRA_ARGS: >-
+ --extra-arg=--gcc-toolchain=/opt/rh/gcc-toolset-12/root/usr
+ --extra-arg=-Wno-error
+ --extra-arg=-Wno-unknown-warning-option
+ --extra-arg=-Wno-implicit-int-float-conversion
+ --extra-arg=-Wno-nullability-completeness
+ --extra-arg=-Wno-mismatched-tags
+ run: |
+ # check.py shells out to git; the workspace is owned by another UID.
+ git config --global --add safe.directory "$GITHUB_WORKSPACE"
+ # Skip the toolchain install when no C++ changed; check.py itself
already no-ops.
+ if ! git diff --name-only HEAD^ HEAD | grep -qE
'^cpp/.*\.(cc|cpp|h)$'; then
+ echo "No C++ changes in this pull request; skipping clang-tidy."
+ exit 0
+ fi
+ dnf install -y clang-tools-extra # TODO remove after image update
+ pip3 install regex
+ clang-tidy --version # log the version so finding changes trace to a
clang bump
+ python3 dev/check.py tidy commit
- name: Run CPP unit test
run: |
cd ./cpp/build && ctest -V
diff --git a/cpp/.clang-tidy b/cpp/.clang-tidy
index c77c0d304a..32d43dd814 100644
--- a/cpp/.clang-tidy
+++ b/cpp/.clang-tidy
@@ -1,5 +1,29 @@
+# Single source of truth for the check list: a CLI `--checks` would override
this key,
+# so `dev/run-clang-tidy.py` does not pass one.
+#
+# Report only on the changed translation unit, never on included headers. The
check is
+# diff-scoped, so header diagnostics would blame a pull request for
pre-existing findings
+# in every header it happens to include, and generated protobuf headers under
`cpp/build`
+# dominate the output. '^$' matches no file name; an empty value does not
filter.
+#
+# 'bugprone-throwing-static-initialization' is off because it flags the
project-wide
+# 'const std::string kFoo = "..."' idiom (~160 sites), rejecting new code
written
+# consistently with its neighbours. Re-enable after migrating to 'constexpr
string_view'.
+HeaderFilterRegex: '^$'
+#
+# Allow-list rather than `*` minus exclusions, so a clang upgrade cannot
silently add
+# checks and fail unrelated PRs.
Checks: >
-*,
+ bugprone-*,
+ -bugprone-branch-clone,
+ -bugprone-easily-swappable-parameters,
+ -bugprone-implicit-widening-of-multiplication-result,
+ -bugprone-narrowing-conversions,
+ -bugprone-throwing-static-initialization,
+ clang-analyzer-core.*,
+ performance-*,
+ -performance-avoid-endl,
readability-identifier-naming,
CheckOptions:
diff --git a/dev/docker/Dockerfile.centos9-dynamic-build
b/dev/docker/Dockerfile.centos9-dynamic-build
index 0d7246b4a2..5991236203 100644
--- a/dev/docker/Dockerfile.centos9-dynamic-build
+++ b/dev/docker/Dockerfile.centos9-dynamic-build
@@ -29,6 +29,8 @@ ENV LC_ALL=C.UTF-8
RUN set -ex; \
yum update -y && yum install -y epel-release sudo dnf && yum install -y
ccache; \
dnf install -y --setopt=install_weak_deps=False gcc-toolset-12
gcc-toolset-13; \
+ # clang-tidy, installed here so its version is pinned to the image, not to
each CI run.
+ dnf install -y --setopt=install_weak_deps=False clang-tools-extra; \
echo "check_certificate = off" >> ~/.wgetrc; \
yum install -y java-${JAVA_VERSION}-openjdk-devel patch wget git perl; \
mirror_host="https://www.apache.org/dyn/closer.lua";; \
diff --git a/dev/run-clang-tidy.py b/dev/run-clang-tidy.py
index e1298106b7..f3a94928e4 100755
--- a/dev/run-clang-tidy.py
+++ b/dev/run-clang-tidy.py
@@ -18,6 +18,7 @@ from __future__ import print_function
import argparse
import os
import regex
+import shlex
import subprocess
import sys
@@ -30,44 +31,6 @@ class string(str):
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"]
@@ -100,13 +63,6 @@ def run(command, compressed=False, **kwargs):
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
@@ -128,12 +84,19 @@ def tidy(args):
fix = "--fix" if args.fix == "fix" else ""
files = args.files
+ # Checks come from 'cpp/.clang-tidy'; passing '--checks' here would
override it.
+ # CLANG_TIDY_EXTRA_ARGS forwards toolchain details the compilation
database omits.
+ # The command below runs through a shell, so split and re-quote the value
instead of
+ # interpolating it raw.
+ extra_args = " ".join(
+ shlex.quote(arg) for arg in
shlex.split(os.environ.get("CLANG_TIDY_EXTRA_ARGS", ""))
+ )
+
cmd = (
- "xargs clang-tidy -p={build} --format-style=file "
- "--checks='{checks}' {fix} --quiet".format(
+ "xargs clang-tidy -p={build} --format-style=file {fix} {extra}
--quiet".format(
build=build_dir,
- checks=CODE_CHECKS,
fix=fix,
+ extra=extra_args,
)
)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
