ci: Remove unnecessary workflows and simplify others for efficiency.#8
Conversation
WalkthroughMultiple GitHub Actions workflow files were deleted, removing all related CI/CD jobs for benchmarking, Breeze builds, build metrics collection, Pyvelox wheel building and publishing, Docker image creation, documentation building, experimental fuzzers, Linux base builds, macOS builds, preliminary checks, and scheduled fuzzer orchestration. The documentation workflow was renamed and simplified to only build docs without pushing. The Linux build workflow was modified to trigger exclusively on the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
Developer->>GitHub Actions: Push/PR to `presto-0.293-clp-connector`
GitHub Actions->>GitHub Actions: Start linux-gcc job
GitHub Actions->>GitHub Actions: Restore ccache
GitHub Actions->>GitHub Actions: Checkout code
GitHub Actions->>GitHub Actions: Install dependencies
GitHub Actions->>GitHub Actions: Check license headers & format
GitHub Actions->>GitHub Actions: Build (Debug)
GitHub Actions->>GitHub Actions: Build documentation
GitHub Actions->>GitHub Actions: Save ccache
GitHub Actions->>GitHub Actions: Run tests (ctest)
GitHub Actions-->>Developer: Report results
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
.github/workflows/experimental.yml (1)
57-58: 🧹 Nitpick (assertive)Runner label standardization
Consider using a GitHub-supported runner likeubuntu-latest-16-coresinstead of16-core-ubuntu.🧰 Tools
🪛 actionlint (1.7.7)
57-57: label "16-core-ubuntu" 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)
♻️ Duplicate comments (1)
scripts/docker/spark-container.dockerfile (1)
16-16: Duplicate: verify new base image availabilitySame as above—confirm that
ghcr.io/y-scope/clp/velox-dev:centos9exists and is accessible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/benchmark.yml(1 hunks).github/workflows/breeze.yml(2 hunks).github/workflows/build-metrics.yml(1 hunks).github/workflows/build_pyvelox.yml(2 hunks).github/workflows/docker.yml(6 hunks).github/workflows/docs.yml(2 hunks).github/workflows/experimental.yml(9 hunks).github/workflows/linux-build-base.yml(1 hunks).github/workflows/linux-build.yml(2 hunks).github/workflows/macos.yml(0 hunks).github/workflows/preliminary_checks.yml(1 hunks).github/workflows/scheduled.yml(26 hunks)docker-compose.yml(6 hunks)pyproject.toml(1 hunks)scripts/docker/adapters.dockerfile(1 hunks)scripts/docker/prestojava-container.dockerfile(1 hunks)scripts/docker/spark-container.dockerfile(1 hunks)velox/docs/develop/testing/spark-query-runner.rst(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/macos.yml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/docker.yml
[error] 54-54: trailing spaces
(trailing-spaces)
[warning] 81-81: comment not indented like content
(comments-indentation)
[warning] 124-124: comment not indented like content
(comments-indentation)
.github/workflows/docs.yml
[warning] 100-100: comment not indented like content
(comments-indentation)
🪛 actionlint (1.7.7)
.github/workflows/linux-build-base.yml
33-33: label "8-core-ubuntu-22.04" 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/workflows/docs.yml
39-39: label "8-core-ubuntu" 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/workflows/build_pyvelox.yml
54-54: label "8-core-ubuntu" 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)
🔇 Additional comments (46)
scripts/docker/prestojava-container.dockerfile (1)
16-16: Verify new base image availabilityPlease ensure that
ghcr.io/y-scope/clp/velox-dev:centos9is published and accessible before merging.pyproject.toml (1)
121-121: Verify manylinux image availabilityEnsure that the Linux wheel build image
ghcr.io/y-scope/clp/velox-dev:pyveloxis published and correctly tagged for cibuildwheel.velox/docs/develop/testing/spark-query-runner.rst (1)
60-60: Approve updated Docker image referenceThe docs now correctly point to
ghcr.io/y-scope/clp/velox-dev:spark-server, matching the Dockerfile changes and maintaining consistency..github/workflows/preliminary_checks.yml (2)
29-29: Conditional cancellation logic
Thecancel-in-progressexpression now scopes cancellations to non-connector branches, avoiding unnecessary restarts onpresto-0.293-clp-connector.
35-35: Update container image for check-matrix
Switched toghcr.io/y-scope/clp/velox-dev:checkto align with the new image namespace..github/workflows/breeze.yml (2)
19-19: Restrict push trigger to connector branch
Limiting thepushevent topresto-0.293-clp-connectorensures builds only run for the intended branch.
42-42: Scope concurrency cancellation
Conditionalcancel-in-progressprevents redundant Breeze runs on the connector branch..github/workflows/linux-build.yml (2)
19-19: Restrict push trigger for Linux build
Thepushevent is now limited to thepresto-0.293-clp-connectorbranch, matching the other workflows.
52-52: Conditional concurrency cancellation
Cancelling in-progress Linux builds only on non-connector refs prevents unnecessary rebuilds.docker-compose.yml (6)
24-24: Update ubuntu-cpp service image namespace
Changed toghcr.io/y-scope/clp/velox-dev:ubuntu-22.04for consistency with CI workflows.
46-52: Update adapters-cpp service image and build args
Switched both theimageand buildargs.imageto they-scope/clpnamespace.
72-78: Update adapters-cuda service image and build args
Aligned CUDA adapter image references with the new namespace.
104-104: Update centos-cpp service image
Now referencesghcr.io/y-scope/clp/velox-dev:centos9.
125-125: Update presto-java service image
Switched toghcr.io/y-scope/clp/velox-dev:presto-javato match repository changes.
146-146: Update spark-server service image
Updated toghcr.io/y-scope/clp/velox-dev:spark-serverfor consistency..github/workflows/linux-build-base.yml (2)
26-29: Refine concurrency management
Adds a workflow-level concurrency group with conditional cancellation to reduce redundant runs on non-connector branches.
34-34: Use updated adapters container image
Switched theadaptersjob toghcr.io/y-scope/clp/velox-dev:adaptersfor alignment with the new namespace..github/workflows/docker.yml (5)
25-25: Branch trigger updated topresto-0.293-clp-connector
This restricts pushes to only the specified branch. Ensure this aligns with release workflow requirements.
34-34: Conditional concurrency cancellation
Using${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}prevents cancelling runs on the connector branch; this matches other workflows.
50-61: Docker image namespace updated
Image tags were changed fromfacebookincubatortoy-scope/clp, ensuring consistency across CI.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 54-54: trailing spaces
(trailing-spaces)
81-83: Simplified push condition
Removed the repository check and now only blocks pushes on pull requests, broadening valid build contexts.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 81-81: comment not indented like content
(comments-indentation)
124-126: Aligned push condition in dependent job
Mirrors the primary job’s logic for consistency and avoids redundant code.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 124-124: comment not indented like content
(comments-indentation)
.github/workflows/docs.yml (3)
32-35: Conditional concurrency cancellation added
Concurrency now skips cancelling for thepresto-0.293-clp-connectorbranch, aligning with other workflows.
39-41: Container image updated
Switched toghcr.io/y-scope/clp/velox-dev:centos9to use the new namespace.🧰 Tools
🪛 actionlint (1.7.7)
39-39: label "8-core-ubuntu" 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)
99-102: Simplified documentation push step
Removed the repository-specific filter so docs update on any push, and updated theifaccordingly.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 100-100: comment not indented like content
(comments-indentation)
.github/workflows/build_pyvelox.yml (2)
33-39: Trigger on PR and push for workflow file
Enabling both events for changes to this workflow file ensures seamless CI updates.
43-46: Conditional concurrency cancellation
Cancelling in-flight runs only for non-connector branches reduces redundant builds..github/workflows/experimental.yml (5)
22-25: Add push trigger for experimental workflow
Includingpushon this workflow file ensures runs on direct edits.
28-31: Default checkout ref updated
Changing the default frommaintopresto-0.293-clp-connectoraligns with branch conventions.
51-54: Implement workflow-level concurrency
Grouping by branch/commit and conditionally cancelling matches your cancellation strategy.
78-79: Consistent checkout refs across jobs
All jobs uniformly reference${{ inputs.ref || 'presto-0.293-clp-connector' }}during checkout—good consistency.Also applies to: 139-143, 193-194, 233-235, 273-275
120-122: Update fuzzer container image
Switched the Presto Java fuzzer job to the newy-scope/clpnamespace.🧰 Tools
🪛 actionlint (1.7.7)
120-120: label "16-core-ubuntu" 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/workflows/benchmark.yml (3)
24-25: Enable push events on all branches
Removing the branch filter allows benchmarks on any push. Confirm this broad trigger is intended.
29-32: Refined concurrency cancellation
Cancelling only on branches other thanpresto-0.293-clp-connectorreduces redundant benchmark runs.
39-39: Verify runner label validity
Confirm that8-core-ubuntu-22.04is a valid GitHub Actions runner label or update to a supported one.🧰 Tools
🪛 actionlint (1.7.7)
39-39: label "8-core-ubuntu-22.04" 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/workflows/scheduled.yml (9)
33-33: Synchronize branch filters and default ref topresto-0.293-clp-connector.
The push trigger andworkflow_dispatchdefault have been correctly updated frommaintopresto-0.293-clp-connector. Ensure any downstream workflows or documentation don’t still reference the oldmainbranch.Also applies to: 50-50
75-80: Adjust concurrency to preserve runs on the CLP connector branch.
Thegroupandcancel-in-progresssettings now avoid cancelling jobs onrefs/heads/presto-0.293-clp-connectorwhile still deduplicating on other branches. This aligns with the PR objective of workflow-level concurrency.
149-150: Includevelox_clp_connectorin Git safe directories.
Adding a secondsafe.directoryentry prevents permission errors when running Git inside the container. Confirm this matches all custom checkout paths.
158-163: Checkout and build against the CLP connector workspace.
Theactions/checkoutref fallback andpath: velox_clp_connectorplus updatedworking-directoryensure builds and signature exports run against the correct branch copy.Also applies to: 166-172
176-178: Align signature export filenames with the CLP connector naming.
The exports to/tmp/signatures/*_clp_connector.jsonnow match the branch context. Downstream steps should reference these updated filenames.
185-185: Use branch-specific SHA in stash key.
Caching function signatures by${{ steps.get-head.outputs.head_clp_connector }}isolates stashes per commit, avoiding cross-branch collisions.
231-239: Verify aggregate bias check invocation arguments.
bias_aggregatesnow takes the CLP connector signatures as the baseline, then the contender file, outputting bias functions and errors. Confirm the argument order and output paths cover all intended cases.
255-259: Prune and rename signature artifacts for the CLP connector.
Removing old_signatures_clp_connector.jsonbefore renaming avoids stale artifacts, and the loop correctly maps_contender.jsonto_clp_connector.json.
91-91:Details
❓ Verification inconclusive
Standardize container images to the new
y-scope/clpnamespace.
All jobs have been updated to useghcr.io/y-scope/clp/velox-dev(centos9 or presto-java variants). Verify that these tags (centos9,presto-java,adapters) exist in the registry before merging.Also applies to: 363-363, 438-438, 493-493, 531-531, 577-577, 621-621, 683-683, 721-721, 783-783, 847-847, 883-883, 917-917, 951-951, 1016-1016, 1204-1204, 1269-1269, 1319-1319
🏁 Script executed:
#!/usr/bin/env bash # set -e for tag in centos9 presto-java adapters; do status=$(curl -s -o /dev/null -w '%{http_code}' \ -H "Accept: application/vnd.oci.image.manifest.v1+json" \ "https://ghcr.io/v2/y-scope/clp/velox-dev/manifests/$tag") echo "velox-dev:$tag → HTTP $status" doneLength of output: 860
Confirm GHCR image availability and access
The unauthenticated manifest checks forcentos9,presto-javaandadaptersall returned HTTP 401, indicating the registry requires authentication. Please ensure that:
- The
ghcr.io/y-scope/clp/velox-devimages with tagscentos9,presto-javaandadaptersare published and set to public, or- Your workflows are updated to authenticate with GHCR before pulling these images (e.g. using
docker/login-actionor an authenticated curl:curl -u ${{ secrets.GHCR_USERNAME }}:${{ secrets.GHCR_TOKEN }} \ -H "Accept: application/vnd.oci.image.manifest.v1+json" \ https://ghcr.io/v2/y-scope/clp/velox-dev/manifests/centos9 ```)Verify that a manual
docker pull ghcr.io/y-scope/clp/velox-dev:centos9succeeds in your CI environment before merging..github/workflows/build-metrics.yml (2)
32-34: Introduce concurrency to cancel redundant metrics runs.
The addedconcurrencyblock groups on workflow name + head ref/sha and skips cancelling on the CLP connector branch. This prevents unnecessary reruns and aligns with other workflows.
40-40: Switch metrics container to the CLP namespace.
Updating toghcr.io/y-scope/clp/velox-dev:adaptersmatches the migration. Ensure this image includes all required tools for metrics collection.
| # limitations under the License. | ||
| # Build the test and build container for presto_cpp | ||
| ARG image=ghcr.io/facebookincubator/velox-dev:centos9 | ||
| ARG image=ghcr.io/y-scope/clp/velox-dev:centos9 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider pinning the image to a digest
Using mutable tags can lead to non-reproducible builds. Consider specifying a digest for ghcr.io/y-scope/clp/velox-dev:centos9 to lock the exact image version.
🤖 Prompt for AI Agents
In scripts/docker/adapters.dockerfile at line 15, the Dockerfile uses a mutable
tag for the base image which can cause non-reproducible builds. Replace the
image tag `ghcr.io/y-scope/clp/velox-dev:centos9` with a pinned digest by
specifying the exact image digest (sha256) to ensure the build uses a fixed,
immutable image version.
| head_clp_connector=$(gh api -q '.parents.[0].sha' '/repos/y-scope/velox/commits/${{ github.sha }}') | ||
| else | ||
| head_main=$(gh api -H "Accept: application/vnd.github.sha" /repos/facebookincubator/velox/commits/heads/main) | ||
| head_clp_connector=$(gh api -H "Accept: application/vnd.github.sha" /repos/y-scope/velox/commits/heads/presto-0.293-clp-connector) | ||
| fi | ||
| echo "head_main=$head_main" >> $GITHUB_OUTPUT | ||
| echo "head_clp_connector=$head_clp_connector" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Normalize commit-SHA retrieval for the CLP connector base.
The gh api calls now fetch the parent SHA and head of presto-0.293-clp-connector. Consider adding explicit --repo y-scope/velox flags to both invocations for clarity.
🤖 Prompt for AI Agents
In .github/workflows/scheduled.yml around lines 124 to 128, the gh api calls
retrieving the commit SHA for the CLP connector base lack explicit --repo
y-scope/velox flags. Add --repo y-scope/velox to both gh api commands to clearly
specify the repository context and improve clarity.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/benchmark.yml(0 hunks).github/workflows/breeze.yml(0 hunks).github/workflows/build-metrics.yml(0 hunks).github/workflows/build_pyvelox.yml(0 hunks).github/workflows/docker.yml(0 hunks).github/workflows/docs.yml(0 hunks).github/workflows/experimental.yml(0 hunks).github/workflows/linux-build-base.yml(0 hunks).github/workflows/linux-build.yml(2 hunks).github/workflows/preliminary_checks.yml(0 hunks).github/workflows/scheduled.yml(0 hunks)
💤 Files with no reviewable changes (10)
- .github/workflows/docker.yml
- .github/workflows/docs.yml
- .github/workflows/preliminary_checks.yml
- .github/workflows/experimental.yml
- .github/workflows/build_pyvelox.yml
- .github/workflows/breeze.yml
- .github/workflows/build-metrics.yml
- .github/workflows/scheduled.yml
- .github/workflows/linux-build-base.yml
- .github/workflows/benchmark.yml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build.yml
57-57: label "8-core-ubuntu-22.04" 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)
70-70: property "use-clang" is not defined in object type {}
(expression)
166-166: property "use-clang" is not defined in object type {}
(expression)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build.yml
[error] 94-94: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/linux-build.yml (1)
19-19: Confirm branch trigger scopeThe workflow now runs only on pushes to
presto-0.293-clp-connector. Verify this aligns with expectations—no CI will run onmainor other branches.
| with: | ||
| path: '${{ env.CCACHE_DIR }}' | ||
| key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} | ||
|
|
There was a problem hiding this comment.
Incorrect stash key interpolation
The stash key uses inputs.use-clang, but no job inputs are defined. It should reference the USE_CLANG env var instead.
Suggested fix:
- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }}
+ key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}📝 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.
| with: | |
| path: '${{ env.CCACHE_DIR }}' | |
| key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} | |
| with: | |
| path: '${{ env.CCACHE_DIR }}' | |
| key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }} |
🧰 Tools
🪛 actionlint (1.7.7)
70-70: property "use-clang" is not defined in object type {}
(expression)
🤖 Prompt for AI Agents
In .github/workflows/linux-build.yml around lines 68 to 71, the stash key
incorrectly references inputs.use-clang, which is undefined. Replace
inputs.use-clang with the environment variable USE_CLANG to correctly
interpolate the key. Update the key expression to use the env var syntax for
USE_CLANG instead of inputs.use-clang.
| - name: Install Docs Dependencies | ||
| run: | | ||
| dnf install -y --setopt=install_weak_deps=False pandoc | ||
| pip3 install pipx | ||
| pipx install --force uv cmake==3.31 | ||
| echo "PATH=$PATH:/github/home/.local/bin" >> $GITHUB_ENV |
There was a problem hiding this comment.
Use apt instead of dnf on Ubuntu
Ubuntu runners don’t include dnf. This step will fail. Replace with apt-get (or apt) to install Pandoc.
Suggested diff:
- dnf install -y --setopt=install_weak_deps=False pandoc
+ sudo apt-get update
+ sudo apt-get install -y pandoc📝 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: Install Docs Dependencies | |
| run: | | |
| dnf install -y --setopt=install_weak_deps=False pandoc | |
| pip3 install pipx | |
| pipx install --force uv cmake==3.31 | |
| echo "PATH=$PATH:/github/home/.local/bin" >> $GITHUB_ENV | |
| - name: Install Docs Dependencies | |
| run: | | |
| sudo apt-get update | |
| sudo apt-get install -y pandoc | |
| pip3 install pipx | |
| pipx install --force uv cmake==3.31 | |
| echo "PATH=$PATH:/github/home/.local/bin" >> $GITHUB_ENV |
🤖 Prompt for AI Agents
In .github/workflows/linux-build.yml around lines 85 to 90, the script uses
`dnf` to install pandoc, but Ubuntu runners do not have `dnf`. Replace the `dnf
install -y --setopt=install_weak_deps=False pandoc` command with `apt-get update
&& apt-get install -y pandoc` to correctly install pandoc on Ubuntu.
| - uses: assignUser/stash/save@v1 | ||
| with: | ||
| path: '${{ env.CCACHE_DIR }}' | ||
| key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} | ||
|
|
There was a problem hiding this comment.
Incorrect stash key interpolation on save
Same issue as restore: inputs.use-clang is undefined. Switch to env.USE_CLANG.
- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }}
+ key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}📝 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: assignUser/stash/save@v1 | |
| with: | |
| path: '${{ env.CCACHE_DIR }}' | |
| key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} | |
| - uses: assignUser/stash/save@v1 | |
| with: | |
| path: '${{ env.CCACHE_DIR }}' | |
| key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }} |
🧰 Tools
🪛 actionlint (1.7.7)
166-166: property "use-clang" is not defined in object type {}
(expression)
🤖 Prompt for AI Agents
In .github/workflows/linux-build.yml around lines 163 to 167, the stash key
interpolation uses `inputs.use-clang` which is undefined. Replace
`inputs.use-clang` with `env.USE_CLANG` in the key expression to correctly
reference the environment variable and fix the stash key interpolation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
.github/workflows/linux-build.yml (6)
66-71: 🧹 Nitpick (assertive)Migrate to official cache action.
Switch from
assignUser/stash/restore@v1toactions/cache@v3for better reliability and community support.🧰 Tools
🪛 actionlint (1.7.7)
70-70: property "use-clang" is not defined in object type {}
(expression)
163-167: 🧹 Nitpick (assertive)Migrate save action to official cache.
Replace
assignUser/stash/save@v1withactions/cache@v3to streamline caching.🧰 Tools
🪛 actionlint (1.7.7)
166-166: property "use-clang" is not defined in object type {}
(expression)
69-71:⚠️ Potential issueFix undefined
inputs.use-clangin cache key.
inputs.use-clangis not defined, so the key interpolation will break. Use the environment variable instead:-key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} +key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}🧰 Tools
🪛 actionlint (1.7.7)
70-70: property "use-clang" is not defined in object type {}
(expression)
51-53:⚠️ Potential issueEnsure
cancel-in-progressis a boolean.The expression is currently quoted and will be treated as a string. Remove the quotes so it evaluates correctly:
-concurrency: - group: ${{ github.workflow }}-${{ github.repository }}-${{ github.head_ref || github.sha }} - cancel-in-progress: "${{github.ref != 'refs/heads/presto-0.293-clp-connector'}}" +concurrency: + group: ${{ github.workflow }}-${{ github.repository }}-${{ github.head_ref || github.sha }} + cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}
165-167:⚠️ Potential issueCorrect cache key interpolation on save.
Same fix as restore: reference
env.USE_CLANGinstead ofinputs.use-clang:-key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} +key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}🧰 Tools
🪛 actionlint (1.7.7)
166-166: property "use-clang" is not defined in object type {}
(expression)
85-90:⚠️ Potential issueUse
apt-getinstead ofdnfon Ubuntu.Ubuntu runners don't include
dnf; this step will fail. Replace with:-run: | - dnf install -y --setopt=install_weak_deps=False pandoc +run: | + sudo apt-get update + sudo apt-get install -y pandoc pip3 install pipx pipx install --force uv cmake==3.31
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build.yml
70-70: property "use-clang" is not defined in object type {}
(expression)
166-166: property "use-clang" is not defined in object type {}
(expression)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build.yml
[error] 94-94: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/linux-build.yml (2)
19-19: Confirm branch filter intent.This change restricts push events to only the
presto-0.293-clp-connectorbranch. Please verify that no other branches (e.g.,mainor release branches) require CI on push.
57-58: Approve valid runner label.Switching to
runs-on: ubuntu-22.04aligns with GitHub-hosted runner defaults and resolves previous Actionlint errors.
| runs-on: ubuntu-22.04 | ||
| env: | ||
| CCACHE_DIR: "${{ github.workspace }}/ccache" | ||
| USE_CLANG: "${{ 'false' }}" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify USE_CLANG default assignment.
No expression interpolation is needed for a static default. You can use:
env:
USE_CLANG: 'false'🤖 Prompt for AI Agents
In .github/workflows/linux-build.yml at line 60, simplify the assignment of
USE_CLANG by removing the unnecessary expression interpolation. Replace
USE_CLANG: "${{ 'false' }}" with a direct static string assignment USE_CLANG:
'false' to make the code cleaner and more readable.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
.github/workflows/linux-build.yml (5)
52-52: Remove quoted expression forcancel-in-progress
Quoted values are treated as strings, breaking cancellation behaviour.- cancel-in-progress: "${{github.ref != 'refs/heads/presto-0.293-clp-connector'}}" + cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}
58-60: SimplifyUSE_CLANGdefault assignment
You can assign the static default without interpolation:-env: - USE_CLANG: "${{ 'false' }}" +env: + USE_CLANG: 'false'
75-79: Fix undefinedinputs.use-clangreference
inputs.use-clangisn’t declared—use the env var instead:- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} + key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}🧰 Tools
🪛 actionlint (1.7.7)
78-78: property "use-clang" is not defined in object type {}
(expression)
171-174: Fix stash save key interpolation
Sameinputs.use-clangissue on save—switch toenv.USE_CLANG:- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} + key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}🧰 Tools
🪛 actionlint (1.7.7)
174-174: property "use-clang" is not defined in object type {}
(expression)
93-102: 🧹 Nitpick (assertive)Remove trailing whitespace in commented docs block
YAMLlint flags trailing spaces even in comments.- # uv pip install -r scripts/docs-requirements.txt + # uv pip install -r scripts/docs-requirements.txt🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 102-102: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build.yml
78-78: property "use-clang" is not defined in object type {}
(expression)
174-174: property "use-clang" is not defined in object type {}
(expression)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build.yml
[error] 102-102: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC
🔇 Additional comments (12)
.github/workflows/linux-build.yml (12)
19-19: Verify branch-specific trigger
This workflow now runs only onpresto-0.293-clp-connector. Ensure this branch filter aligns with your intended CI coverage.
57-57: Runner label is valid
ubuntu-22.04is a standard GitHub-hosted runner—no changes needed here.
61-64: Defaults block correctly sets shell and working-directory
Thedefaultsscope properly appliesbashandveloxas the working directory.
80-83: Stash directory creation is correct
Themkdir -p '${{ env.CCACHE_DIR }}'ensures the cache path exists before use.
85-88: Checkout step is properly scoped
Usingwith: path: veloxaligns with the configured working-directory.
89-92: Core dependency installation is sound
Thescripts/setup-ubuntu.sh && install_apt_depssequence correctly installs required packages.
104-107: Clearing CCache statistics is appropriate
Theccache -szstep resets the stats before the build.
108-127: License header check logic looks good
Themake header-fixfollowed by a diff-based failure is well structured.
129-147: Format check logic is solid
Themake format-fixguard with patch instructions strikes the right balance.
150-158: Debug build configuration is correct
Environment flags and conditional clang override are properly defined.
167-170: Displaying CCache stats post-build is useful
Theccache -vssummary helps track cache efficiency across runs.
176-178: Test execution step is solid
Runningctest -j 8 --output-on-failure --no-tests=errorensures parallel tests and proper failure output.
| steps: | ||
| - name: Node Cleanup | ||
| working-directory: ${{ github.workspace }} | ||
| run: | | ||
| df -h | ||
| sudo rm -rf /usr/share/dotnet /usr/local/lib/android /opt/ghc /opt/hostedtoolcache/CodeQL | ||
| sudo docker image prune --all --force | ||
| sudo docker builder prune -a | ||
| df -h | ||
| - name: Get Ccache Stash |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify cleanup step name
The step removes Docker images/builders and other dirs—consider renaming from “Node Cleanup” to “System Cleanup” for accuracy.
- - name: Node Cleanup
+ - name: System Cleanup📝 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.
| steps: | |
| - name: Node Cleanup | |
| working-directory: ${{ github.workspace }} | |
| run: | | |
| df -h | |
| sudo rm -rf /usr/share/dotnet /usr/local/lib/android /opt/ghc /opt/hostedtoolcache/CodeQL | |
| sudo docker image prune --all --force | |
| sudo docker builder prune -a | |
| df -h | |
| - name: Get Ccache Stash | |
| steps: | |
| - name: System Cleanup | |
| working-directory: ${{ github.workspace }} | |
| run: | | |
| df -h | |
| sudo rm -rf /usr/share/dotnet /usr/local/lib/android /opt/ghc /opt/hostedtoolcache/CodeQL | |
| sudo docker image prune --all --force | |
| sudo docker builder prune -a | |
| df -h | |
| - name: Get Ccache Stash |
🤖 Prompt for AI Agents
In .github/workflows/linux-build.yml around lines 65 to 74, the step named "Node
Cleanup" performs system-wide cleanup including removing Docker images and
directories unrelated to Node. Rename the step to "System Cleanup" to accurately
reflect its broader cleanup scope.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
.github/workflows/linux-build.yml (7)
52-52: Use boolean forcancel-in-progress
The expression is quoted, so it evaluates to a string rather than a boolean. Remove the quotes to ensure proper concurrency cancellation:- cancel-in-progress: "${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}" + cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}
58-60: SimplifyUSE_CLANGdefault assignment
No interpolation is needed for a static default. Update to:env: CCACHE_DIR: ${{ github.workspace }}/ccache USE_CLANG: 'false'
66-67: Rename cleanup step for accuracy
The step named “Node Cleanup” performs system-wide cleanup. Consider renaming to “System Cleanup” to reflect its broader scope.
81-82: Prefer official cache action
Instead ofassignUser/stash/restore@v1, use the built-inactions/cacheaction for ccache. It’s maintained by GitHub and avoids a third-party dependency.
99-109: Clean up commented docs block
The entire documentation-install section is commented out. If docs are no longer built here, remove these lines to reduce clutter.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 108-108: trailing spaces
(trailing-spaces)
177-181: Mirror stash restore issues in save
The save step still usesassignUser/stash/saveand referencesinputs.use-clang. Apply the same fixes as the restore step: migrate toactions/cacheand correct key interpolation.🧰 Tools
🪛 actionlint (1.7.7)
180-180: property "use-clang" is not defined in object type {}
(expression)
83-85:⚠️ Potential issueFix stash key interpolation
inputs.use-clangis undefined, so the key will always resolve to the empty string. Reference the environment variable instead:- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} + key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}🧰 Tools
🪛 actionlint (1.7.7)
84-84: property "use-clang" is not defined in object type {}
(expression)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build.yml
84-84: property "use-clang" is not defined in object type {}
(expression)
180-180: property "use-clang" is not defined in object type {}
(expression)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build.yml
[error] 108-108: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC
🔇 Additional comments (11)
.github/workflows/linux-build.yml (11)
19-19: Confirm branch filter scope
The workflow now only triggers on pushes topresto-0.293-clp-connector. Please verify that this restriction aligns with project requirements and that you don’t need to include other long-lived branches.
57-57: Valid runner label
Switching toubuntu-22.04is correct and uses a standard GitHub-hosted label.
86-90: Ensure cache directory creation is correct
The step explicitly creates the cache directory before checkout, which is good. No issues spotted here.
91-94: Checkout path alignment
Cloning intoveloxand setting the working directory accordingly ensures later steps run in the right context. Looks good.
96-98: Core dependencies installation
Invokingscripts/setup-ubuntu.sh && install_apt_depsis clear and maintainable. No changes needed.
110-112: Clear CCache before build
Runningccache -szat this point is appropriate to avoid skew from previous runs.
114-118: Header compliance check
The license-header validation with patch instructions is robust. No further tweaks required.
135-154: Format enforcement
Themake format-fixstep followed by diff detection is solid. It provides clear instructions for failure.
156-165: Validate optional Clang integration
You conditionally switch to Clang-15 ifUSE_CLANGis true. Confirm thatclang-15andclang++-15packages are pre-installed on the Ubuntu 22.04 runner. Otherwise, the build will fail when enabled.
173-175: Report CCache stats after build
Showing statistics withccache -vshelps debug cache efficiency. Approved.
182-185: Test execution
Running CTest in parallel with--output-on-failureis standard. This meets project conventions.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
.github/workflows/linux-build.yml (5)
66-67: 🧹 Nitpick (assertive)Rename Node Cleanup step
The step removes system-wide artifacts, not just Node. Consider:- - name: Node Cleanup + - name: System Cleanup
213-214:⚠️ Potential issueFix incorrect ccache key interpolation (save)
Match the restore key by switching toenv.USE_CLANG:- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} + key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}🧰 Tools
🪛 actionlint (1.7.7)
213-213: property "use-clang" is not defined in object type {}
(expression)
84-85:⚠️ Potential issueFix incorrect ccache key interpolation (restore)
inputs.use-clangis undefined, so the key never switches for clang builds. Use theUSE_CLANGenv var:- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} + key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}🧰 Tools
🪛 actionlint (1.7.7)
84-84: property "use-clang" is not defined in object type {}
(expression)
52-52:⚠️ Potential issueFix boolean expression in
cancel-in-progress
The value is quoted, yielding a string instead of a boolean and preventing proper cancellation behaviour. Unquote the expression for a real boolean.
Apply this diff:- cancel-in-progress: "${{github.ref != 'refs/heads/presto-0.293-clp-connector'}}" + cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}
69-79: 🧹 Nitpick (assertive)Consolidate and dedupe cleanup commands
There’s a duplicate removal of/usr/share/dotnetand many individualrm -rfcalls. Consider bundling into a single script or loop for readability and speed:- sudo rm -rf /usr/local/lib/android - sudo rm -rf /usr/share/dotnet - sudo rm -rf /usr/share/dotnet - sudo rm -rf /opt/ghc - sudo rm -rf /opt/hostedtoolcache/CodeQL - sudo rm -rf "/usr/local/share/boost" - sudo rm -rf "$AGENT_TOOLSDIRECTORY" + for dir in /usr/local/lib/android /usr/share/dotnet /opt/ghc \ + /opt/hostedtoolcache/CodeQL "/usr/local/share/boost" \ + "$AGENT_TOOLSDIRECTORY"; do + sudo rm -rf "$dir" + done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build.yml
84-84: property "use-clang" is not defined in object type {}
(expression)
213-213: property "use-clang" is not defined in object type {}
(expression)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build.yml
[error] 108-108: trailing spaces
(trailing-spaces)
[warning] 215-215: comment not indented like content
(comments-indentation)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC
🔇 Additional comments (4)
.github/workflows/linux-build.yml (4)
19-19: Restrict workflow triggers to the connector branch
Limiting pushes topresto-0.293-clp-connectoraligns the Linux build with the intended branch and reduces unnecessary CI runs.
57-57: Use a valid runner label
Switching toubuntu-22.04aligns with GitHub-hosted runners. No further changes needed here.
58-60: Simplify static env assignment
TheUSE_CLANGdefault is a static'false'value, so you can remove the interpolation:- USE_CLANG: "${{ 'false' }}" + USE_CLANG: 'false'[ suggest_nitpick ]
61-64: Defaults for shell and working-directory look good
Usingbashand setting the working directory toveloxacross all steps is clear and consistent.
| - uses: assignUser/stash/save@v1 | ||
| with: | ||
| path: '${{ env.CCACHE_DIR }}' |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider official cache action for saving ccache
Similarly replace assignUser/stash/save@v1 with actions/cache to unify caching logic.
🤖 Prompt for AI Agents
In .github/workflows/linux-build.yml around lines 210 to 212, replace the usage
of the third-party action assignUser/stash/save@v1 for saving ccache with the
official actions/cache action. Update the step to use actions/cache with
appropriate keys and paths to unify caching logic and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
.github/workflows/linux-build.yml (6)
52-52: Remove quotes aroundcancel-in-progress
Quoting the expression forces it to a string instead of a boolean, breaking cancellation.- cancel-in-progress: "${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}" + cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}
66-66: 🧹 Nitpick (assertive)Rename cleanup step for clarity
This step removes system‐wide artefacts, not just Node; consider:- - name: Node Cleanup + - name: System Cleanup
210-214:⚠️ Potential issueFix undefined
inputs.use-clangon stash save key
Same issue as restore—reference the environment variable for key interpolation:- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} + key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}🧰 Tools
🪛 actionlint (1.7.7)
213-213: property "use-clang" is not defined in object type {}
(expression)
61-61: 🧹 Nitpick (assertive)Simplify default
USE_CLANGassignment
No interpolation needed for a static default—assign directly:- USE_CLANG: "${{ 'false' }}" + USE_CLANG: 'false'
81-85:⚠️ Potential issueFix undefined
inputs.use-clangin stash key
inputs.use-clangisn’t defined, so the key interpolation fails. Use the env var instead:- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} + key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}🧰 Tools
🪛 actionlint (1.7.7)
84-84: property "use-clang" is not defined in object type {}
(expression)
72-73: 🧹 Nitpick (assertive)Remove duplicated cleanup command
Therm -rf /usr/share/dotnetline appears twice—remove the extra to streamline.- sudo rm -rf /usr/share/dotnet
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build.yml
84-84: property "use-clang" is not defined in object type {}
(expression)
213-213: property "use-clang" is not defined in object type {}
(expression)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build.yml
[error] 108-108: trailing spaces
(trailing-spaces)
[warning] 215-215: comment not indented like content
(comments-indentation)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC
🔇 Additional comments (2)
.github/workflows/linux-build.yml (2)
19-19: Confirm branch trigger scope
You’ve restricted push events topresto-0.293-clp-connector, which means pushes to other branches (e.g.,main) won’t run this workflow. Please verify that this is intentional.
57-57: Valid standard runner label
Theubuntu-22.04label is correct for GitHub-hosted runners.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/docs.yml(2 hunks).github/workflows/linux-build-base.yml(2 hunks).github/workflows/linux-build.yml(2 hunks).github/workflows/scheduled.yml(3 hunks)Makefile(1 hunks)scripts/check.py(2 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)
🪛 Pylint (3.3.7)
scripts/check.py
[refactor] 192-192: Consider merging these comparisons with 'in' by using 'files in ('main', 'master', 'presto-0.293-clp-connector')'. Use a set instead if elements are hashable.
(R1714)
🪛 Ruff (0.11.9)
scripts/check.py
246-246: Local variable branch_parser is assigned to but never used
Remove assignment to unused variable branch_parser
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (20)
.github/workflows/linux-build.yml (2)
20-20: Branch trigger updated to new feature branch
The workflow now only runs on pushes topresto-0.293-clp-connector, which matches the PR intent.
52-52: Cancel-in-progress condition aligns with branch filter
Thecancel-in-progressboolean expression is correctly unquoted and targets the new branch.Makefile (4)
188-193: Consistent branch passed to format-fix target
Updatedformat-fixto invoke formatting againstpresto-0.293-clp-connector, as intended.
195-201: Consistent branch passed to format-check target
format-checknow correctly referencespresto-0.293-clp-connector.
203-208: Consistent branch passed to header-fix target
header-fixupdated to use the new branch argument accordingly.
210-215: Consistent branch passed to header-check target
header-checknow invokes the license header script withpresto-0.293-clp-connector..github/workflows/linux-build-base.yml (5)
27-28: Job renamed to reflect release intent
Theubuntu-releasejob and its descriptive name now clearly indicate a release build.
38-40: Checkout action targetsveloxsubdirectory
Usingpath: veloxaligns with thedefaults.working-directorysetting.
50-55: MAKEFLAGS updated for parallelism variables
TheMAKEFLAGSassignments forMAX_HIGH_MEM_JOBSandMAX_LINK_JOBSlook correct for propagating build settings.
56-60: Conditional Clang invocation
The branchableCC/CXXexports based onUSE_CLANGensure flexibility between GCC and Clang.
63-65: Test step uses dynamic parallelism
Thectestinvocation correctly leverages available CPUs and handles no-tests as errors..github/workflows/docs.yml (6)
15-15: Workflow renamed to Build Documentation
Updating the name reflects its sole responsibility of generating docs.
29-29: Permission scope reduced to read
Restricting tocontents: readfollows best practices for minimal permissions.
33-33: Concurrency cancel-in-progress expression updated
The workflow now correctly cancels in-progress runs for other branches.
37-39: Job metadata simplified
Thebuild_docsjob and its runner onubuntu-latestare appropriately renamed and scoped.
48-50: Use apt for installing system dependencies
Switching fromdnftoaptmatchesubuntu-latest.
56-56: Install Python dependencies via uv pip
Usinguv pip install -rconforms with the project's selected tooling..github/workflows/scheduled.yml (3)
34-34: Verify push branch filter.
Thepushevent is now restricted to thepresto-0.293-clp-connectorbranch. Confirm this aligns with your release workflows and that other branches (e.g.main) are covered elsewhere.
48-48: Adjust scheduled run timing.
The cron schedule has been updated to0 6 * * *(6 AM UTC). Ensure this timing is appropriate for your maintenance window and does not conflict with other scheduled workflows.
58-58: Review concurrency cancellation logic.
The expression${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}gatescancel-in-progress. Verify this condition correctly cancels only unwanted runs and doesn’t inadvertently cancel necessary workflows (e.g. PR builds).
|
|
||
| branch_parser = add_check_options(files, "main") | ||
| branch_parser = add_check_options(files, "master") | ||
| branch_parser = add_check_options(files, "presto-0.293-clp-connector") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused variable branch_parser
The assignment to branch_parser isn’t used later; drop the variable to avoid confusion.
🧰 Tools
🪛 Ruff (0.11.9)
246-246: Local variable branch_parser is assigned to but never used
Remove assignment to unused variable branch_parser
(F841)
🤖 Prompt for AI Agents
In scripts/check.py at line 246, the variable branch_parser is assigned but
never used. Remove the assignment and the variable name, calling
add_check_options(files, "presto-0.293-clp-connector") without assigning its
result to branch_parser to eliminate the unused variable.
| if files == "main" or files == "master" or files == "presto-0.293-clp-connector": | ||
| return util.run(f"git merge-base origin/{files} HEAD")[1] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify branch comparison with membership test
Consider replacing multiple == checks with if files in ("main", "master", "presto-0.293-clp-connector"): for readability.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 192-192: Consider merging these comparisons with 'in' by using 'files in ('main', 'master', 'presto-0.293-clp-connector')'. Use a set instead if elements are hashable.
(R1714)
🤖 Prompt for AI Agents
In scripts/check.py around lines 192 to 193, simplify the condition that checks
if the variable files matches one of several branch names by replacing multiple
equality checks with a single membership test using if files in ("main",
"master", "presto-0.293-clp-connector"). This improves readability and
conciseness without changing functionality.
|
|
||
| - name: Checkout | ||
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | ||
| - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Checkout pinned to specific commit
Pinning actions/checkout to a SHA guarantees workflow stability but consider switching to a semver tag for easier upgrades.
🤖 Prompt for AI Agents
In .github/workflows/docs.yml at line 42, the actions/checkout step is pinned to
a specific commit SHA, which ensures stability but makes upgrades harder. Change
the checkout action reference from the commit SHA to a semver tag like 'v4' or
'v4.1.1' to allow easier updates while maintaining reasonable stability.
| name: Linux Build using Clang | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prune redundant PR and push triggers.
The workflow name is “Linux Build using Clang” yet the linux-clang job is conditional on the schedule event, leaving pull_request and push triggers without any jobs. Remove these triggers for clarity or introduce jobs to handle PR and push events as needed.
🤖 Prompt for AI Agents
In .github/workflows/scheduled.yml around lines 15 to 16, the workflow defines
pull_request and push triggers but has no jobs running for these events, only
for the schedule event. To fix this, remove the pull_request and push triggers
from the workflow triggers section to avoid redundant triggers without jobs, or
alternatively add appropriate jobs that run on these events if needed.
|
|
||
| - name: Checkout | ||
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | ||
| - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 |
There was a problem hiding this comment.
| - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | |
| - name: Checkout | |
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 |
Just to revert unnecessary changes.
| - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | ||
| with: | ||
| fetch-depth: 0 | ||
| persist-credentials: true |
There was a problem hiding this comment.
| persist-credentials: true | |
| persist-credentials: true | |
Just to revert unnecessary changes.
| @@ -30,7 +30,6 @@ on: | |||
| - "scripts/setup-helper-functions.sh" | |||
| - ".github/workflows/linux-build.yml" | |||
| - ".github/workflows/linux-build-base.yml" | |||
There was a problem hiding this comment.
| - ".github/workflows/linux-build-base.yml" | |
| - ".github/workflows/linux-build-base.yml" | |
Just to revert unnecessary changes.
| - name: Get Ccache Stash | ||
| uses: assignUser/stash/restore@v1 | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| path: '${{ env.CCACHE_DIR }}' | ||
| key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} | ||
|
|
||
| path: velox |
There was a problem hiding this comment.
Previously, this step was after line 41, right? Does it need to be moved here?
| if [[ "${USE_CLANG}" = "true" ]]; then | ||
| export CC=/usr/bin/clang-15 | ||
| export CXX=/usr/bin/clang++-15 | ||
| fi |
There was a problem hiding this comment.
Sorry, since these steps are moved back into linux-build-base.yml, we can revert this change:
| if [[ "${USE_CLANG}" = "true" ]]; then | |
| export CC=/usr/bin/clang-15 | |
| export CXX=/usr/bin/clang++-15 | |
| fi | |
| if [[ "${USE_CLANG}" = "true" ]]; then export CC=/usr/bin/clang-15; export CXX=/usr/bin/clang++-15; fi |
| working-directory: velox/_build/release | ||
| run: | | ||
| cd _build/debug && ctest -j 8 --output-on-failure --no-tests=error | ||
| ctest -j $(getconf _NPROCESSORS_ONLN) --output-on-failure --no-tests=error |
There was a problem hiding this comment.
Likewise, since these steps are back in linux-build-base.yml, let's revert:
- name: Run Tests
run: |
cd _build/release && ctest -j $(getconf _NPROCESSORS_ONLN) --output-on-failure --no-tests=error| endif | ||
|
|
||
| header-check: #: Check for license header issues on the main branch | ||
| header-check: #: Check for license header issues on the presto-0.293-clp-connector branch |
There was a problem hiding this comment.
| header-check: #: Check for license header issues on the presto-0.293-clp-connector branch | |
| header-check: #: Check for license header issues |
|
|
||
| schedule: | ||
| - cron: '0 3 * * *' | ||
| - cron: '0 6 * * *' |
There was a problem hiding this comment.
We don't need to change this, right?
There was a problem hiding this comment.
yea, but I thought we might still work on 11pm in EST, so I just delay the run to 2pm EST. Shall I revert this?
There was a problem hiding this comment.
Gotcha. We don't need to revert then, but can you add a comment? Otherwise it's unclear why we picked 6am UTC.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/docs.yml(3 hunks).github/workflows/linux-build-base.yml(2 hunks).github/workflows/linux-build.yml(2 hunks)Makefile(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 (15)
Makefile (4)
188-193: Branch argument updated correctly for format-fix
Theformat-fixtarget now points topresto-0.293-clp-connectormatching the updated branch.
195-201: Branch argument updated correctly for format-check
Theformat-checktarget has been aligned with the new branch name, ensuring consistent formatting checks.
203-208: Branch argument updated correctly for header-fix
Theheader-fixtarget now referencespresto-0.293-clp-connector, keeping license header fixes in sync with the branch change.
210-215: Branch argument updated correctly for header-check
Theheader-checktarget is updated to use the new branch name, maintaining consistency for header validation..github/workflows/docs.yml (6)
15-15: Workflow name simplified
Renaming to “Build Documentation” accurately reflects the job’s sole responsibility.
29-29: Reduced permissions to least privilege
Switchingcontentstoreadis a good security hardening step.
33-33: Concurrency cancellation logic correct
The boolean expression now properly cancels in-progress runs for non-connector branches.
37-37: Job name updated succinctly
The step name “Build” aligns with the workflow’s streamlined purpose.
38-38: Runner standardized to Ubuntu
Switching toubuntu-latestsimplifies environment management without a container.
58-58: Python dependency installation is correct
Usinguv pipvia pipx after venv activation installs requirements as expected..github/workflows/linux-build.yml (2)
19-21: Trigger restricted to the connector branch
Limiting push triggers topresto-0.293-clp-connectormatches the upstream changes and avoids CI noise onmain.
32-34: Concurrency cancellation logic correct
The unquoted boolean expression will correctly cancel in-progress runs when not on the connector branch..github/workflows/linux-build-base.yml (3)
27-28: Job renamed for release context
Renaming toubuntu-releaseand clarifying its resolve_dependency role is consistent with your simplified pipeline.
57-58: Clang toolchain selection is valid
The conditional export ofCC/CXXbased onUSE_CLANGaligns with the job input.
62-62: Test invocation uses dynamic parallelism
Runningctestwithgetconf _NPROCESSORS_ONLNis a clean way to maximize resource usage.
| run: | | ||
| dnf install -y --setopt=install_weak_deps=False pandoc | ||
| pip3 install pipx | ||
| sudo apt install -y pipx |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider updating apt cache before install
It’s safer to run sudo apt-get update && sudo apt-get install -y pipx to avoid stale indexes.
🤖 Prompt for AI Agents
In .github/workflows/docs.yml at line 50, the command installs pipx without
updating the apt cache first, which can lead to stale package indexes. Modify
the command to run `sudo apt-get update` before `sudo apt-get install -y pipx`
by combining them with `&&` to ensure the package list is up to date before
installation.
| - name: Build Artifact | ||
| env: | ||
| VELOX_DEPENDENCY_SOURCE: BUNDLED | ||
| ICU_SOURCE: SYSTEM | ||
| MAKEFLAGS: "NUM_THREADS=8 MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3" | ||
| EXTRA_CMAKE_FLAGS: "-DVELOX_ENABLE_ARROW=ON -DVELOX_ENABLE_PARQUET=ON -DVELOX_ENABLE_EXAMPLES=ON" | ||
| MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3" | ||
| run: | |
There was a problem hiding this comment.
Invalid MAKEFLAGS assignment
Setting MAKEFLAGS to "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3" won’t pass proper flags to make. Please revisit the intended concurrency flags—e.g.:
- MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3"
+ MAKEFLAGS: "-j$(getconf _NPROCESSORS_ONLN)"or inject variables directly into the make invocation.
📝 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: Build Artifact | |
| env: | |
| VELOX_DEPENDENCY_SOURCE: BUNDLED | |
| ICU_SOURCE: SYSTEM | |
| MAKEFLAGS: "NUM_THREADS=8 MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3" | |
| EXTRA_CMAKE_FLAGS: "-DVELOX_ENABLE_ARROW=ON -DVELOX_ENABLE_PARQUET=ON -DVELOX_ENABLE_EXAMPLES=ON" | |
| MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3" | |
| run: | | |
| - name: Build Artifact | |
| env: | |
| VELOX_DEPENDENCY_SOURCE: BUNDLED | |
| ICU_SOURCE: SYSTEM | |
| - MAKEFLAGS: "MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3" | |
| + MAKEFLAGS: "-j$(getconf _NPROCESSORS_ONLN)" | |
| run: | |
🤖 Prompt for AI Agents
In .github/workflows/linux-build-base.yml around lines 51 to 56, the MAKEFLAGS
environment variable is incorrectly assigned a string "MAX_HIGH_MEM_JOBS=4
MAX_LINK_JOBS=3" which is not a valid format for make flags. To fix this, remove
the MAKEFLAGS assignment or set it to proper make flags like "-j4" for
concurrency, or alternatively, pass MAX_HIGH_MEM_JOBS and MAX_LINK_JOBS as
separate environment variables or directly in the make command invocation to
control parallel jobs correctly.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
ci: Remove unnecessary workflows and simplify others for efficiency.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/scheduled.yml (1)
18-26: Remove dead triggers or enable jobs for PR/push events
This workflow still declarespull_requestandpushtriggers, but the only job (linux-clang) is gated toif: github.event_name == 'schedule', so no CI runs occur on PR or push. Either remove these triggers to avoid confusion or adjust the job filter to run on the intended events.Also applies to: 32-45
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scheduled.yml(3 hunks)
⏰ 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/scheduled.yml (3)
15-15: Workflow name updated to reflect Clang build
The new name "Linux Build using Clang" accurately describes this workflow’s purpose and aligns with the job’suse-clang: trueinput.
34-34: Verify branch pattern for push trigger
The push trigger is scoped to"presto-0.293-clp-connector". Confirm that this branch name matches the primary development branch across all workflows and docs to prevent CI misfires.
48-49: Clarify scheduled run timing
The inline comment clearly states why the workflow runs at 6 AM UTC (“to avoid interrupting late night work in Toronto”), satisfying past feedback.
| concurrency: | ||
| # This will not cancel fuzzer runs on main (regardless of which trigger) | ||
| # by making the commit sha part of the group but will use the branch | ||
| # name in PRs to cancel on going runs on a new commit. | ||
| group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }} | ||
| cancel-in-progress: true | ||
|
|
||
| env: | ||
| # Run for 15 minute on PRs | ||
| DURATION: "${{ inputs.duration || ( github.event_name != 'schedule' && 900 || 1800 )}}" | ||
| # minimize artifact duration for PRs, keep them a bit longer for nightly runs | ||
| RETENTION: "${{ github.event_name == 'pull_request' && 1 || 3 }}" | ||
| cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify or document concurrency cancellation condition
The cancel-in-progress expression omits cancellation for runs on presto-0.293-clp-connector, but since only scheduled runs currently execute, consider simplifying this logic or adding a comment to explain its future purpose.
🤖 Prompt for AI Agents
In .github/workflows/scheduled.yml around lines 57 to 60, the cancel-in-progress
condition is complex and currently only affects scheduled runs. Simplify the
expression by removing the condition if it is unnecessary now, or add a clear
comment above it explaining that cancellation is disabled for the
'presto-0.293-clp-connector' branch to support future workflow changes.
…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
…or. (y-scope#8) CLP's filter module requires xxHash 0.8.3 via find_package(xxHash CONFIG REQUIRED), but no system or bundled package provided it. Add an xxhash.cmake resolve module that fetches xxHash 0.8.3 from GitHub using OVERRIDE_FIND_PACKAGE to intercept the find_package call, and wire it into CMakeLists.txt before CLP is resolved.
2 participants
Description
Simiar to this one: y-scope/presto#7
Removed some unnecessary workflows for now.
For header and format check, we will apply this commit, which also uses a new way to implement ccache, and we will try that in the next PR.
For clang-15 build, there is an error cause the CI failure. By
gcc --help=warnings | grep conversionthere is no-Wimplicit-const-int-float-conversion, but in clang, there is such a flag (added in commit). So the root cause is the way that velox configures the flags is first using-Wallthen manually disabling some specific flags, so this-Wimplicit-const-int-float-conversionis missed forclangdue to the difference of sets of warning flags. This is why it can be built with gcc but not with clang. Since we don't modify the code, this issue should also exist in main repo's 0.293, will try to submit a fix PR to the OSS as well.A few more things about the error mentioned above. After applying the fix similar to this commit, there is another place triggers the error (in this CI failure).
Checklist
breaking change.
Validation performed