Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .github/matchers/clang-tidy.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"problemMatcher": [
{
"owner": "clang-tidy",
"pattern": [
{
"regexp": "^(.*):(\\d+):(\\d+):\\s+(error|warning):\\s+(.*) \\[([a-z0-9,\\-]+)\\]\\s*$",
"file": 1,
"line": 2,
"column": 3,
"severity": 4,
"message": 5
}
]
}
]
}
18 changes: 18 additions & 0 deletions .github/matchers/gcc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"problemMatcher": [
{
"owner": "gcc",
"severity": "error",
"pattern": [
{
"regexp": "^(.*):(\\d+):(\\d+):\\s+(?:fatal\\s+)?(warning|error):\\s+(.*)$",
"file": 1,
"line": 2,
"column": 3,
"severity": 4,
"message": 5
}
]
}
]
}
18 changes: 18 additions & 0 deletions .github/matchers/shellcheck.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"problemMatcher": [
{
"owner": "shellcheck",
"severity": "error",
"pattern": [
{
"regexp": "^(.*):(\\d+):(\\d+):\\s+(?:fatal\\s+)?(warning|error):\\s+(.*)$",
"file": 1,
"line": 2,
"column": 3,
"severity": 4,
"message": 5
}
]
}
]
}
93 changes: 92 additions & 1 deletion .github/workflows/preliminary_checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,104 @@ jobs:
- uses: actions/setup-python@e797f83bcb11b83ae66e0230d6156d7c80228e7c # v6.0.0
- uses: pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd # v3.0.1

get-changes:
runs-on: ubuntu-latest
outputs:
run-clang-tidy: ${{ steps.changes.outputs.run_clang_tidy }}
changed-files: ${{ steps.changes.outputs.files}}
diff-range: ${{ steps.changes.outputs.range }}
steps:
- uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
with:
persist-credentials: false
fetch-depth: 0

- name: Get changed files
id: changes
env:
GH_TOKEN: ${{ github.token }}
BASE_REF: ${{ github.base_ref }}
HEAD_REF: ${{ github.head_ref }}
PR_OWNER: ${{ github.event.pull_request.head.repo.owner.login }}
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
Comment thread
assignUser marked this conversation as resolved.
run: |
merge_base_commit=$(gh api -q '.merge_base_commit.sha' \
/repos/facebookincubator/velox/compare/facebookincubator:$BASE_REF...$PR_OWNER:$HEAD_REF \
)

range="$merge_base_commit..$HEAD_SHA"
echo "range=$range" >> "$GITHUB_OUTPUT"

git diff --name-only $range > changed_files.txt

cpp_files='.+\.(cpp|h|hpp)$'

{
echo 'files<<EOF'
cat changed_files.txt
echo 'EOF'
} >> "$GITHUB_OUTPUT"

if grep -qE $cpp_files changed_files.txt; then
echo "run_clang_tidy=true" >> "$GITHUB_OUTPUT"
fi

clang-tidy:
needs: get-changes
if: ${{ needs.get-changes.outputs.run-clang-tidy == 'true' }}
runs-on: ubuntu-latest
container: ghcr.io/facebookincubator/velox-dev:adapters
steps:
- uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
with:
persist-credentials: false
fetch-depth: 0

- run: uv tool install clang-tidy==18.1.8

- name: Configure Maximal Build
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to configure the build?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy needs the compilation database ('compile_commands.json')

env:
CUDA_VERSION: "12.8"
CUDA_ARCHITECTURES: 70
CUDA_COMPILER: /usr/local/cuda-${CUDA_VERSION}/bin/nvcc
# Set compiler to GCC 12
CUDA_FLAGS: -ccbin /opt/rh/gcc-toolset-12/root/usr/bin
faiss_SOURCE: BUNDLED
EXTRA_CMAKE_FLAGS: >-
-DVELOX_ENABLE_BENCHMARKS=ON
-DVELOX_ENABLE_EXAMPLES=ON
-DVELOX_ENABLE_ARROW=ON
-DVELOX_ENABLE_GEO=ON
-DVELOX_ENABLE_PARQUET=ON
-DVELOX_ENABLE_HDFS=ON
-DVELOX_ENABLE_S3=ON
-DVELOX_ENABLE_GCS=ON
-DVELOX_ENABLE_ABFS=ON
-DVELOX_ENABLE_WAVE=ON
-DVELOX_MONO_LIBRARY=ON
-DVELOX_BUILD_SHARED=ON
-DVELOX_ENABLE_CUDF=ON
-DVELOX_ENABLE_FAISS=ON
-DVELOX_ENABLE_REMOTE_FUNCTIONS=ON
run: |
make cmake BUILD_DIR=release BUILD_TYPE=Release

