Skip to content

ci: Cherry-pick prestodb/presto#13361 to use pre-commit for quality checks; Fix compilation caching by using cache on runner. #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kirkrodrigues merged 19 commits into from
Jun 19, 2025
Merged

ci: Cherry-pick prestodb/presto#13361 to use pre-commit for quality checks; Fix compilation caching by using cache on runner. #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
17a3ee4
misc: Use pre-commit for quality checks (#13361)
assignUser Jun 10, 2025
40afa57
Fix pre-commit check
Jun 18, 2025
e972b81
Add DIY cache management
Jun 18, 2025
f7a5f79
Fix
Jun 18, 2025
7ee96d9
Remove the DIY cache manage ment for prelimiary check because it is n…
Jun 18, 2025
b9f5816
Add full path
Jun 18, 2025
13c0794
Fix precommit check and use minimal build to test
Jun 18, 2025
3f445dd
Fix the path
Jun 18, 2025
04c6191
Fix
Jun 18, 2025
8df7c10
Fix
Jun 18, 2025
f430fd6
Test ccache
Jun 18, 2025
a8d72a0
Fix
Jun 18, 2025
0659d3e
Use ccache
Jun 18, 2025
a1ae373
exp
Jun 18, 2025
49ee963
Fix
Jun 18, 2025
175e131
Test finished
Jun 18, 2025
16e8eaf
Fix
Jun 18, 2025
d99245f
Apply comments
Jun 19, 2025
8d95db1
Fix
Jun 19, 2025
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
96 changes: 96 additions & 0 deletions .clang-tidy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
Checks: >
*,
-abseil-*,
-android-*,
-cert-err58-cpp,
-cert-err58-cpp,
-clang-analyzer-osx-*,
Comment on lines +3 to +7

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove duplicate check entries

The disabled patterns list contains duplicates (e.g., cert-err58-cpp twice). Deduplicate entries to streamline maintenance.

🤖 Prompt for AI Agents 
In the .clang-tidy file between lines 3 and 7, there are duplicate entries in
the disabled checks list, specifically the `cert-err58-cpp` pattern appears
twice. Remove the duplicate entries so that each disabled check pattern appears
only once to keep the configuration clean and maintainable.

-cppcoreguidelines-avoid-c-arrays,
-cppcoreguidelines-avoid-goto,
-cppcoreguidelines-avoid-magic-numbers,
-cppcoreguidelines-avoid-non-const-global-variables,
-cppcoreguidelines-owning-memory,
-cppcoreguidelines-pro-bounds-array-to-pointer-decay,
-cppcoreguidelines-pro-bounds-pointer-arithmetic,
-cppcoreguidelines-pro-type-reinterpret-cast,
Comment on lines +1 to +15

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Switch to literal block for Checks list

Using YAML folded style (>) merges lines, which can break comma-separated patterns. Prefer a literal block (|) to preserve each entry:

-Checks: >
+Checks: |
   *
   -abseil-*
   -android-*
   ...

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In the .clang-tidy file from lines 1 to 15, the Checks list uses YAML folded
style (>) which merges lines and can break comma-separated patterns. Change the
folded style indicator (>) to a literal block indicator (|) to preserve each
entry on its own line and ensure the list is parsed correctly.

-cppcoreguidelines-pro-type-vararg,
-cppcoreguidelines-pro-type-vararg,
-cppcoreguidelines-special-member-functions,
-fuchsia-*,
-google-*,
-hicpp-avoid-c-arrays,
-hicpp-avoid-goto,
-hicpp-deprecated-headers,
-hicpp-no-array-decay,
-hicpp-special-member-functions,
-hicpp-use-equals-default,
-hicpp-vararg,
-hicpp-vararg,
-llvm-header-guard,
-llvm-include-order,
-llvmlibc-*,
-misc-no-recursion,
-misc-no-recursion,
-misc-non-private-member-variables-in-classes,
-misc-unused-parameters,
-modernize-avoid-c-arrays,
-modernize-deprecated-headers,
-modernize-use-nodiscard,
-modernize-use-trailing-return-type,
-mpi-*,
-objc-*,
-openmp-*,
-readability-avoid-const-params-in-decls,
-readability-convert-member-functions-to-static,
-readability-implicit-bool-conversion,
-readability-magic-numbers,
-zircon-*,

HeaderFilterRegex: '.*'

WarningsAsErrors: ''

Comment on lines +49 to +52

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Restrict HeaderFilterRegex for performance

HeaderFilterRegex: '.*' makes clang-tidy run on all headers, including external deps. Narrow this to project directories (e.g., '(src|include)/.*') to speed up analysis.

🤖 Prompt for AI Agents 
In the .clang-tidy file around lines 49 to 52, the HeaderFilterRegex is set to
'.*', causing clang-tidy to analyze all headers including external dependencies,
which slows down performance. Change the HeaderFilterRegex value to restrict it
to your project directories, for example '(src|include)/.*', to limit analysis
to relevant headers and improve speed.

CheckOptions:
# Naming conventions as explicitly stated in CODING_STYLE.md
- key: readability-identifier-naming.ClassCase
value: CamelCase
- key: readability-identifier-naming.StructCase
value: CamelCase
- key: readability-identifier-naming.EnumCase
value: CamelCase
- key: readability-identifier-naming.TypeAliasCase
value: CamelCase
- key: readability-identifier-naming.TypeTemplateParameterCase
value: CamelCase
- key: readability-identifier-naming.FunctionCase
value: camelBack
- key: readability-identifier-naming.VariableCase
value: camelBack
- key: readability-identifier-naming.ParameterCase
value: camelBack
- key: readability-identifier-naming.PrivateMemberCase
value: camelBack
- key: readability-identifier-naming.PrivateMemberSuffix
value: _
- key: readability-identifier-naming.ProtectedMemberCase
value: camelBack
- key: readability-identifier-naming.ProtectedMemberSuffix
value: _
- key: readability-identifier-naming.MacroDefinitionCase
value: UPPER_CASE
- key: readability-identifier-naming.NamespaceCase
value: lower_case
- key: readability-identifier-naming.StaticConstantPrefix
value: k
- key: readability-identifier-naming.EnumConstantCase
value: CamelCase
- key: readability-identifier-naming.EnumConstantPrefix
value: k

# Use nullptr instead of NULL or 0
- key: modernize-use-nullptr.NullMacros
value: 'NULL'

# Prefer enum class over enum
- key: modernize-use-using.IgnoreUsingStdAllocator
value: 1
29 changes: 14 additions & 15 deletions .cmake-format.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,46 +24,46 @@ format:
separate_ctrl_name_with_space: false
separate_fn_name_with_space: false
dangle_parens: false
command_case: "canonical"
keyword_case: "unchanged"
command_case: canonical
keyword_case: unchanged
always_wrap:
- set_target_properties
- target_sources
- target_link_libraries

parse:
# We define these for our custom
# We define these for our custom
# functions so they get formatted correctly
additional_commands:
velox_add_library:
pargs:
nargs: 1+
flags:
- OBJECT
- STATIC
- SHARED
- INTERFACE
- OBJECT
- STATIC
- SHARED
- INTERFACE
kwargs: {}

velox_base_add_library:
pargs:
nargs: 1+
flags:
- OBJECT
- STATIC
- SHARED
- INTERFACE
- OBJECT
- STATIC
- SHARED
- INTERFACE
kwargs: {}

velox_compile_definitions:
pargs: 1
pargs: 1
kwargs:
PRIVATE: '*'
PUBLIC: '*'
INTERFACE: '*'

velox_include_directories:
pargs: '1+'
pargs: 1+
flags:
- SYSTEM
- BEFORE
Expand All @@ -74,11 +74,10 @@ parse:
INTERFACE: '*'

velox_link_libraries:
pargs: '1+'
pargs: 1+
kwargs:
PRIVATE: '*'
PUBLIC: '*'
INTERFACE: '*'

markup:
first_comment_is_literal: true
6 changes: 3 additions & 3 deletions .github/CODEOWNERS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
# request a review from owners on PRs with changes to matching files.
# We currently do not enforce these reviews as required so it's only a tool
# for more granular notifications at the moment. For example component maintainers
# can set a rule so that they are pinged on changes to the sections of the
# can set a rule so that they are pinged on changes to the sections of the
# codebase that are relevant for their component.

# Only users that have write access to the repo can be added as owners.
Expand All @@ -29,7 +29,7 @@ CMake/ @assignUser @majetideepak
scripts/ @assignUser @majetideepak
.github/ @assignUser @majetideepak

# Breeze
# Breeze
velox/experimental/breeze @dreveman

# Parquet
Expand All @@ -42,4 +42,4 @@ velox/connectors/hive/storage_adapters/ @majetideepak
velox/connectors/ @majetideepak

# Caching
velox/common/caching/ @majetideepak
velox/common/caching/ @majetideepak
4 changes: 2 additions & 2 deletions .github/ISSUE_TEMPLATE/bug.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

name: Bug Report
description: Report a bug or unexpected behavior.
labels: ["bug", "triage"]
labels: [bug, triage]
body:
- type: markdown
attributes:
Expand All @@ -26,7 +26,7 @@ body:
attributes:
label: Bug description
description: Please describe the issue and the expected behavior.
value: "[Expected behavior] and [actual behavior]."
value: '[Expected behavior] and [actual behavior].'
validations:
required: true
- type: textarea
Expand Down
4 changes: 2 additions & 2 deletions .github/ISSUE_TEMPLATE/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

name: Build problem
description: Report an issue when building Velox.
labels: ["build", "triage"]
labels: [build, triage]
body:
- type: markdown
attributes:
Expand All @@ -26,7 +26,7 @@ body:
attributes:
label: Problem description
description: Please describe the problem.
value: "Please describe how you were trying to build velox and what issue occured"
value: Please describe how you were trying to build velox and what issue occured

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Fix typo and align placeholder formatting
Unquoted the default value for consistency; also correct “occured” → “occurred.”

🤖 Prompt for AI Agents 
In .github/ISSUE_TEMPLATE/build.yml at line 29, fix the typo in the placeholder
text by changing "occured" to "occurred" and ensure the value is quoted
consistently with other placeholders in the file. This involves adding quotes
around the entire string and correcting the spelling mistake.

validations:
required: true
- type: textarea
Expand Down
2 changes: 1 addition & 1 deletion .github/ISSUE_TEMPLATE/enhancement.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

name: Enhancement
description: Raise a potential enhancement.
labels: ["enhancement"]
labels: [enhancement]
body:
- type: markdown
attributes:
Expand Down
4 changes: 2 additions & 2 deletions .github/ISSUE_TEMPLATE/fuzzer.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

name: Fuzzer Report
description: Report an issue with the fuzzer or found through fuzzing.
labels: ["bug", "fuzzer-found", "fuzzer"]
labels: [bug, fuzzer-found, fuzzer]
body:
- type: markdown
attributes:
Expand All @@ -26,7 +26,7 @@ body:
attributes:
label: Description
description: Please describe the issue.
placeholder: "[Expected behavior] and [actual behavior]."
placeholder: '[Expected behavior] and [actual behavior].'
validations:
required: true
- type: textarea
Expand Down
8 changes: 4 additions & 4 deletions .github/dependabot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@

version: 2
updates:
- package-ecosystem: "github-actions"
directory: "/"
- package-ecosystem: github-actions
directory: /
schedule:
interval: "weekly"
interval: weekly
commit-message:
prefix: "build(ci): "
prefix: 'build(ci): '
24 changes: 19 additions & 5 deletions .github/workflows/docs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ name: Build Documentation
on:
push:
paths:
- "velox/docs/**"
- ".github/workflows/docs.yml"
- velox/docs/**
- .github/workflows/docs.yml

pull_request:
paths:
- "velox/docs/**"
- ".github/workflows/docs.yml"
- velox/docs/**
- .github/workflows/docs.yml

permissions:
contents: read
Expand All @@ -37,8 +37,16 @@ jobs:
name: Build
runs-on: ubuntu-latest
env:
CCACHE_DIR: "/tmp/ccache"
CCACHE_DIR: /tmp/ccache
steps:
- name: Restore ccache
if: false
uses: apache/infrastructure-actions/stash/restore@3354c1565d4b0e335b78a76aedd82153a9e144d4
id: restore-cache
with:
path: ${{ env.CCACHE_DIR }}
key: ccache-wheels-8-core-ubuntu

- name: Checkout
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
with:
Expand All @@ -57,6 +65,12 @@ jobs:
which uv
uv pip install -r scripts/docs-requirements.txt

- name: Save ccache
uses: apache/infrastructure-actions/stash/save@3354c1565d4b0e335b78a76aedd82153a9e144d4
with:
path: ${{ env.CCACHE_DIR }}
key: ccache-wheels-8-core-ubuntu

- name: Build Documentation
run: |
source .venv/bin/activate
Expand Down
25 changes: 15 additions & 10 deletions .github/workflows/linux-build-base.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,45 +18,50 @@ on:
workflow_call:
inputs:
use-clang:
description: 'Use Clang to compile the project.'
description: Use Clang to compile the project.
default: false
required: false
type: boolean

jobs:
ubuntu-release:
name: "Ubuntu release with resolve_dependency"
name: Ubuntu release with resolve_dependency

@coderabbitai coderabbitai Bot Jun 19, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider hyphenating job name for consistency
The name uses an underscore in resolve_dependency. To align with the hyphenated naming in other parts (e.g., ubuntu-release), consider renaming to resolve-dependency.

🤖 Prompt for AI Agents 
In .github/workflows/linux-build-base.yml at line 28, the job name uses an
underscore in "resolve_dependency" which is inconsistent with other hyphenated
names like "ubuntu-release". Rename "resolve_dependency" to "resolve-dependency"
to maintain consistent hyphenated naming conventions.

runs-on: yscope-gh-runner

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix invalid runner label in runs-on
The runner label yscope-gh-runner is not recognized by GitHub Actions. If this is a self-hosted runner, prefix with self-hosted (e.g., runs-on: [self-hosted, yscope-gh-runner]), or switch to a valid hosted runner label.

🧰 Tools
🪛 actionlint (1.7.7)

29-29: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)

🤖 Prompt for AI Agents 
In .github/workflows/linux-build-base.yml at line 29, the runner label
`yscope-gh-runner` used in `runs-on` is invalid for GitHub Actions. To fix this,
update the `runs-on` value to include the `self-hosted` prefix if this is a
self-hosted runner, changing it to `runs-on: [self-hosted, yscope-gh-runner]`.
Alternatively, replace it with a valid GitHub-hosted runner label if not
self-hosted.

env:
CCACHE_DIR: "${{ github.workspace }}/ccache"
USE_CLANG: "${{ inputs.use-clang && 'true' || 'false' }}"
CCACHE_COMPRESSLEVEL: 2
CCACHE_MAX_SIZE: 5G
USE_CLANG: ${{ inputs.use-clang && 'true' || 'false' }}

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Quote the USE_CLANG expression.

Wrapping ${{ inputs.use-clang && 'true' || 'false' }} in double quotes prevents YAML parsing edge cases.

🤖 Prompt for AI Agents 
In .github/workflows/linux-build-base.yml at line 32, the expression for
USE_CLANG is not quoted, which can cause YAML parsing issues. Fix this by
wrapping the entire expression `${{ inputs.use-clang && 'true' || 'false' }}` in
double quotes to ensure it is treated as a string and parsed correctly.

defaults:
run:
shell: bash
working-directory: velox
steps:
- name: Ensure Stash Dirs Exists
working-directory: ${{ github.workspace }}
run: |
mkdir -p '${{ env.CCACHE_DIR }}'

- uses: actions/checkout@v4
with:
path: velox
persist-credentials: false

- name: Install Dependencies
run: |
source scripts/setup-ubuntu.sh && install_apt_deps

- name: Clear CCache Statistics
run: |
ccache -sz

Comment on lines +48 to +51

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure cache directory exists.

The ccache -sz command will fail if CCACHE_DIR doesn’t exist. Add a step or inline mkdir -p "$CCACHE_DIR" before clearing stats.

🤖 Prompt for AI Agents 
In .github/workflows/linux-build-base.yml around lines 56 to 59, the ccache
clear stats command fails if the CCACHE_DIR does not exist. Add a command to
create the cache directory before running ccache -sz by inserting mkdir -p
"$CCACHE_DIR" as a step or inline before the ccache command to ensure the
directory exists.

- name: Build Artifact
env:
VELOX_DEPENDENCY_SOURCE: BUNDLED
ICU_SOURCE: SYSTEM
MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3"
MAKEFLAGS: MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3
run: |
Comment on lines +56 to 57

@coderabbitai coderabbitai Bot Jun 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Quote complex MAKEFLAGS string

The unquoted MAKEFLAGS value can be misinterpreted. Either quote it or use a list:

env:
  MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3"
🤖 Prompt for AI Agents 
In .github/workflows/linux-build-base.yml at lines 66 to 67, the MAKEFLAGS
environment variable is set without quotes, which can cause misinterpretation of
the complex string. Fix this by enclosing the MAKEFLAGS value in double quotes
to ensure it is parsed correctly, for example, MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4
MAX_LINK_JOBS=3".

if [[ "${USE_CLANG}" = "true" ]]; then export CC=/usr/bin/clang-15; export CXX=/usr/bin/clang++-15; fi
make release

- name: CCache after
run: |
ccache -vs

- name: Run Tests
run: |
cd _build/release && ctest -j $(getconf _NPROCESSORS_ONLN) --output-on-failure --no-tests=error
Loading