ci: Cherry-pick prestodb/presto#13361 to use pre-commit for quality checks; Fix compilation caching by using cache on runner.#11
Conversation
Summary: This PR adds a [`.pre-commit-config.yaml`](https://pre-commit.com) with a number of hooks that mirror our previous `check.py` and some new ones to further improve code quality. Hooks to replace check.py: - cmake-format - clang-format - license header check - ruff (python formatting and linting) - clang-tidy (has to be called manually) New Hooks: - general quality checks - trailing white space removal - end of file fixer - large file check (is in the default config, we can probably drop it?) - check that files with shebangs are actually executable - and that executable scripts have shebangs - Several checks to improve our GitHub Actions quality - zizmor, a security focused linter - action-validator, make sure actions are valid! (we have had issues with that before) also checks if paths globs are correct - yamlfmt/yamllint make sure we use a unified style in our yaml/workflows I have added all fixes in this PR ~~but we could also merge them as a separate PR so it can be easier ignored when blaming.~~ Actually I think it is better to just handle everything in this PR to avoid causing a bunch of merge conflicts. Compared to `check.py` this is also ***much*** faster. Running all hooks on all files in the repo takes under ten seconds! Pull Request resolved: facebookincubator#13361 Reviewed By: kKPulla Differential Revision: D76274374 Pulled By: kgpai fbshipit-source-id: ba23f75fd1ff6e05d0e2a41d791028ac0220da56
## Walkthrough
This update introduces a comprehensive transition from custom formatting and header scripts to a standardized pre-commit framework for code quality enforcement. It adds and configures pre-commit, clang-tidy, and YAML linting, removes legacy formatting scripts and Makefile targets, updates documentation to reflect new workflows, and applies widespread formatting, whitespace, and type annotation improvements across configuration, documentation, and Python stub files.
## Changes
| Files/Groups | Change Summary |
|------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------|
| `.pre-commit-config.yaml`, `.clang-tidy`, `.yamllint.yml`, `.yamlfmt.yml` | Added pre-commit, clang-tidy, and YAML lint/formatter configuration files. |
| `.cmake-format.yaml`, `.github/ISSUE_TEMPLATE/*.yml`, `.github/dependabot.yml`, `.github/workflows/*.yml` | YAML, workflow, and configuration files: reformatted to remove unnecessary quotes, improve indentation, and standardize lists; added ccache caching steps in some workflows. |
| `.github/CODEOWNERS`, `NOTICE.txt`, `CONTRIBUTING.md`, `docker-compose.yml` | Removed trailing spaces, fixed minor typos, and cleaned up formatting in documentation and config files. |
| `.github/zizmor.yml` | Replaced Dockerfile instructions with a rules configuration ignoring a specific YAML file. |
| `Makefile`, `scripts/check.py`, `scripts/git-clang-format` | Removed Makefile targets and scripts for formatting, header checking, and Docker container builds. |
| `scripts/checks/license-header.py`, `scripts/checks/run-clang-tidy.py`, `scripts/checks/util.py` | Minor improvements: updated default license header path, removed unused imports, and refined type checks. |
| `scripts/ci/benchmark-runner.py`, `scripts/ci/bm-report/build-metrics.py` | Improved exception handling, removed unused imports, and replaced lambdas with named functions. |
| `CODING_STYLE.md` | Overhauled documentation to explain pre-commit workflow and hook usage, replacing previous Makefile-based instructions. |
| `CMake/Findlz4.cmake`, `CMake/Findlzo2.cmake`, `CMake/Findzstd.cmake`, `CMake/FindSodium.cmake`, `CMake/Findglog.cmake` | Added Apache 2.0 license headers and reformatted property settings in CMake scripts. |
| `python/pyvelox/__init__.py`, `python/pyvelox/utils/__init__.py` | Added linter directives and explicit import aliases. |
| `python/test/test_vector.py`, `python/test/test_plan_builder.py` | Test code: variable naming and string formatting improvements. |
| `velox/python/plan_builder/plan_builder.pyi` | Updated type annotations to use built-in generics, removed unused imports and shebang. |
| `velox/python/arrow/arrow.pyi`, `velox/python/file/file.pyi`, `velox/python/runner/runner.pyi`, `velox/python/type/type.pyi`, `velox/python/vector/vector.pyi` | Removed shebangs, unused imports, and extra blank lines from Python stub files. |
| `velox/docs/*`, `velox/docs/functions/spark/string.rst` | Widespread whitespace cleanup, formatting corrections, and addition of new internal function documentation. |
| `velox/exec/tests/utils/TableScanTestBase.cpp` | Replaced legacy copyright with Apache 2.0 license. |
| `velox/dwio/parquet/thrift/ParquetThriftTypes.cpp` | Streamlined output stream formatting in printTo methods. |
| `velox/experimental/breeze/test/*`, `velox/experimental/cudf/.clang-format` | Linter suppression comments, idiomatic Python, and newline fixes. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Developer
participant Git
participant Pre-commit
participant CI
Developer->>Git: git commit / push
Git->>Pre-commit: Trigger hooks (pre-commit, pre-push)
Pre-commit->>Developer: Run formatting, linting, header, and custom hooks
Pre-commit-->>Developer: Report issues, auto-fix if configured
Developer->>Git: Stage/recommit if changes made by hooks
Git->>CI: Push triggers CI workflows
CI->>Pre-commit: Run hooks in CI (enforces checks)
CI-->>Developer: Report pass/fail statusSuggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🔭 Outside diff range comments (3)
scripts/ci/bm-report/build-metrics.py (1)
154-155: Typo: “Unkown” → “Unknown”- print(f"Unkown file type found: {object_path}") + print(f"Unknown file type found: {object_path}").github/workflows/preliminary_checks.yml (1)
49-58: Robust file detection for pre-commitThe
git diff --name-only HEAD^1 HEADapproach may miss files in multi-commit or squash-based PRs. A more reliable pattern is:git fetch origin ${{ github.event.pull_request.base.ref }} files=$(git diff --name-only origin/${{ github.event.pull_request.base.ref }} HEAD)velox/python/plan_builder/plan_builder.pyi (1)
34-34: Add type annotations and avoid shadowing built-in
The stubdef deserialize_plan(str): ...both shadows the built-instrand lacks type hints. Consider:def deserialize_plan(serialized: str) -> PlanNode: ...
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (8)
static/icon.svgis excluded by!**/*.svgstatic/logo.svgis excluded by!**/*.svgwebsite/static/img/banner-pattern.svgis excluded by!**/*.svgwebsite/static/img/icon-commits.svgis excluded by!**/*.svgwebsite/static/img/icon-contributors.svgis excluded by!**/*.svgwebsite/static/img/icon-github-star.svgis excluded by!**/*.svgwebsite/static/img/logo.svgis excluded by!**/*.svgwebsite/static/img/velox-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (107)
.clang-tidy(1 hunks).cmake-format.yaml(2 hunks).github/CODEOWNERS(3 hunks).github/ISSUE_TEMPLATE/bug.yml(2 hunks).github/ISSUE_TEMPLATE/build.yml(2 hunks).github/ISSUE_TEMPLATE/enhancement.yml(1 hunks).github/ISSUE_TEMPLATE/fuzzer.yml(2 hunks).github/dependabot.yml(1 hunks).github/workflows/docs.yml(3 hunks).github/workflows/linux-build-base.yml(2 hunks).github/workflows/linux-build.yml(1 hunks).github/workflows/preliminary_checks.yml(1 hunks).github/workflows/scheduled.yml(1 hunks).github/zizmor.yml(1 hunks).pre-commit-config.yaml(1 hunks).yamlfmt.yml(1 hunks).yamllint.yml(1 hunks)CMake/FindSodium.cmake(2 hunks)CMake/Findglog.cmake(2 hunks)CMake/Findlz4.cmake(1 hunks)CMake/Findlzo2.cmake(1 hunks)CMake/Findzstd.cmake(1 hunks)CODING_STYLE.md(1 hunks)CONTRIBUTING.md(6 hunks)Makefile(0 hunks)NOTICE.txt(1 hunks)docker-compose.yml(6 hunks)pyproject.toml(0 hunks)python/pyvelox/__init__.py(1 hunks)python/pyvelox/utils/__init__.py(1 hunks)python/pyvelox/utils/data_generator.py(1 hunks)python/test/test_plan_builder.py(1 hunks)python/test/test_vector.py(3 hunks)scripts/check.py(0 hunks)scripts/checks/license-header.py(1 hunks)scripts/checks/run-clang-tidy.py(0 hunks)scripts/checks/util.py(1 hunks)scripts/ci/benchmark-runner.py(2 hunks)scripts/ci/bm-report/build-metrics.py(1 hunks)scripts/ci/bm-report/report.qmd(3 hunks)scripts/ci/hdfs-client.xml(1 hunks)scripts/ci/presto/etc/hive.properties(1 hunks)scripts/ci/presto/start-prestojava.sh(0 hunks)scripts/ci/signature.py(1 hunks)scripts/docker/pyvelox.dockerfile(0 hunks)scripts/git-clang-format(0 hunks)scripts/setup-centos9.sh(0 hunks)scripts/setup-classpath.sh(0 hunks)scripts/setup-helper-functions.sh(0 hunks)scripts/velox_env_linux.yml(2 hunks)scripts/velox_env_mac.yml(2 hunks)velox/docs/bindings/python/arrow.rst(1 hunks)velox/docs/bindings/python/index.rst(0 hunks)velox/docs/bindings/python/legacy.rst(1 hunks)velox/docs/bindings/python/runners.rst(1 hunks)velox/docs/bindings/python/vector.rst(1 hunks)velox/docs/conf.py(1 hunks)velox/docs/develop/aggregations.rst(3 hunks)velox/docs/develop/debugging/print-plan-with-stats.rst(0 hunks)velox/docs/develop/dynamic-loading.rst(1 hunks)velox/docs/develop/operators.rst(4 hunks)velox/docs/ext/issue.py(1 hunks)velox/docs/ext/pr.py(1 hunks)velox/docs/ext/spark.py(1 hunks)velox/docs/functions/presto/binary.rst(2 hunks)velox/docs/functions/presto/window.rst(1 hunks)velox/docs/functions/spark/aggregate.rst(3 hunks)velox/docs/functions/spark/array.rst(1 hunks)velox/docs/functions/spark/binary.rst(1 hunks)velox/docs/functions/spark/bitwise.rst(2 hunks)velox/docs/functions/spark/comparison.rst(2 hunks)velox/docs/functions/spark/coverage.rst(1 hunks)velox/docs/functions/spark/datetime.rst(0 hunks)velox/docs/functions/spark/decimal.rst(1 hunks)velox/docs/functions/spark/json.rst(2 hunks)velox/docs/functions/spark/misc.rst(1 hunks)velox/docs/functions/spark/string.rst(10 hunks)velox/docs/monitoring.rst(1 hunks)velox/docs/monthly-updates/2021/december-2021.rst(1 hunks)velox/docs/monthly-updates/2021/november-2021.rst(1 hunks)velox/docs/monthly-updates/2022/april-2022.rst(1 hunks)velox/docs/monthly-updates/2022/august-2022.rst(1 hunks)velox/docs/monthly-updates/2022/july-2022.rst(1 hunks)velox/docs/monthly-updates/2022/june-2022.rst(1 hunks)velox/docs/monthly-updates/2022/may-2022.rst(1 hunks)velox/docs/monthly-updates/2022/october-2022.rst(1 hunks)velox/docs/monthly-updates/2023/august-2023.rst(1 hunks)velox/docs/monthly-updates/2023/december-2023.rst(1 hunks)velox/docs/monthly-updates/2023/november-2023.rst(1 hunks)velox/docs/monthly-updates/january-2024.rst(0 hunks)velox/docs/monthly-updates/july-2024.rst(1 hunks)velox/dwio/common/tests/Lemire/FastPFor/LICENSE(1 hunks)velox/dwio/dwrf/test/CommonTests.cpp(2 hunks)velox/dwio/parquet/thrift/ParquetThriftTypes.cpp(28 hunks)velox/exec/tests/utils/TableScanTestBase.cpp(1 hunks)velox/experimental/breeze/test/generator_common.py(1 hunks)velox/experimental/breeze/test/kernel_generator.py(2 hunks)velox/experimental/breeze/test/test_fixture_generator.py(5 hunks)velox/experimental/cudf/.clang-format(1 hunks)velox/experimental/wave/README.md(1 hunks)velox/functions/prestosql/coverage/README.md(1 hunks)velox/python/arrow/arrow.pyi(0 hunks)velox/python/file/file.pyi(0 hunks)velox/python/plan_builder/plan_builder.pyi(2 hunks)velox/python/runner/runner.pyi(0 hunks)velox/python/type/type.pyi(0 hunks)velox/python/vector/vector.pyi(0 hunks)
⛔ Files not processed due to max files limit (16)
- velox/substrait/tests/data/q1_first_stage.json
- velox/substrait/tests/data/q6_first_stage.json
- website/README.md
- website/blog/2023-03-15-build-experience.mdx
- website/blog/2024-05-31-optimize-try-more.mdx
- website/blog/2024-08-23-ci-migration.mdx
- website/blog/2025-03-25-velox-primer-part-2.mdx
- website/blog/2025-05-12-velox-primer-part-3.mdx
- website/blog/authors.yml
- website/docs/community/01-design-philosophy.md
- website/docs/community/02-technical-governance.md
- website/docs/community/03-components-and-maintainers.md
- website/docs/community/index.md
- website/src/components/HomepageFeatures/index.js
- website/src/components/VeloxConBanner/index.js
- website/src/pages/index.js
💤 Files with no reviewable changes (19)
- scripts/setup-classpath.sh
- velox/docs/bindings/python/index.rst
- scripts/setup-centos9.sh
- scripts/ci/presto/start-prestojava.sh
- velox/python/arrow/arrow.pyi
- scripts/docker/pyvelox.dockerfile
- scripts/checks/run-clang-tidy.py
- scripts/setup-helper-functions.sh
- velox/python/runner/runner.pyi
- velox/docs/functions/spark/datetime.rst
- velox/python/type/type.pyi
- velox/python/file/file.pyi
- velox/docs/monthly-updates/january-2024.rst
- pyproject.toml
- velox/python/vector/vector.pyi
- velox/docs/develop/debugging/print-plan-with-stats.rst
- Makefile
- scripts/git-clang-format
- scripts/check.py
🧰 Additional context used
🪛 Pylint (3.3.7)
velox/docs/conf.py
[warning] 33-34: No exception type(s) specified
(W0702)
python/pyvelox/utils/data_generator.py
[convention] 160-160: Line too long (122/100)
(C0301)
[error] 160-160: Instance of 'dict' has no 'row_count' member
(E1101)
[error] 160-160: Instance of 'dict' has no 'file_count' member
(E1101)
[error] 160-160: Instance of 'dict' has no 'output_path' member
(E1101)
python/pyvelox/utils/__init__.py
[convention] 15-15: Import alias does not rename original package
(C0414)
python/pyvelox/__init__.py
[error] 15-15: Unable to import 'pyvelox.legacy'
(E0401)
[error] 16-16: Unable to import 'pyvelox.legacy'
(E0401)
[convention] 16-16: Import alias does not rename original package
(C0414)
scripts/ci/benchmark-runner.py
[warning] 84-84: Catching too general exception Exception
(W0718)
python/test/test_vector.py
[error] 191-191: Module 'pyvelox' has no 'from_list' member
(E1101)
[error] 194-194: Module 'pyvelox' has no 'from_list' member
(E1101)
[error] 197-197: Module 'pyvelox' has no 'from_list' member
(E1101)
[error] 274-274: Module 'pyvelox' has no 'from_list' member
(E1101)
[error] 275-275: Module 'pyvelox' has no 'from_list' member
(E1101)
velox/experimental/breeze/test/generator_common.py
[error] 74-74: Unable to import 'clang.cindex'
(E0401)
[convention] 74-74: Import "import clang.cindex" should be placed at the top of the module
(C0413)
[error] 75-75: Unable to import 'clang.cindex'
(E0401)
[convention] 75-75: Import "from clang.cindex import CursorKind" should be placed at the top of the module
(C0413)
scripts/checks/util.py
[convention] 40-40: Use isinstance() rather than type() for a typecheck.
(C0123)
scripts/ci/signature.py
[convention] 149-149: Line too long (143/100)
(C0301)
🪛 Ruff (0.11.9)
python/pyvelox/utils/data_generator.py
160-160: Logging statement uses f-string
(G004)
160-160: Trailing comma missing
Add trailing comma
(COM812)
scripts/ci/bm-report/build-metrics.py
127-127: Missing return type annotation for private function ms2sec
(ANN202)
127-127: Missing type annotation for function argument x
(ANN001)
scripts/ci/benchmark-runner.py
84-84: Do not catch blind exception: Exception
(BLE001)
scripts/checks/license-header.py
32-32: Trailing comma missing
Add trailing comma
(COM812)
python/test/test_vector.py
193-193: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
196-196: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
340-340: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
342-342: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
344-344: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~210-~210: Possible missing comma found.
Context: ...h information at a time; beyond that threshold the ability to find bugs and other flaw...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~235-~235: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...s possible, try to avoid functions with very large bodies. In the (rare) cases where la...
(EN_WEAK_ADJECTIVE)
[uncategorized] ~241-~241: Possible missing article found.
Context: ...upport your claims. * As needed, add micro-benchmark to objectively evaluate perfo...
(AI_HYDRA_LEO_MISSING_A)
velox/experimental/wave/README.md
[typographical] ~28-~28: Use a comma after an introductory phrase.
Context: ...al CUDA driver installed (like our CI). For this reason we need to use the base functions to cr...
(COMMA_INTRODUCTORY_WORDS_PHRASES)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pre-commit
🔇 Additional comments (101)
velox/docs/monitoring.rst (1)
9-9: Add trailing newline for POSIX compliance
This matches the pattern applied to other documentation files, ensuring a newline at EOF.velox/docs/monthly-updates/2022/april-2022.rst (1)
59-59: Append newline at end of file
Aligns with the repository’s convention of terminating text files with a newline.scripts/ci/presto/etc/hive.properties (1)
5-5: Ensure trailing newline in configuration file
Adds a final newline for consistency and to prevent potential parsing issues.velox/docs/monthly-updates/2022/may-2022.rst (1)
68-68: Insert EOF newline for consistency
Standardises file endings across documentation.velox/docs/monthly-updates/2022/october-2022.rst (1)
64-64: Add final newline to conform with POSIX
Maintains consistent formatting for all .rst files.scripts/ci/hdfs-client.xml (1)
7-7: Add trailing newline for consistent file endings.Ensures the XML file ends with a newline, aligning with repository conventions.
velox/dwio/common/tests/Lemire/FastPFor/LICENSE (1)
191-191: Add missing trailing newline.Aligns the LICENSE file with POSIX text file newline conventions.
velox/docs/ext/issue.py (1)
16-16: Remove unused import.The
utilsimport was correctly removed, tidying up unused references.velox/docs/monthly-updates/2022/august-2022.rst (1)
88-88: Add trailing newline for POSIX compliance.Ensures the document ends with a newline, matching repository style guidelines.
velox/docs/functions/spark/binary.rst (1)
35-37: Clean up trailing whitespace.Removed extraneous spaces at the ends of lines to improve formatting consistency.
velox/docs/monthly-updates/2023/december-2023.rst (1)
99-99: Add missing newline at end of file
Ensures consistent POSIX-style EOF formatting and satisfies pre-commit’s newline requirement.velox/docs/monthly-updates/july-2024.rst (1)
128-128: Add missing newline at end of file
Maintains uniform file endings across documentation, aligning with pre-commit formatting checks.velox/docs/monthly-updates/2021/december-2021.rst (1)
38-38: Remove trailing whitespace and add newline at EOF
Cleans up stray whitespace and adds final newline to comply with formatting standards.velox/docs/functions/presto/window.rst (1)
171-171: Add missing newline at end of file
Aligns file with the project’s newline-at-EOF convention enforced by pre-commit hooks.velox/docs/monthly-updates/2023/august-2023.rst (1)
85-85: Add missing newline at end of file
Ensures consistent documentation formatting and passes pre-commit checks.velox/docs/bindings/python/legacy.rst (1)
12-12: Formatting: add missing trailing newline
A newline has been added after the last directive to ensure consistent POSIX‐compliant file termination across documentation.velox/docs/bindings/python/runners.rst (1)
8-8: Formatting: add missing trailing newline
A newline has been appended at EOF to align with the project's documentation style and avoid warnings in CI checks.velox/docs/monthly-updates/2021/november-2021.rst (1)
38-38: Formatting: remove extra blank line
The trailing blank line at the end of the credits section was removed to normalize file endings and satisfy the repository’s newline‐at‐EOF policy.velox/docs/ext/pr.py (1)
16-16: Cleanup: remove unused import
Dropped the unusedutilsimport fromdocutils, reducing clutter and eliminating linter warnings.velox/docs/monthly-updates/2023/november-2023.rst (1)
105-105: Formatting: add missing trailing newline
Inserted a newline after the last contributor to comply with the project’s consistent file termination requirements.velox/docs/monthly-updates/2022/july-2022.rst (1)
58-58: Ensure consistent newline at end of file.A trailing newline has been added to this documentation file to align with repository conventions and avoid issues with tools that expect EOF newlines.
velox/experimental/cudf/.clang-format (1)
27-27: Add missing newline at end of.clang-format.Including a final newline prevents potential warnings from formatting utilities and upholds POSIX file standards.
velox/docs/bindings/python/arrow.rst (1)
8-8: Add trailing newline.This change introduces a final newline to the RST file, ensuring compatibility with Sphinx and other documentation tools.
velox/docs/bindings/python/vector.rst (1)
8-8: Add trailing newline.A newline at EOF ensures consistent formatting and prevents parsing issues in the documentation build.
python/pyvelox/utils/data_generator.py (1)
160-160: Remove trailing whitespace before inline comment.The space before
# pyre-ignoreintroduces trailing whitespace that will be flagged by pre-commit hooks. Please remove it.Apply this diff:
- f"Written {result.row_count} records to {result.file_count} output files at '{result.output_path}'" # pyre-ignore + f"Written {result.row_count} records to {result.file_count} output files at '{result.output_path}'" # pyre-ignoreLikely an incorrect or invalid review comment.
velox/docs/monthly-updates/2022/june-2022.rst (1)
66-66: Add trailing newline at EOF
Ensures POSIX-compliant file formatting and consistency across documentation.velox/docs/functions/spark/bitwise.rst (2)
7-7: Remove trailing whitespace
Cleans up an extra space in the function description and keeps the documentation tidy.
50-50: Ensure newline at EOF
Adds a trailing newline for POSIX compliance and consistency with other docs.CMake/Findzstd.cmake (1)
2-13: Add Apache 2.0 license header
Standardizes licensing comments to match other CMake find modules.velox/exec/tests/utils/TableScanTestBase.cpp (1)
1-15: Update to Apache 2.0 license header
Replaces the proprietary notice with the standard Apache License block, aligning with project-wide licensing.scripts/velox_env_linux.yml (2)
25-25: Add space after comment marker
Improves readability by standardizing comment formatting.
40-40: Add space after comment marker
Maintains consistent YAML comment style with other environment configuration files.velox/experimental/wave/README.md (2)
20-20: Approve removal of trailing whitespace.The updated line cleans up trailing spaces without altering the intended instructions.
27-28: Add comma after introductory phrase.Consider inserting a comma after “For this reason” to improve readability:
- For this reason we need to use the base functions... + For this reason, we need to use the base functions...Likely an incorrect or invalid review comment.
CMake/Findlzo2.cmake (1)
2-14: Approve addition of Apache 2.0 license header.The new header aligns with other CMake modules and introduces no functional changes.
velox/functions/prestosql/coverage/README.md (1)
18-18: Approve removal of trailing space.This cleanup removes extraneous whitespace without modifying the documentation semantics.
velox/docs/functions/spark/misc.rst (1)
7-7: Approve removal of trailing whitespace.The formatting change is purely cosmetic and preserves all existing content.
velox/docs/develop/dynamic-loading.rst (1)
15-15: Approve trailing-whitespace removal
This change tidies up formatting and aligns the file with the new pre-commit whitespace rules.CMake/Findlz4.cmake (1)
2-14: Approve addition of Apache 2.0 license header
The newly inserted header correctly applies the required licensing boilerplate without altering any logic.velox/docs/functions/spark/comparison.rst (2)
30-30: Approve removal of trailing space
This minor cleanup improves consistency in the documentation source.
47-47: Approve removal of trailing space
The whitespace correction ensures uniform formatting across the docs.NOTICE.txt (1)
13-13: Approve typo fix in project notice
Removing the extra “The” corrects the phrasing in the legal notice.velox/docs/functions/spark/array.rst (1)
265-265: Approve removal of trailing space
This adjustment cleans up the shuffle function description to meet formatting standards.velox/docs/functions/presto/binary.rst (1)
78-86: Whitespace cleanup LGTMOnly trailing-space removal and minor re-flow – no semantic impact.
Also applies to: 92-101
.github/CODEOWNERS (1)
19-20: No-op formatting changesTrailing-space trims don’t affect CODEOWNERS resolution.
All good.Also applies to: 32-32, 45-45
velox/docs/functions/spark/aggregate.rst (1)
29-31: Whitespace tweaks acknowledgedPure formatting; content unchanged.
No further action needed.Also applies to: 51-53, 165-167, 169-169
velox/experimental/breeze/test/generator_common.py (1)
74-75: Import placement rationale accepted
clang.cindexmust be imported aftersys.pathmanipulation; suppressing E402 with# noqa E402is the correct compromise.
Nothing else to address..github/dependabot.yml (1)
17-22: Consistent quoting style – looks goodUnquoting the scalar values while keeping the
commit-message.prefixquoted improves readability and poses no functional risk.velox/docs/functions/spark/json.rst (1)
24-33: Whitespace cleanup approvedOnly spacing was touched; content and semantics remain intact.
Also applies to: 56-56
velox/experimental/breeze/test/kernel_generator.py (1)
109-110: Pythonicis not None👍Switching from
!= Nonetois not Noneremoves a lint warning and is the correct identity check.scripts/velox_env_mac.yml (1)
25-25: Minor style tweak acknowledgedThe added space after comment markers aligns this file with common YAML guidelines.
Also applies to: 39-39
scripts/ci/bm-report/report.qmd (1)
225-225: Trailing-space removal only – no action requiredAlso applies to: 335-335
velox/docs/develop/aggregations.rst (2)
121-123: Pure formatting cleanup.
Removed trailing spaces and adjusted blank lines in the bullet list for consistent spacing. No content changes introduced.
137-139: Pure formatting cleanup.
Adjusted indentation and blank lines around the SQL example for consistency. No functional or semantic changes.velox/docs/functions/spark/coverage.rst (1)
84-86: Pure formatting cleanup.
Removed trailing whitespace characters from the function table entries to enforce consistent styling. No content modifications.velox/docs/ext/spark.py (1)
656-658: Stylistic consistency only.
Updated the f-string quoting around the index entry text for built-in functions. This is purely a style tweak with no behavioural impact.scripts/ci/benchmark-runner.py (1)
456-459: Updated help text looks goodRe-flowing the description into one line improves readability without altering semantics.
velox/docs/develop/operators.rst (1)
319-319: Whitespace cleanup acknowledgedTrailing-space removals are harmless and keep the docs tidy.
Also applies to: 350-360, 392-394, 400-407, 412-418, 424-425
.github/zizmor.yml (1)
14-17: Rule section LGTMSwitching to the
rules:stanza is clear and the ignore list is explicit.velox/docs/functions/spark/decimal.rst (1)
163-169: Minor wording/whitespace tweak acceptedNo functional impact; documentation reads well.
velox/dwio/dwrf/test/CommonTests.cpp (1)
98-99: Formatting nit resolvedRemoval of stray spaces keeps clang-format happy. No behaviour change.
Also applies to: 148-149
python/test/test_vector.py (3)
190-198: Underscore-prefixed throw-away variables are fine hereRenaming the captured variables to
_a/_v/_amakes their “unused” nature explicit and keeps the linter quiet. No functional impact.
274-276: Consistent unused-variable convention retainedThe new
_bigger_than_int64sentinel follows the same pattern as above and preserves the intent of the overflow test.
336-346: Readability of slice step tests unchangedOnly variable names were swapped to the underscore form; the
NotImplementedErrorcontract remains intact.CMake/FindSodium.cmake (3)
270-273: Property list re-formatting LGTMPurely cosmetic re-wrap of
set_target_properties; no behavioural change.
277-281: Static-link branch still sets all required locations
IMPORTED_LOCATION(_DEBUG)now grouped under the same call—looks clean, no regressions spotted.
297-301: Extra IMPORTED_LOCATION_ variants retained*The additional config-specific locations for the DLL are still wired up correctly.
CMake/Findglog.cmake (2)
2-14: Licence header addition onlyAdding the ASF-2.0 header is a non-code change—good housekeeping.
40-46: Imported target properties remain correctInterface include path and imported location are still set; switching to the modern formatting is harmless.
velox/experimental/breeze/test/test_fixture_generator.py (5)
37-41: Dropped unnecessary f-string – nice micro-clean-up
AUTOGEN_HEADERcontained no interpolation, so using a plain triple-quoted string is clearer.
266-267: Kernel launcher line re-wrapped onlyIndentation change doesn’t affect code generation; everything still compiles.
417-418: Lambda body formatting tweakNo semantic change; fine to merge.
433-438: Constant include block – interpolation not requiredThe OpenCL fixture includes no longer depend on
test_type, so converting to a raw string is OK.
545-549: Same for Metal fixture includesMatches the pattern above; good consistency.
.yamllint.yml (1)
14-47: Comprehensive yamllint policy addedThe rule-set is strict but sensible and pairs well with the new pre-commit hook.
velox/dwio/parquet/thrift/ParquetThriftTypes.cpp (1)
852-862: Formatting Consistency: Consolidate stream insertions acrossprintTomethodsStylistic update merges separate
out << ", "and field‐label insertions into single streaming expressions for uniform readability. No functional change.Also applies to: 1363-1363, 1666-1668, 1779-1779, 1997-1997, 2781-2799, 2990-2995
.github/ISSUE_TEMPLATE/enhancement.yml (1)
17-17: YAML list syntax update is correct
Switching to an unquoted list[enhancement]is valid YAML and aligns with other issue templates’ style updates.scripts/checks/license-header.py (1)
31-33: Update default license header path
Pointing--headertoscripts/checks/license.headermatches the new file layout. Ensure thatlicense.headerresides at this path.velox/docs/conf.py (1)
32-34: Suppress bare except lint warning correctly
The# noqa E722comment on the bareexcept:silences the warning as intended..github/workflows/scheduled.yml (2)
20-29: Unquote paths and modernize trigger to pyproject.toml
Removing quotes around paths and replacingsetup.pywithpyproject.tomlkeeps workflow configuration consistent with current project standards.
35-44: Verify removal ofthird_party/**from CI triggers
Droppingthird_party/**may skip CI checks on submodule or vendored code. Confirm this change won’t unintentionally bypass necessary tests.CONTRIBUTING.md (1)
7-8: No substantive change—looks good.The edits in this block are purely whitespace/word-wrapping adjustments; there is no content impact.
.github/workflows/linux-build.yml (1)
22-30: Quotation removal is acceptable.Unquoting the path globs is syntactically valid YAML and should not affect the filter logic. Nothing further.
.github/ISSUE_TEMPLATE/bug.yml (1)
17-18: Minor YAML style tweak acknowledged.Label list formatting change is stylistic only; behaviour remains unchanged.
python/test/test_plan_builder.py (1)
58-59: String literal consolidation is fine.The expected plan string is functionally identical; test clarity preserved.
.github/ISSUE_TEMPLATE/build.yml (1)
17-17: Standardize YAML list quoting
Removed unnecessary quotes around thelabelsarray to align with other templates..github/ISSUE_TEMPLATE/fuzzer.yml (2)
17-17: Standardize YAML list quoting
Removed quotes around thelabelsarray to match other issue templates.
29-29: Unify placeholder formatting
Updated the placeholder to use single quotes for consistency; no functional change..cmake-format.yaml (2)
27-28: Normalize YAML string quoting
Removed quotes fromcommand_caseandkeyword_casevalues for consistency and readability; no semantic change.
42-45: Normalize YAML list quoting
Unquoted the flags undervelox_add_libraryto match style elsewhere; unchanged behaviour..yamlfmt.yml (1)
14-22: Convert to declarative YAML formatter config
Replaced embedded setup script with a structured configuration for yamlfmt. Confirm this aligns with the pre-commit hook configuration..github/workflows/docs.yml (4)
20-21: Standardize path pattern quoting
Removed quotes around trigger paths inpushandpull_requestevents; purely stylistic.Also applies to: 25-26
40-40: Simplify environment variable syntax
UnquotedCCACHE_DIRvalue for consistency with other workflows.
42-49: Verify ccache restore step
The restore action is explicitly disabled (if: false). Confirm if this was intentional or if caching should be enabled to speed up builds.
68-73: Add ccache save step
Introduced aSave ccachestep to persist the cache with a consistent key; this complements the caching strategy.docker-compose.yml (1)
53-58: Ensure folded style preserves intended whitespaceUsing the YAML folded style (
>) forEXTRA_CMAKE_FLAGSconcatenates lines with spaces, which may alter flag parsing. Verify that this layout yields the correct flags in CMake. If literal line breaks are required, switch to a literal block (|)..pre-commit-config.yaml (4)
1-8: License header inclusion is correct
Including the full Apache 2.0 header at the top clearly communicates project licensing.
26-35: Verifytrailing-whitespacehook arguments
The--markdown-linebreak-ext=mdflag is uncommon; please confirm it’s supported inpre-commit-hooksv5.0.0 to avoid hook failures.
48-58: Verify clang-tidy hook configuration
You’ve declaredclang-tidyas a Python hook withadditional_dependencies: [clang-tidy==18.1.8], but upstreamclang-tidyis usually a system binary, not a PyPI package. Please confirm this installs correctly or consider switching tolanguage: systemor using the officialpre-commit-mirrors-clang-tidyrepo.
59-78: Confirm license-header script path
The hook entry points to./scripts/checks/license-header.py. Ensure that script still exists in the repo at that path and is executable—if it was removed with the legacy scripts, update or restore it.velox/python/plan_builder/plan_builder.pyi (1)
18-18: Remove unused imports cleanly
Retaining onlyOptionalfromtypingaligns with PEP 585 improvements and reduces clutter.
| # limitations under the License. | ||
|
|
||
| from .data_generator import generate_tpch_data | ||
| from .data_generator import generate_tpch_data as generate_tpch_data |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant import alias.
The alias duplicates the original name and can be omitted for clarity:
-from .data_generator import generate_tpch_data as generate_tpch_data
+from .data_generator import generate_tpch_data📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from .data_generator import generate_tpch_data as generate_tpch_data | |
| -from .data_generator import generate_tpch_data as generate_tpch_data | |
| +from .data_generator import generate_tpch_data |
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 15-15: Import alias does not rename original package
(C0414)
🤖 Prompt for AI Agents
In python/pyvelox/utils/__init__.py at line 15, remove the redundant alias in
the import statement by changing "from .data_generator import generate_tpch_data
as generate_tpch_data" to "from .data_generator import generate_tpch_data" to
improve clarity.
| def ms2sec(x): | ||
| return x / 1000 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add type hints to silence Ruff & improve readability
ms2sec is flagged (ANN001/ANN202). A tiny hint keeps the codebase clean without extra ignores.
- def ms2sec(x):
- return x / 1000
+ def ms2sec(x: int | float) -> float:
+ return x / 1000.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def ms2sec(x): | |
| return x / 1000 | |
| def ms2sec(x: int | float) -> float: | |
| return x / 1000.0 |
🧰 Tools
🪛 Ruff (0.11.9)
127-127: Missing return type annotation for private function ms2sec
(ANN202)
127-127: Missing type annotation for function argument x
(ANN001)
🤖 Prompt for AI Agents
In scripts/ci/bm-report/build-metrics.py around lines 127 to 129, the function
ms2sec lacks type hints, causing Ruff to flag it. Add type hints to the function
signature by specifying the input parameter type as float and the return type as
float to improve readability and silence the linter warnings.
| warp_threads = self.num_warp_threads() | ||
| if warp_threads != None: | ||
| if warp_threads is not None: | ||
| out.write(f"enum {{ WARP_THREADS = {warp_threads} }};") | ||
| for kernel in kernels: |
There was a problem hiding this comment.
Missing newline after generated enum may break emitted code
out.write(f"enum {{ WARP_THREADS = {warp_threads} }};") emits no trailing newline, so the next token (often template or a function return-type) will appear on the same line, producing malformed C++.
- if warp_threads is not None:
- out.write(f"enum {{ WARP_THREADS = {warp_threads} }};")
+ if warp_threads is not None:
+ out.write(f"enum {{ WARP_THREADS = {warp_threads} }};\n")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| warp_threads = self.num_warp_threads() | |
| if warp_threads != None: | |
| if warp_threads is not None: | |
| out.write(f"enum {{ WARP_THREADS = {warp_threads} }};") | |
| for kernel in kernels: | |
| warp_threads = self.num_warp_threads() | |
| if warp_threads is not None: | |
| out.write(f"enum {{ WARP_THREADS = {warp_threads} }};\n") | |
| for kernel in kernels: |
🧰 Tools
🪛 Pylint (3.3.7)
[error] 108-108: Assigning result of a function call, where the function returns None
(E1128)
🤖 Prompt for AI Agents
In velox/experimental/breeze/test/kernel_generator.py around lines 108 to 111,
the code writes an enum declaration without a trailing newline, causing the next
token to be on the same line and resulting in malformed C++ code. Fix this by
appending a newline character to the string passed to out.write, ensuring the
enum declaration ends with a newline.
| error_message = "" | ||
| for rep_change in delta["repetition_change"]: | ||
| error_message += f"""'{rep_change.get_root_key()}{rep_change.t1}' is repeated {rep_change.repetition['new_repeat']} times.\n""" | ||
| error_message += f"""'{rep_change.get_root_key()}{rep_change.t1}' is repeated {rep_change.repetition["new_repeat"]} times.\n""" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Address line length violation.
The new f-string at line 149 is 143 characters long, triggering Pylint C0301. Consider breaking the message into multiple concatenated strings or using parentheses and implicit concatenation for better readability and to satisfy line-length limits.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 149-149: Line too long (143/100)
(C0301)
🤖 Prompt for AI Agents
In scripts/ci/signature.py around lines 147 to 149, the f-string used to build
error_message exceeds the line length limit. To fix this, break the long
f-string into multiple shorter strings using parentheses for implicit
concatenation or split it into concatenated parts, ensuring each line stays
within the allowed length for better readability and compliance with Pylint
C0301.
| from .legacy import * # noqa: F403 | ||
| from .legacy import __version__ as __version__ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove redundant import alias.
The statement from .legacy import __version__ as __version__ uses an unnecessary alias. Simplify to:
from .legacy import __version__This preserves clarity and resolves the Pylint C0414 alias warning.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 15-15: Unable to import 'pyvelox.legacy'
(E0401)
[error] 16-16: Unable to import 'pyvelox.legacy'
(E0401)
[convention] 16-16: Import alias does not rename original package
(C0414)
🤖 Prompt for AI Agents
In python/pyvelox/__init__.py at lines 15 to 16, the import statement uses an
unnecessary alias for __version__. Replace `from .legacy import __version__ as
__version__` with `from .legacy import __version__` to remove the redundant
alias and fix the Pylint C0414 warning.
| The `clang-tidy` hook will *not* be run automatically as it takes a long time and | ||
| requires CMake to be run first to create `compile_commands.json`. | ||
| It can be run explicitly via `pre-commit run --hook-stage=manual`. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify manual clang-tidy invocation
Specify the hook id for clarity:
pre-commit run --hook-stage=manual clang-tidyThis aligns the docs with the actual hook configuration.
🤖 Prompt for AI Agents
In CODING_STYLE.md around lines 45 to 48, clarify the manual invocation of the
clang-tidy hook by specifying the exact hook id in the example command. Update
the documentation to show the command as "pre-commit run --hook-stage=manual
clang-tidy" to align with the actual hook configuration and improve clarity.
| -abseil-*, | ||
| -android-*, | ||
| -cert-err58-cpp, | ||
| -cert-err58-cpp, | ||
| -clang-analyzer-osx-*, |
There was a problem hiding this comment.
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.
| HeaderFilterRegex: '.*' | ||
|
|
||
| WarningsAsErrors: '' | ||
|
|
There was a problem hiding this comment.
🧹 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.
| Checks: > | ||
| *, | ||
| -abseil-*, | ||
| -android-*, | ||
| -cert-err58-cpp, | ||
| -cert-err58-cpp, | ||
| -clang-analyzer-osx-*, | ||
| -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, |
There was a problem hiding this comment.
🛠️ 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.
| aliases: dict[str, str] = {}, | ||
| subfields: dict[str, list[int]] = {}, | ||
| row_index: str = "", | ||
| connector_id: str = "prism", | ||
| input_files: List[File] = [], | ||
| input_files: list[File] = [], | ||
| ) -> PlanBuilder: ... |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid mutable default arguments in stubs
Using {} and [] as defaults can be misleading; consider switching to None with Optional[...] for clarity:
aliases: Optional[dict[str, str]] = None
subfields: Optional[dict[str, list[int]]] = None
input_files: Optional[list[File]] = None🤖 Prompt for AI Agents
In velox/python/plan_builder/plan_builder.pyi around lines 41 to 46, mutable
default arguments like {} and [] are used, which can lead to unexpected
behavior. Change these defaults to None and update the type hints to
Optional[...] accordingly for aliases, subfields, and input_files to clearly
indicate that the default is no value.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/linux-build-base.yml (3)
31-33: Quote environment variable valuesWrap complex values in quotes to ensure proper YAML parsing and avoid potential ambiguity.
- CCACHE_DIR: ${{ github.workspace }}/ccache - USE_CLANG: ${{ inputs.use-clang && 'true' || 'false' }} - STASH_KEY: velox-ccache-ubuntu-release-${{ inputs.use-clang && 'clang' || 'gcc' }}-${{ replace(github.repository, '/', '-') }}-${{ replace(github.ref_name, '/', '-') }} + CCACHE_DIR: "${{ github.workspace }}/ccache" + USE_CLANG: "${{ inputs.use-clang && 'true' || 'false' }}" + STASH_KEY: "velox-ccache-ubuntu-release-${{ inputs.use-clang && 'clang' || 'gcc' }}-${{ replace(github.repository, '/', '-') }}-${{ replace(github.ref_name, '/', '-') }}"
57-59: Split the concatenated step nameThe step name
Clear CCache StatisticsMore actionsis malformed; split into a proper name and ensure the run block is correctly indented.- - name: Clear CCache StatisticsMore actions + - name: Clear CCache Statistics run: | ccache -sz
65-65: QuoteMAKEFLAGSvalueWrap the
MAKEFLAGSstring to avoid misinterpretation.- MAKEFLAGS: MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3 + MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build-base.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build-base.yml
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)
33-33: undefined function "replace". available functions are "always", "cancelled", "contains", "endswith", "failure", "format", "fromjson", "hashfiles", "join", "startswith", "success", "tojson"
(expression)
🔇 Additional comments (7)
.github/workflows/linux-build-base.yml (7)
21-21: Approve user-friendly input descriptionThe
use-clangdescription clearly communicates its purpose.
28-28: Job name is descriptiveRenaming to “Ubuntu release with resolve_dependency” aligns with the job’s scope.
31-33: Verifyreplaceusage in expressionsActionlint flags
replaceas undefined. Confirm that GitHub Actions supportsreplace()here or adjust to a supported function.
39-42: Approve checkout step positioningMoving
actions/checkout@v4withpersist-credentials: falseto the top ensures a clean workspace before credential loading.
47-47: Approve stash directory creationCreating the ccache directory upfront avoids errors in subsequent steps.
68-69: Verify build target changeConfirm that switching from
make releasetomake minimalcovers all intended artefacts and that it doesn’t reduce test coverage accidentally.
75-77: Approve cache save stepSaving the ccache stash after the build maintains efficiency for future runs.
| - name: Get Release Build Ccache Stash | ||
| run: | | ||
| ~/yscache/restore.sh "$STASH_KEY" "$CCACHE_DIR" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using $HOME instead of ~
Using $HOME/yscache/restore.sh improves portability in non-interactive shells.
- ~/yscache/restore.sh "$STASH_KEY" "$CCACHE_DIR"
+ "$HOME/yscache/restore.sh" "$STASH_KEY" "$CCACHE_DIR"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Get Release Build Ccache Stash | |
| run: | | |
| ~/yscache/restore.sh "$STASH_KEY" "$CCACHE_DIR" | |
| - name: Get Release Build Ccache Stash | |
| run: | | |
| "$HOME/yscache/restore.sh" "$STASH_KEY" "$CCACHE_DIR" |
🤖 Prompt for AI Agents
In .github/workflows/linux-build-base.yml around lines 49 to 51, replace the
tilde (~) with $HOME in the path to the restore.sh script to improve portability
in non-interactive shells. Change the script path from ~/yscache/restore.sh to
$HOME/yscache/restore.sh in the run command.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
.github/workflows/linux-build-base.yml (5)
31-32: Quote environment variable expressions.
Wrap environment values in double quotes to prevent YAML parsing issues, for example:-env: - CCACHE_DIR: ${{ github.workspace }}/ccache - USE_CLANG: ${{ inputs.use-clang && 'true' || 'false' }} +env: + CCACHE_DIR: "${{ github.workspace }}/ccache" + USE_CLANG: "${{ inputs.use-clang && 'true' || 'false' }}"
59-59: Use$HOMEinstead of~for script paths.
Replace~with$HOMEto ensure correct expansion in non-interactive shells:- ~/yscache/restore.sh "$STASH_KEY" "$CCACHE_DIR" + "$HOME/yscache/restore.sh" "$STASH_KEY" "$CCACHE_DIR"
65-67: Fix malformed step name.
Split the concatenated name to clarify the step title:- - name: Clear CCache StatisticsMore actions + - name: Clear CCache Statistics
73-73: QuoteMAKEFLAGSvalue.
Enclose the complexMAKEFLAGSstring in quotes to prevent misinterpretation:- MAKEFLAGS: MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3 + MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3"
85-85: Use$HOMEinstead of~for script paths.
Align with the restore step and ensure consistent path expansion:- ~/yscache/save.sh "$STASH_KEY" "$CCACHE_DIR" + "$HOME/yscache/save.sh" "$STASH_KEY" "$CCACHE_DIR"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build-base.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build-base.yml
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)
🪛 GitHub Actions: Run Checks
.github/workflows/linux-build-base.yml
[error] 43-50: Zizmor audit detected code injection via template expansion. github.ref_name may expand into attacker-controllable code in the 'Prepare Stash Key' step.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (1)
.github/workflows/linux-build-base.yml (1)
38-41: Review of checkout step configuration.
Disablingpersist-credentialsis a good security practice if no write operations on the repo are needed later. Remember to enable it if pushing back to the repository in future steps.
| - name: Prepare Stash Key | ||
| run: | | ||
| SANITIZED_REPO="${{ github.repository }}" | ||
| SANITIZED_REPO="${SANITIZED_REPO//\//-}" | ||
| SANITIZED_REF_NAME="${{ github.ref_name }}" | ||
| SANITIZED_REF_NAME="${SANITIZED_REF_NAME//\//-}" | ||
| STASH_KEY="velox-ccache-ubuntu-release-${{ inputs.use-clang && 'clang' || 'gcc' }}-${SANITIZED_REPO}-${SANITIZED_REF_NAME}" | ||
| echo "STASH_KEY=$STASH_KEY" >> $GITHUB_ENV |
There was a problem hiding this comment.
Sanitize stash key components to prevent injection.
Using github.ref_name directly can enable code injection via template expansion. Restrict characters to alphanumerics, hyphens and underscores:
- SANITIZED_REPO="${{ github.repository }}"
- SANITIZED_REPO="${SANITIZED_REPO//\//-}"
+ SANITIZED_REPO=$(echo "${{ github.repository }}" | sed 's/[^A-Za-z0-9_-]/-/g')
- SANITIZED_REF_NAME="${{ github.ref_name }}"
- SANITIZED_REF_NAME="${SANITIZED_REF_NAME//\//-}"
+ SANITIZED_REF_NAME=$(echo "${{ github.ref_name }}" | sed 's/[^A-Za-z0-9_-]/-/g')🧰 Tools
🪛 GitHub Actions: Run Checks
[error] 43-50: Zizmor audit detected code injection via template expansion. github.ref_name may expand into attacker-controllable code in the 'Prepare Stash Key' step.
🤖 Prompt for AI Agents
In .github/workflows/linux-build-base.yml around lines 43 to 50, sanitize the
stash key components by restricting github.ref_name and github.repository to
only alphanumeric characters, hyphens, and underscores to prevent code
injection. Replace any disallowed characters in these variables with safe
characters (e.g., hyphens) before constructing the STASH_KEY. This ensures the
stash key is safe for use in the environment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
.github/workflows/linux-build-base.yml (5)
31-33: Quote environment variable expressions for YAML parsingThe
CCACHE_DIRandUSE_CLANGvalues should be wrapped in quotes to prevent parsing issues:- CCACHE_DIR: ${{ github.workspace }}/ccache - USE_CLANG: ${{ inputs.use-clang && 'true' || 'false' }} + CCACHE_DIR: "${{ github.workspace }}/ccache" + USE_CLANG: "${{ inputs.use-clang && 'true' || 'false' }}"
73-73: Quote complexMAKEFLAGSstringUnquoted
MAKEFLAGSmay be misinterpreted. Wrap the value in quotes:- MAKEFLAGS: MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3 + MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3"
85-85: Avoid using~in scripts pathSame here: prefer
$HOMEover~in CI environments:- ~/yscache/save.sh "$STASH_KEY" "$CCACHE_DIR" + "$HOME/yscache/save.sh" "$STASH_KEY" "$CCACHE_DIR"
59-59: Avoid using~in scripts pathPaths like
~/yscache/restore.shmay not resolve in non-interactive shells. Use$HOMEinstead:- ~/yscache/restore.sh "$STASH_KEY" "$CCACHE_DIR" + "$HOME/yscache/restore.sh" "$STASH_KEY" "$CCACHE_DIR"
43-51: Sanitize stash key components to prevent injectionUsing
github.ref_nameandgithub.repositorydirectly can allow code injection via template expansion. Restrict these values to alphanumeric characters, hyphens, and underscores:- SANITIZED_REPO="${{ github.repository }}" - SANITIZED_REPO="${SANITIZED_REPO//\//-}" - SANITIZED_REF_NAME="${{ github.ref_name }}" - SANITIZED_REF_NAME="${SANITIZED_REF_NAME//\//-}" + SANITIZED_REPO=$(echo "${{ github.repository }}" | sed 's/[^A-Za-z0-9_-]/-/g') + SANITIZED_REF_NAME=$(echo "${{ github.ref_name }}" | sed 's/[^A-Za-z0-9_-]/-/g') STASH_KEY="velox-ccache-ubuntu-release-${{ inputs.use-clang && 'clang' || 'gcc' }}-${SANITIZED_REPO}-${SANITIZED_REF_NAME}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build-base.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build-base.yml
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)
🪛 GitHub Actions: Run Checks
.github/workflows/linux-build-base.yml
[error] 43-50: zizmor: code injection via template expansion detected. github.ref_name may expand into attacker-controllable code in step 'Prepare Stash Key'. Audit confidence: High.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (2)
.github/workflows/linux-build-base.yml (2)
79-82: Display cache stats after buildIncluding the
ccache -vsstep post-build is a good practice to inspect cache efficiency.
76-77: ```shell
#!/bin/bash
set -eLook for a 'release' target in the root Makefile
if rg "^[[:space:]]*release:" -n Makefile; then
echo "Found 'release' target"
else
echo "No 'release' target in Makefile"
fiLook for a 'minimal' target in the root Makefile
if rg "^[[:space:]]*minimal:" -n Makefile; then
echo "Found 'minimal' target"
else
echo "No 'minimal' target in Makefile"
fi</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
.github/workflows/preliminary_checks.yml (1)
39-42: Fetch full history for accurate diffs.
fetch-depth: 2can omit earlier commits, leading to incomplete diffs when generating the file list. Usefetch-depth: 0to fetch the full history..github/workflows/linux-build-base.yml (1)
55-55: Quote theMAKEFLAGSstring.Unquoted multi-value env vars can be misinterpreted. Enclose
MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3in double quotes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/linux-build-base.yml(1 hunks).github/workflows/preliminary_checks.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build-base.yml
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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (1)
.github/workflows/preliminary_checks.yml (1)
52-56: Ensure pre-commit targets all PR changes.
git diff --name-only HEAD^1 HEADonly covers the latest commit. For multi-commit PRs, diff against the base ref (e.g.,origin/${{ github.event.pull_request.base.ref }}...HEAD) to capture every changed file.
| CCACHE_CONFIGPATH: ~/yscache/pre-commit/ccache.conf | ||
| steps: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use $HOME instead of ~ in CCACHE_CONFIGPATH.
The tilde (~) may not expand in non-interactive shells. Replace it with $HOME for portability.
🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml at lines 35 to 36, replace the tilde
(~) in the CCACHE_CONFIGPATH environment variable with $HOME to ensure proper
path expansion in non-interactive shells. Change the value from
'~/yscache/pre-commit/ccache.conf' to '$HOME/yscache/pre-commit/ccache.conf' for
better portability.
| CCACHE_DIR: "${{ github.workspace }}/ccache" | ||
| USE_CLANG: "${{ inputs.use-clang && 'true' || 'false' }}" | ||
| CCACHE_CONFIGPATH: ~/yscache/velox-ubuntu-release-${{ inputs.use-clang && 'clang' || 'gcc' }}/ccache.conf | ||
| USE_CLANG: ${{ inputs.use-clang && 'true' || 'false' }} |
There was a problem hiding this comment.
🧹 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.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
.github/workflows/linux-build-base.yml (4)
31-31: Use$HOMEinstead of~and quoteCCACHE_DIR
Tilde expansion isn’t guaranteed in YAML. Replace~/yscache/...with$HOME/yscache/...and wrap the entire value in double quotes to ensure reliable path expansion.
33-33: Quote theUSE_CLANGexpression
Wrapping${{ inputs.use-clang && 'true' || 'false' }}in double quotes prevents YAML parsing edge cases.
56-56: Quote complexMAKEFLAGSstring
The unquotedMAKEFLAGSvalue can be misinterpreted. Enclose it in double quotes or switch to a list for clarity:MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3"
59-60: Clean up commented-outmake release
Either remove the#make releaseline or add a TODO explaining why it’s disabled to keep the workflow tidy.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build-base.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build-base.yml
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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (3)
.github/workflows/linux-build-base.yml (3)
42-42: Securely disable credential persistence
Explicitly settingpersist-credentials: falsein the checkout step is a strong security practice to avoid leaking GitHub tokens.
48-51: Add Clear CCache Statistics step
Including a dedicated step to clear previous ccache statistics helps maintain consistent cache metrics across builds.
62-64: Display post-build ccache statistics
Runningccache -vsafter the build provides visibility into cache performance and hit rates.
| ubuntu-release: | ||
| name: "Ubuntu release with resolve_dependency" | ||
| name: Ubuntu release with resolve_dependency | ||
| runs-on: yscope-gh-runner |
There was a problem hiding this comment.
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.
fefbc4b to
16e8eaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
.github/workflows/preliminary_checks.yml (2)
49-58: Install system dependencies required by C/C++ hooks
Your.pre-commit-config.yamlincludesclang-formatandclang-tidyhooks, but the runner doesn’t install those binaries by default. Add a step to install them before runningpre-commit.Suggested addition:
- name: Install C/C++ tools run: | sudo apt-get update sudo apt-get install -y clang-format clang-tidy
53-58: Make file diff detection more robust
Usinggit diff HEAD^1 HEADcan break in merge commits or nested PR workflows. Consider comparing against the base branch or using thegithub.event.beforeandgithub.shacontext variables.Example:
- files=$(git diff --name-only HEAD^1 HEAD) + files=$(git diff --name-only ${{ github.event.before }} ${{ github.sha }})
♻️ Duplicate comments (7)
.github/workflows/preliminary_checks.yml (1)
37-40: Increasefetch-depthfor accurate diffs
Withfetch-depth: 2, older commits may be omitted and some file changes can be missed. Usefetch-depth: 0to fetch the full history..github/workflows/linux-build-base.yml (6)
46-50: Use$HOMEinstead of~for portability.Tilde expansion isn’t guaranteed in non-interactive shells. Replace
~with$HOME:- cp -r ~/yscache/velox-ubuntu-release-"${{ inputs.use-clang && 'clang' || 'gcc' }}"/ccache "$CCACHE_DIR" + cp -r "$HOME/yscache/velox-ubuntu-release-${{ inputs.use-clang && 'clang' || 'gcc' }}/ccache" "$CCACHE_DIR"
64-64: Quote theMAKEFLAGSstring.Wrap the complex
MAKEFLAGSvalue in quotes to prevent misinterpretation:- MAKEFLAGS: MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3 + MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3"
67-68: Address commented-out build command.Either remove
#make releaseor add aTODOexplaining why it’s disabled to keep the workflow clean.
74-77: Consistent stash save path.Also replace
~with$HOMEwhen saving the ccache stash to avoid path-expansion issues.
35-35: Quote boolean expression.Wrap the
USE_CLANGexpression in quotes to ensure it’s treated as a string:- USE_CLANG: ${{ inputs.use-clang && 'true' || 'false' }} + USE_CLANG: "${{ inputs.use-clang && 'true' || 'false' }}"
31-32: Quote environment variable values.To avoid YAML parsing edge cases, wrap complex values in quotes:
- CCACHE_DIR: ${{ github.workspace }}/ccache - CCACHE_CONFIGPATH: ${{ github.workspace }}/ccache.conf + CCACHE_DIR: "${{ github.workspace }}/ccache" + CCACHE_CONFIGPATH: "${{ github.workspace }}/ccache.conf"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/linux-build-base.yml(1 hunks).github/workflows/preliminary_checks.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build-base.yml
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)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build-base.yml
[error] 33-33: trailing spaces
(trailing-spaces)
[warning] 34-34: missing starting space in comment
(comments)
🪛 GitHub Actions: Run Checks
.github/workflows/linux-build-base.yml
[error] 31-31: trailing-whitespace hook failed and fixed trailing whitespace issues in this file. Command exited with code 1.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (5)
.github/workflows/linux-build-base.yml (5)
21-21: Well-defined input description.The
use-clanginput has a clear description aligning with its boolean type.
28-28: Clarify job name.The custom job
namereflects its purpose; it’s clear and descriptive.
29-29: Validateruns-onrunner label.The
yscope-gh-runnerlabel is not a standard GitHub Actions runner. If this is a self-hosted runner, prependself-hosted(e.g.,runs-on: [self-hosted, yscope-gh-runner]), or switch to a valid hosted runner.
44-44: Disable credential persistence.Explicitly disabling credential persistence is a good security practice for downstream steps.
70-73: Good visibility of cache stats.Adding a
ccache -vsstep after the build helps in monitoring cache effectiveness.
| name: Run Checks | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: | ||
| - opened | ||
| - reopened | ||
| - edited | ||
| - synchronize | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding manual and push triggers
Currently, this workflow only runs on pull request events. It may be useful to allow manual (workflow_dispatch) or push triggers to test formatting and linting on branches before opening a PR.
🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml around lines 14 to 23, the workflow
triggers only on pull request events. To enable manual runs and trigger on push
events, add `workflow_dispatch` and `push` under the `on` section. This allows
running the workflow manually and on branch pushes for earlier feedback on
formatting and linting.
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.repository }}-${{ github.head_ref || github.sha }} | ||
| cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify cancel-in-progress logic
The special-case branch exclusion (presto-0.293-clp-connector) is unusual and will need updates for every new branch. To ensure in-flight runs are always cancelled on new pushes, you can set cancel-in-progress: true.
🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml around lines 27 to 30, the
cancel-in-progress field uses a special-case condition to exclude a specific
branch, which complicates maintenance. Replace the conditional expression with a
simple boolean true to always cancel in-progress runs on new pushes, removing
the branch-specific exception.
| - run: python -m pip install pre-commit | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Pin the pre-commit version for reproducibility
Installing the latest pre-commit by default can introduce breaking changes unexpectedly. Lock to a tested version (e.g. 2.20.0) or a version range.
Proposed diff:
- - run: python -m pip install pre-commit
+ - run: python -m pip install "pre-commit>=2.20,<3.0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - run: python -m pip install pre-commit | |
| - run: python -m pip install "pre-commit>=2.20,<3.0" |
🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml at lines 35 to 36, the pre-commit
installation command installs the latest version without version pinning, which
can cause unexpected breaking changes. Modify the pip install command to specify
a fixed version of pre-commit, such as 2.20.0, by changing it to "python -m pip
install pre-commit==2.20.0" to ensure reproducibility and stability.
| - uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 | ||
| with: | ||
| path: ~/.cache/pre-commit | ||
| key: pre-commit-${{ hashFiles('.pre-commit-config.yaml') }} | ||
| restore-keys: | | ||
| pre-commit- | ||
|
|
||
| - name: Run pre-commit |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use semver tags for actions/cache
Referencing a commit SHA pins you forever and can be confusing when upgrading. Switch to the latest stable version tag (e.g. actions/cache@v3).
Example:
- - uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3
+ - uses: actions/cache@v4.2.3📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 | |
| with: | |
| path: ~/.cache/pre-commit | |
| key: pre-commit-${{ hashFiles('.pre-commit-config.yaml') }} | |
| restore-keys: | | |
| pre-commit- | |
| - name: Run pre-commit | |
| - uses: actions/cache@v4.2.3 | |
| with: | |
| path: ~/.cache/pre-commit | |
| key: pre-commit-${{ hashFiles('.pre-commit-config.yaml') }} | |
| restore-keys: | | |
| pre-commit- | |
| - name: Run pre-commit |
🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml around lines 42 to 49, the
actions/cache step uses a commit SHA to reference the action, which makes
upgrades confusing and pins the workflow indefinitely. Replace the commit SHA
with a stable semver tag like 'actions/cache@v3' to ensure clarity and easier
maintenance.
| - name: Clear CCache Statistics | ||
| run: | | ||
| ccache -sz | ||
|
|
There was a problem hiding this comment.
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.
| CCACHE_DIR: ${{ github.workspace }}/ccache | ||
| CCACHE_CONFIGPATH: ${{ github.workspace }}/ccache.conf | ||
|
|
||
| #CCACHE_CONFIGPATH: ~/yscache/velox-ubuntu-release-${{ inputs.use-clang && 'clang' || 'gcc' }}/ccache.conf |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add space after comment marker.
YAML style requires a space after # for readability:
- #CCACHE_CONFIGPATH: ...
+ # CCACHE_CONFIGPATH: ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #CCACHE_CONFIGPATH: ~/yscache/velox-ubuntu-release-${{ inputs.use-clang && 'clang' || 'gcc' }}/ccache.conf | |
| # CCACHE_CONFIGPATH: ~/yscache/velox-ubuntu-release-${{ inputs.use-clang && 'clang' || 'gcc' }}/ccache.conf |
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 34-34: missing starting space in comment
(comments)
🤖 Prompt for AI Agents
In .github/workflows/linux-build-base.yml at line 34, the comment line starting
with # does not have a space after the # symbol. Add a single space after the #
to comply with YAML style guidelines for better readability.
| CCACHE_CONFIGPATH: ${{ github.workspace }}/ccache.conf | ||
| CCACHE_DIR: ${{ github.workspace }}/ccache |
There was a problem hiding this comment.
We can configure ccache using environment variables, to avoid needing to set up the runner with a config file. I'll explain in the other comment why we don't need to set CCACHE_DIR.
| CCACHE_CONFIGPATH: ${{ github.workspace }}/ccache.conf | |
| CCACHE_DIR: ${{ github.workspace }}/ccache | |
| CCACHE_COMPRESSLEVEL: 2 | |
| CCACHE_MAX_SIZE: 5G |
| - name: Get Release Build Ccache Stash | ||
| run: | | ||
| rm -rf "$CCACHE_DIR" | ||
| cp -r ~/yscache/velox-ubuntu-release-"${{ inputs.use-clang && 'clang' || 'gcc' }}"/ccache "$CCACHE_DIR" | ||
| cp ~/yscache/velox-ubuntu-release-"${{ inputs.use-clang && 'clang' || 'gcc' }}"/ccache.conf "$CCACHE_CONFIGPATH" |
There was a problem hiding this comment.
Instead of moving these directories around, we can just let ccache fully manage the cache:
- ccache already handles keeping track of which compiler a file was built with, so we don't need to do that ourselves.
- I guess the reason you want to only "commit" the cache is if the build is successful. However, let's say a source file gets cached even though the build fails; that cache entry will either get reused in the future or eventually get evicted because it's not used frequently, right?
There was a problem hiding this comment.
For the first one I initially direclty set the CCACHE_DIR to the ~/yscache/velox-ubuntu-release-"${{ inputs.use-clang && 'clang' || 'gcc' }}"/ccache, however, even I put the same cache contents there, it cannot detect any existing cache. But if I put the cache at ${{ github.workspace }}/ccache, then it can be detected. I don't know why, but I can try to just use the default ccache dir.
For the second, yea, I agree. Didn't realize this before because my thought was limited by the current Velox's workflows, which is not designed for a consistent usage but for per PR and saving disk space.
| - opened | ||
| - reopened | ||
| - edited | ||
| - synchronize |
There was a problem hiding this comment.
| - synchronize | |
| - synchronize | |
| push: |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
.github/workflows/linux-build-base.yml (2)
56-56: Quote the complex MAKEFLAGS string
Unquoted values with spaces may be parsed incorrectly. Wrap the string in quotes:- MAKEFLAGS: MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3 + MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3"
48-51: Ensure ccache directory exists before clearing stats
Theccache -szcommand will fail if the default cache directory doesn’t exist. Add a step or inline command to create it, for example:+ - run: mkdir -p "$HOME/.ccache" - name: Clear CCache Statistics run: | ccache -sz.github/workflows/preliminary_checks.yml (2)
36-36: Pinpre-commitversion for reproducibility
Installing the latestpre-commitcan introduce breaking changes. Lock to a tested range, for example:- - run: python -m pip install pre-commit + - run: python -m pip install "pre-commit>=2.20,<3.0"
43-43: Use semver tag foractions/cache
Referencing a commit SHA pins the action indefinitely. Switch to a stable tag, for example:- - uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 + - uses: actions/cache@v4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/linux-build-base.yml(1 hunks).github/workflows/preliminary_checks.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build-base.yml
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)
🪛 GitHub Actions: Run Checks
.github/workflows/preliminary_checks.yml
[error] 23-23: yamllint: empty value in block mapping (empty-values)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (5)
.github/workflows/linux-build-base.yml (4)
21-21: Looks good: input description
Theuse-clangdescription is clear and correctly documented.
31-33: Environment variables configuration
The newCCACHE_COMPRESSLEVEL,CCACHE_MAX_SIZE, andUSE_CLANGentries simplify ccache and clang usage under CI. Ensure these values match your intended cache strategy and build requirements.
42-42: Explicitly disable credential persistence
Settingpersist-credentials: falsefor the checkout step is a good security practice to avoid leaking tokens to subsequent steps.
61-64: Display ccache stats after build
The addition of theccache -vsstep improves visibility into cache usage and hit rates..github/workflows/preliminary_checks.yml (1)
14-14: Validate workflow naming
TheRun Checksname accurately reflects the job’s purpose.
| jobs: | ||
| ubuntu-release: | ||
| name: "Ubuntu release with resolve_dependency" | ||
| name: Ubuntu release with resolve_dependency |
There was a problem hiding this comment.
🧹 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.
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.repository }}-${{ github.head_ref || github.sha }} | ||
| cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify cancel-in-progress logic
The conditional exception for presto-0.293-clp-connector complicates maintenance. Use cancel-in-progress: true to always cancel in-flight runs on new pushes.
🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml at line 30, simplify the
cancel-in-progress setting by replacing the conditional expression with a direct
boolean true value. Change the line to "cancel-in-progress: true" to always
cancel in-progress workflow runs on new pushes, removing the special case for
the 'presto-0.293-clp-connector' branch.
| pull_request: | ||
| types: | ||
| - opened | ||
| - reopened | ||
| - edited | ||
| - synchronize | ||
| push: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enable manual and branch push triggers
Consider adding workflow_dispatch under on, and explicitly list branch patterns under push, to allow manual runs and early feedback on non-PR branches.
🧰 Tools
🪛 GitHub Actions: Run Checks
[error] 23-23: yamllint: empty value in block mapping (empty-values)
🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml around lines 17 to 23, the workflow
triggers lack manual trigger support and explicit branch push triggers. Add
`workflow_dispatch` under the `on` section to enable manual runs, and specify
branch patterns explicitly under `push` to allow early feedback on non-PR
branches. This will improve flexibility and coverage of the workflow execution.
| fetch-depth: 2 | ||
| persist-credentials: false |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fetch full history for accurate diffs
With fetch-depth: 2, earlier commits might be omitted, causing incomplete diffs for pre-commit. Use fetch-depth: 0 to fetch the full history.
🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml at lines 40 to 41, the fetch-depth
is set to 2, which limits the git history and can cause incomplete diffs for
pre-commit checks. Change the fetch-depth value from 2 to 0 to fetch the full
git history and ensure accurate diffs.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
.github/workflows/preliminary_checks.yml (5)
17-23: Consider adding manual and branch‐push triggers
To enable early feedback and ad-hoc runs, you can includeworkflow_dispatchunderonand list branch patterns explicitly underpush.
28-31: Simplify the cancel-in-progress logic
Replace the conditional exception with a booleantrueso all in-flight runs are cancelled on new pushes, removing the special‐case branch.
36-36: Pin the pre-commit version for reproducibility
Installing the latest pre-commit can introduce unexpected breaking changes. Lock to a tested version, e.g.:-run: python -m pip install pre-commit +run: python -m pip install "pre-commit>=2.20,<3.0"
38-41: Fetch full Git history for accurate diffs
Withfetch-depth: 2, only the last two commits are retrieved, which may miss changes outside the most recent commit. Consider:- fetch-depth: 2 + fetch-depth: 0
43-49: Use semver tag for actions/cache
Referencing a commit SHA makes upgrades confusing. Switch to a stable semver tag, e.g.:- - uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 + - uses: actions/cache@v4.2.3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/preliminary_checks.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
|
|
||
| - uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 | ||
| with: | ||
| path: ~/.cache/pre-commit |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure proper path expansion in cache step
Tildes may not expand in non-interactive shells. Replace ~/.cache/pre-commit with:
path: $HOME/.cache/pre-commit🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml at line 45, replace the path value
from '~/.cache/pre-commit' to '$HOME/.cache/pre-commit' to ensure proper
environment variable expansion in the cache step, as tildes may not expand
correctly in non-interactive shells.
| # Copyright (c) Facebook, Inc. and its affiliates. | ||
| # | ||
| # Licensed 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. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Update license header to match repository ownership.
The current header still references Facebook, Inc.; please update it to reflect the correct project or organisation name and ensure consistency with other files in this repository.
🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml lines 1 to 13, the license header
references Facebook, Inc., which is outdated. Update the license header to
reflect the current project or organization name consistent with other files in
the repository by replacing "Facebook, Inc. and its affiliates" with the correct
entity name.
| files=$(git diff --name-only HEAD^1 HEAD) | ||
| echo "::group::Changed files" | ||
| echo $files | tr ' ' '\n' | ||
| echo "::endgroup::" | ||
| pre-commit run --show-diff-on-failure --color=always --files $files |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Revise diff logic to capture the full PR
The current command only lists files changed in the latest commit. It’s better to diff against the PR base so all changes are linted. For example:
- files=$(git diff --name-only HEAD^1 HEAD)
+ files=$(git diff --name-only "$(git merge-base HEAD origin/${{ github.event.pull_request.base.ref }})" HEAD)This ensures pre-commit runs on every file modified in the pull request.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| files=$(git diff --name-only HEAD^1 HEAD) | |
| echo "::group::Changed files" | |
| echo $files | tr ' ' '\n' | |
| echo "::endgroup::" | |
| pre-commit run --show-diff-on-failure --color=always --files $files | |
| files=$(git diff --name-only "$(git merge-base HEAD origin/${{ github.event.pull_request.base.ref }})" HEAD) | |
| echo "::group::Changed files" | |
| echo $files | tr ' ' '\n' | |
| echo "::endgroup::" | |
| pre-commit run --show-diff-on-failure --color=always --files $files |
🤖 Prompt for AI Agents
In .github/workflows/preliminary_checks.yml around lines 54 to 58, the git diff
command only captures files changed in the latest commit, missing other changes
in the PR. Update the diff command to compare against the PR base branch (e.g.,
origin/main or the merge base) instead of HEAD^1, so it lists all files changed
in the entire pull request. This ensures pre-commit runs on every modified file
in the PR.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
ci: Cherry-pick prestodb/presto#13361 to use `pre-commit` for quality checks; Fix compilation caching by using cache on runner.
kirkrodrigues
changed the title
pre-commit for quality checks and fix ccache.pre-commit for quality checks; Fix compilation caching by using cache on runner.
0251e98
into
y-scope:presto-0.293-clp-connector
…ger-overflow (facebookincubator#13831) Summary: Pull Request resolved: facebookincubator#13831 This avoids the following errors: ``` fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56:41: runtime error: negation of -9223372036854775808 cannot be represented in type 'long'; cast to an unsigned type to negate this value to itself #0 0x000000346ce5 in std::abs(long) fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56 #1 0x000000345879 in std::shared_ptr<facebook::velox::BiasVector<facebook::velox::test::EvalTypeHelper<long>::Type>> facebook::velox::test::VectorMaker::biasVector<long>(std::vector<std::optional<long>, std::allocator<std::optional<long>>> const&) fbcode/velox/vector/tests/utils/VectorMaker-inl.h:58 #2 0x000000344d34 in facebook::velox::test::BiasVectorErrorTest::errorTest(std::vector<std::optional<long>, std::allocator<std::optional<long>>>) fbcode/velox/vector/tests/BiasVectorTest.cpp:39 #3 0x00000033ec99 in facebook::velox::test::BiasVectorErrorTest_checkRangeTooLargeError_Test::TestBody() fbcode/velox/vector/tests/BiasVectorTest.cpp:44 #4 0x7fe0a2342c46 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) fbsource/src/gtest.cc:2727 #5 0x7fe0a234275d in testing::Test::Run() fbsource/src/gtest.cc:2744 #6 0x7fe0a2345fb3 in testing::TestInfo::Run() fbsource/src/gtest.cc:2890 #7 0x7fe0a234c8eb in testing::TestSuite::Run() fbsource/src/gtest.cc:3068 #8 0x7fe0a237b52b in testing::internal::UnitTestImpl::RunAllTests() fbsource/src/gtest.cc:6059 #9 0x7fe0a237a0a2 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) fbsource/src/gtest.cc:2727 #10 0x7fe0a23797f5 in testing::UnitTest::Run() fbsource/src/gtest.cc:5599 #11 0x7fe0a2239800 in RUN_ALL_TESTS() fbsource/gtest/gtest.h:2334 #12 0x7fe0a223952c in main fbcode/common/gtest/LightMain.cpp:20 #13 0x7fe09ec2c656 in __libc_start_call_main /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../sysdeps/nptl/libc_start_call_main.h:58:16 #14 0x7fe09ec2c717 in __libc_start_main@GLIBC_2.2.5 /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../csu/libc-start.c:409:3 #15 0x00000033d8b0 in _start /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../sysdeps/x86_64/start.S:116 UndefinedBehaviorSanitizer: signed-integer-overflow fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56:41 ``` Avoid overflow by using the expression (static_cast<uint64_t>(1) + ~static_cast<uint64_t>(min)) to calculate the absolute value of min without using std::abs Reviewed By: dmm-fb, peterenescu Differential Revision: D76901449 fbshipit-source-id: 7eb3bd0f83e42f44cdf34ea1759f3aa9e1042dae
3 participants
Description
As mentioned in #8 , for header and format check, we will apply this commit, which also uses a new way to implement ccache.
Checklist
breaking change.
Validation performed
Passed the CI.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Refactor
Style
Chores
Bug Fixes
Tests