- name: Run clang-tidy
env:
FILES: ${{ needs.get-changes.outputs.changed-files }}
RANGE: ${{ needs.get-changes.outputs.diff-range }}
run: |
git config --global --add safe.directory /__w/velox/velox

python ./scripts/checks/run-clang-tidy.py --commit $RANGE $FILES

title-check:
name: PR Title Format
runs-on: ubuntu-latest
steps:
- shell: python
env:
title: '${{ github.event.pull_request.title }}'
title: "${{ github.event.pull_request.title }}"
run: |
import re
import os
Expand Down
10 changes: 7 additions & 3 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,14 @@ repos:
hooks:
- id: clang-tidy
name: clang-tidy
description: Run clang-tidy on C/C++ files
description: >-
Run clang-tidy on C/C++ files, requires 'compile_commands.json'
to be available in the repo root (e.g. symlinked) or BUILD_PATH
set to it's path e.g. _build/release.
stages:
- manual # Needs compile_commands.json
entry: clang-tidy
- pre-commit
entry: ./scripts/checks/run-clang-tidy.py
args: [--commit, HEAD]
language: python
types_or: [c++, c]
additional_dependencies: [clang-tidy==18.1.8]
Expand Down
11 changes: 9 additions & 2 deletions scripts/checks/license-header.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def wrapper_hash(header, args):
"*.cmake": attrdict({"wrapper": wrapper_hash, "hashbang": False}),
"*.cpp": attrdict({"wrapper": wrapper_chpp, "hashbang": False}),
"*.hpp": attrdict({"wrapper": wrapper_chpp, "hashbang": False}),
"*.dockfile": attrdict({"wrapper": wrapper_hash, "hashbang": False}),
"*.dockerfile": attrdict({"wrapper": wrapper_hash, "hashbang": False}),
"*.h": attrdict({"wrapper": wrapper_chpp, "hashbang": False}),
"*.inc": attrdict({"wrapper": wrapper_chpp, "hashbang": False}),
"*.java": attrdict({"wrapper": wrapper_chpp, "hashbang": False}),
Expand All @@ -110,6 +110,7 @@ def wrapper_hash(header, args):
"*.thrift": attrdict({"wrapper": wrapper_chpp, "hashbang": False}),
"*.txt": attrdict({"wrapper": wrapper_hash, "hashbang": True}),
"*.yml": attrdict({"wrapper": wrapper_hash, "hashbang": False}),
"*.yaml": attrdict({"wrapper": wrapper_hash, "hashbang": False}),
"*.cu": attrdict({"wrapper": wrapper_chpp, "hashbang": False}),
"*.cuh": attrdict({"wrapper": wrapper_chpp, "hashbang": False}),
"*.clcpp": attrdict({"wrapper": wrapper_chpp, "hashbang": False}),
Expand Down Expand Up @@ -228,7 +229,13 @@ def main():
# If removing the header text, zero it out there.
header_comment = ""

message(log_to, "Fix : " + filepath)
if os.environ.get("GITHUB_ACTIONS"):
Copy link
Copy Markdown
Collaborator

@czentgr czentgr Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize this but this change causes build failures in PrestoC++. Namely, it re-generates the protocol in github and this adds the new error message to the output causing compile errors.
We do a regenerate in the CI to try and ensure java-cpp protocol compatibility.

We call "remove" explicitly.

print(
f"::error file={filepath},line=1,endLine=1,title=Missing license header::"
+ "Please add the properly commented license header."
)
else:
message(log_to, "Fix : " + filepath)

if args.check:
fail = True
Expand Down
Loading
Loading