feat(deps): Validate dependency pins against the pinned Presto commit. - #29
Merged
Merged
Conversation
📝 WalkthroughWalkthroughChangesThe change centralizes the pinned Presto repository and commit in Dependency validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BuildOrCI
participant Validator as validate-presto-dep-sync.py
participant Presto as Pinned Presto checkout
participant Velox as Pinned Velox tree
BuildOrCI->>Validator: Run dependency validation
Validator->>Presto: Read pinned commit and Maven versions
Validator->>Velox: Read submodule SHA and Velox versions
Validator-->>BuildOrCI: Return validation status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
jackluo923
force-pushed
the
feat/validate-dep-sync
branch
2 times, most recently
from
July 22, 2026 22:45
bb68d9a to
0853437
Compare
Member
Author
|
@coderabbitai let's run the code review |
|
✅ Action performedReview finished.
|
jackluo923
force-pushed
the
feat/validate-dep-sync
branch
3 times, most recently
from
July 23, 2026 06:12
cc0b0dd to
fdfc2ee
Compare
Adds tools/presto-deps/validate-presto-dep-sync.py, which checks the pom.xml Presto-synced version pins against the Presto root pom at G_PRESTO_GIT_TAG, and the deps.yaml G_*_VERSION header pins against that commit's presto-native-execution/velox submodule tree. It prints OK/FAIL per pin with a suggested value and never edits anything; sources come from a local checkout containing the pinned commit when available, otherwise from blobless shallow clones under the build directory. Runs before the presto-connector build/test tasks, the velox-connector build, and the packaging build, plus a standalone validate-deps workflow on pull requests and pushes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jackluo923
force-pushed
the
feat/validate-dep-sync
branch
from
July 23, 2026 16:35
fdfc2ee to
36d878e
Compare
…he pom needs the edit.
…ed in the tools README.
…ks, shared git helper.
20001020ycx
reviewed
Jul 24, 2026
20001020ycx
left a comment
20001020ycx
left a comment
Collaborator
There was a problem hiding this comment.
Did a pass for everything except validate-presto-dep-sync.py.
20001020ycx
reviewed
Jul 24, 2026
20001020ycx
left a comment
20001020ycx
left a comment
Collaborator
There was a problem hiding this comment.
Velox side looks pretty aligned with what I have in mind, but I do have a few question regarding the Presto side, might have been me missing some context.
…alidator into Presto and Velox classes. The pin (G_PRESTO_GIT_URL/G_PRESTO_GIT_TAG) is consumed by both connectors and the tools/presto-deps/ scripts, so it belongs in taskfile.yaml rather than the velox-connector deps taskfile; Task's global vars propagate to all includes. Per review, the validator now encapsulates each side's constants, sources, and checks: Presto owns the pin (taskfile.yaml), the connector pom pins, and the pinned-commit checkout; Velox owns deps.yaml's G_*_VERSION pins and the submodule resolution. Also documents why the Presto root pom (not presto-spi) is the version source of truth and what each checkout candidate is, and rewords the validate-deps workflow header to describe the report output.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@taskfiles/velox-connector/main.yaml`:
- Around line 17-20: Update the build task’s deps configuration to run
validate-dep-sync before deps:install-all, using go-task’s sequential dependency
mechanism rather than parallel deps entries. Preserve the existing task names
and ensure dependency installation starts only after validation succeeds.
In `@tools/presto-deps/validate-presto-dep-sync.py`:
- Around line 3-6: Add S603 to the file-level Ruff noqa suppression list in
validate-presto-dep-sync.py, preserving the existing rationale for trusted
PATH-resolved git subprocess usage and leaving the subprocess.run call
unchanged.
- Around line 99-109: Protect the shared checkout operations in shallow_repo
with a file lock located under BUILD_DIR, acquiring it before directory creation
and releasing it after the repository validation or fetch completes. Ensure
concurrent validate-dep-sync invocations serialize the entire shallow_repo
critical section, including git init, remote updates, and fetches, while
preserving the existing return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7ef77aaa-b2e1-4092-89d0-34f96a0d5469
📒 Files selected for processing (9)
.github/workflows/validate-deps.yamlpresto-connector/pom.xmltaskfile.yamltaskfiles/presto-connector/main.yamltaskfiles/velox-connector/deps.yamltaskfiles/velox-connector/main.yamltools/build-packages/internal/container/build-artifacts.shtools/presto-deps/install-presto-artifacts.shtools/presto-deps/validate-presto-dep-sync.py
💤 Files with no reviewable changes (1)
- taskfiles/velox-connector/deps.yaml
…cross-cache scavenging. The Presto-side tools (validator + installer) share build-dir presto-src, seeded by whichever runs first; the velox-connector's FetchContent tree stays its own. The FetchContent-cache candidate scanning was nearly dead code: the validator runs before the builds as a task dependency, so presto-src is already populated when the installer looks.
The check is repo-scoped (like the pin it guards), so both connectors now depend on the root task via ":validate-dep-sync" instead of defining identical copies.
Velox no longer takes the Presto instance: both classes independently derive their inputs from the root taskfile pin and the shared presto-src clone via module helpers, so either check can run without the other.
…banners. Per review (too many variables to keep track of): classes keep only the attributes their checks read (Presto: pin/connector_pom/root_pom; Velox: presto_pin/sha/repo), every comparison uses one ours/theirs vocabulary, the unv_* locals fold into strip_v(), and the color constants move inside Reporter. Section banners group the file's helpers, and the ElementTree namespace and blobless-fetch idioms are now explained where used.
Parallel deps let the (long) dependency install start even when validation was about to fail; sequential cmds make the validator a true gate.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/presto-deps/install-presto-artifacts.sh (1)
75-79: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject ambiguous pin definitions.
The
sedpatterns match the key anywhere in a line and only check for a non-empty result. A commented-out key or multiple matching entries can therefore be accepted even when the Taskfile/validator resolves a different value. Anchor the match to an active mapping entry and fail unless exactly one value is found.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/presto-deps/install-presto-artifacts.sh` around lines 75 - 79, Update the `presto_git_url` and `presto_git_tag` extraction in the install script to match only active Taskfile mapping entries, not commented or unrelated text. Require exactly one matching value for each key and call `die` when zero or multiple entries are found, keeping the resolved value consistent with the Taskfile/validator.taskfiles/velox-connector/main.yaml (1)
16-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
:validate-dep-syncstill races withdeps:install-allinstead of gating it.Both entries remain under
deps:, which go-task runs in parallel by default, so validation isn't guaranteed to complete beforedeps:install-allstarts — contradicting this PR's stated goal that "validation runs before connector builds." This is the same gap raised on a prior revision (then against the un-prefixedvalidate-dep-syncname); the suggested reordering was never applied.🔧 Suggested fix: sequence validation first
build: deps: - - "deps:install-all" - - ":validate-dep-sync" + - ":validate-dep-sync" cmds: + - task: "deps:install-all" - task: "build-with-installed-deps"[reliability_and_availability]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@taskfiles/velox-connector/main.yaml` around lines 16 - 22, Update the build task’s dependency orchestration so :validate-dep-sync completes before deps:install-all starts, rather than listing both under parallel deps. Preserve the existing build-with-installed-deps command and ensure the validation-first ordering is enforced by the task configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@taskfiles/velox-connector/main.yaml`:
- Around line 16-22: Update the build task’s dependency orchestration so
:validate-dep-sync completes before deps:install-all starts, rather than listing
both under parallel deps. Preserve the existing build-with-installed-deps
command and ensure the validation-first ordering is enforced by the task
configuration.
In `@tools/presto-deps/install-presto-artifacts.sh`:
- Around line 75-79: Update the `presto_git_url` and `presto_git_tag` extraction
in the install script to match only active Taskfile mapping entries, not
commented or unrelated text. Require exactly one matching value for each key and
call `die` when zero or multiple entries are found, keeping the resolved value
consistent with the Taskfile/validator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c7a5789a-6200-4dc4-a0ba-b03f04c4e325
📒 Files selected for processing (6)
.github/workflows/validate-deps.yamltaskfile.yamltaskfiles/presto-connector/main.yamltaskfiles/velox-connector/main.yamltools/presto-deps/install-presto-artifacts.shtools/presto-deps/validate-presto-dep-sync.py
20001020ycx
reviewed
Jul 27, 2026
20001020ycx
left a comment
20001020ycx
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the refactor, I think the code structure is clean now, I dont have too much comment on the code itself, but I do have a few questions regarding the design.
validate-presto-dep-sync.py checks the same invariant against the pinned commit, and every path that runs the installer (task deps, the packaging container, CI) runs the validator first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
20001020ycx
approved these changes
Jul 28, 2026
jackluo923
added a commit
that referenced
this pull request
Jul 28, 2026
The Presto pin moved there in #29. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jackluo923
added a commit
that referenced
this pull request
Jul 31, 2026
The Presto pin moved there in #29. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Several dependency versions in this repository must stay synchronized with the pinned Presto commit, and until now that synchronization was maintained by hand and comment: the pom's Presto-synced version pins (Presto's plugin classloader supplies some packages from the Presto runtime rather than from the plugin, so drift crashes the coordinator when it loads the plugin), and the Velox header pins that must match the Velox tree Presto builds with.
This PR makes all of that automatically checked, and (per review) consolidates where the pin itself lives:
G_PRESTO_GIT_URL/G_PRESTO_GIT_TAGmove fromtaskfiles/velox-connector/deps.yamlto the roottaskfile.yaml: both connectors and thetools/presto-deps/scripts consume them, so the velox-scoped location was misleading.deps.yamlkeeps only the Velox-onlyG_*_VERSIONheader pins.tools/presto-deps/validate-presto-dep-sync.pyvalidates 12 pins — the 4 pom pins against the Presto root pom atG_PRESTO_GIT_TAG(the root pom'sdep.*properties are Presto's source of truth; modules such as presto-spi inherit them), and the 8 Velox header pins against the pinned commit'spresto-native-execution/veloxsubmodule tree. On drift it fails, naming the pin, both versions, and the exact fix; it never edits anything.Prestoclass and aVeloxclass, each deriving its inputs from the pin and its own files, runnable without the other.build/presto-src, seeded with a blobless fetch by whichever runs first; the Velox check clonesbuild/velox-srcfrom the submodule URL in Presto's own.gitmodules. The velox-connector's CMake FetchContent tree stays its own — no cross-cache scavenging.validate-dep-syncis defined once in the root taskfile, next to the pin it guards. Thepresto-connectorbuild/test tasks and thevelox-connectorbuild depend on it via:validate-dep-sync; the packaging build and the standalonevalidate-depsworkflow (pull requests, pushes to main, manual dispatch) invoke the script directly — an out-of-sync pin cannot survive unnoticed.presto.versionre-check was dropped — every path that runs it runs the validator first.Validation performed
All 12 pins pass warm (existing clones, zero network) and cold (empty build directory; both blobless fetches complete in ~7 s).
Each class verified in isolation:
Presto()alone (4 checks) andVelox()alone (8 checks), confirming the paths are independent.Negative tests: perturbed pins (pom and deps.yaml) fail with the expected suggested value and a non-zero exit:
Output on failure (jackson.version perturbed to 2.15.4)
Verified on Python 3.6.8 inside the packaging build-env container.
task presto-connector:buildandtask presto-connector:test(32/32 tests) pass through the root-task wiring, exercising the validator and the installer's relocated pin parsing.task packagepasses end to end in a cold build-env container: in-container validation (fresh clones), Presto artifact installation from the relocated pin, and all three package formats produced.The
validate-depsCI workflow passes on this PR:Output on success (CI run)
Summary by CodeRabbit