feat(packaging): Add local packaging workflow with caching. - #25
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a local package-build entry point, shared host/container build caches, configurable CMake and Maven cache paths, cache-enabled dependency images, and documentation for local packaging workflows. ChangesPackaging and build-cache flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant BuildScript as build-packages.sh
participant Docker
participant Builder as build-artifacts.sh
participant Output as packages directory
Developer->>BuildScript: Run package command with options
BuildScript->>Docker: Mount repository, cache, and staging paths
Docker->>Builder: Run container-side packaging
Builder-->>Docker: Produce staged artifacts
Docker-->>BuildScript: Return build result
BuildScript->>Output: Copy generated packages
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tools/build-packages/build-packages.sh`:
- Line 123: Update the artifact copy command to use the POSIX-compatible -f
option instead of GNU-specific --remove-destination, preserving the existing
source and destination behavior in the build script.
🪄 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
Run ID: 9e855e29-f090-4e47-839c-bf6294242c7e
📒 Files selected for processing (11)
.dockerignore.gitignoreREADME.mdtaskfiles/velox-connector/main.yamltools/build-packages/README.mdtools/build-packages/build-packages.shtools/build-packages/dependency-image/Dockerfiletools/build-packages/internal/build-cache/README.mdtools/build-packages/internal/build-cache/container.shtools/build-packages/internal/build-cache/host.shtools/build-packages/internal/container/build-artifacts.sh
--remove-destination is a GNU cp extension absent on BSD/macOS cp; -f is POSIX-specified and behaves equivalently for copying staged artifacts into the user-owned output directory.
The top-level README replaced the CI-build reference with a local-build one; both paths exist, so restore the CI mention and add the local one.
Tighten prose (intro, Container API, Lifecycle) while keeping every contract and integration note. Use one prose paragraph per line instead of manual hard wraps.
…d build tree
Local builds previously ran the container build as root and wrote the project
build tree to the disposable container layer, so every invocation recompiled
the C++ connector from scratch and left root-owned files in the repo.
Changes:
* build-packages.sh: run `docker run` as the host uid/gid and set
CLP_PLUGIN_BUILD_DIR to a path under the persistent .cache/build/<hash>
cache, so the CMake build tree survives across runs (incremental rebuilds).
* dependency-image/Dockerfile: pin CLP_PLUGIN_DEPS_CPP_DIR to the image's
installed deps so the deps dir is not derived from the overridden build dir.
* taskfiles/velox-connector/main.yaml: make G_DEPS_CPP_DIR env-overridable
via CLP_PLUGIN_DEPS_CPP_DIR (mirrors the existing FETCHCONTENT_BASE_DIR
pattern).
* internal/build-cache/{host,container}.sh: create the build/<key> cache
subdir; document it in internal/build-cache/README.md.
* internal/container/build-artifacts.sh: place packaging staging under
project_build_dir so it lives in the cache for local builds (CI still
resolves to the image's /opt).
Result: a no-op rebuild completes in under 30 seconds (C++ compile is a
cache hit) and produces host-owned .deb/.rpm/.tar.gz with no root-owned
files in the repo.
Reverts the behavior-neutral formatting changes that came over with the
host-user/cached-build port so the diff is purely functional:
* .github/workflows/build-packages.yaml: restore the two-line ::error:: echo.
* taskfiles/velox-connector/main.yaml: restore the quoted `desc:`.
* tools/build-packages/internal/container/build-artifacts.sh: restore the
MAVEN_OPTS/mvnw line wrapping and the submodule-init comment wording.
* tools/build-packages/build-packages.sh: restore the defensive
${build_args[@]+"${build_args[@]}"} quoting.
Functional changes (host-user run, CLP_PLUGIN_BUILD_DIR env, CLP_PLUGIN_DEPS_CPP_DIR
pin, cache build/<key> helpers + README, build_root relocation) are unchanged.
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 (1)
tools/build-packages/build-packages.sh (1)
78-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate that the extracted image hash is non-empty.
This check only verifies that
:env-exists. A malformed value such asrepo:env-passes with an emptyimage_hash, causing the cache to be prepared without a real image key and potentially sharing unscoped build/FetchContent state. Match and extract a non-emptyenv-<hash>tag instead.Proposed fix
-image_hash="${image##*:env-}" -if [[ "${image_hash}" == "${image}" ]]; then +if [[ "${image}" =~ :env-([[:alnum:]][[:alnum:]._-]*)$ ]]; then + image_hash="${BASH_REMATCH[1]}" +else echo >&2 "ERROR: build-env image lacks an env-<hash> tag: ${image}" exit 1 fi🤖 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/build-packages/build-packages.sh` around lines 78 - 82, Update the image tag validation around image_hash to require a non-empty hash after the env- prefix, rejecting malformed values such as repo:env-. Match and extract only a valid env-<hash> tag before proceeding with cache preparation, while preserving the existing error-and-exit behavior for invalid image values.
🤖 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 `@tools/build-packages/build-packages.sh`:
- Around line 78-82: Update the image tag validation around image_hash to
require a non-empty hash after the env- prefix, rejecting malformed values such
as repo:env-. Match and extract only a valid env-<hash> tag before proceeding
with cache preparation, while preserving the existing error-and-exit behavior
for invalid image values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b5da872-6533-4ff2-9e6e-46b59333854f
📒 Files selected for processing (3)
taskfiles/velox-connector/main.yamltools/build-packages/build-packages.shtools/build-packages/internal/container/build-artifacts.sh
added 3 commits
July 16, 2026 23:39
Fold the inline bash setup (cache wiring, HOME/TASK_TEMP_DIR scratch dirs, umask) that build-packages.sh ran before build-artifacts.sh into a BUILD_CACHE_DIR-guarded block at the top of build-artifacts.sh itself. Local builds set BUILD_CACHE_DIR (via build-packages.sh's --env flags), so they run that setup. CI invokes build-artifacts.sh directly without it, so the block is skipped and CI inherits the image's /opt layout unchanged. Removes the inline 'bash -c' block from build-packages.sh in favor of a direct 'bash /repo/.../build-artifacts.sh' invocation. No new file needed.
…uild-artifacts.sh Comment-only: name the BUILD_CACHE_DIR/HOME/TASK_TEMP_DIR env contract in build-artifacts.sh and reword the docker-run comment in build-packages.sh so the --env flags aren't orphaned from the setup they drive.
derive_build_env_hash called sha256sum directly, which stock macOS doesn't ship (it provides shasum -a 256). Resolve a sha256_cmd that falls back to shasum -a 256 and use it in both stages of the hash pipeline. Both tools emit the same <hash> <file> format, so the resulting tag is identical on either host. Also update the Requires: header in build-packages.sh and build-dependency-image.sh and the README prerequisites to list both.
20001020ycx
reviewed
Jul 17, 2026
20001020ycx
left a comment
20001020ycx
left a comment
Collaborator
There was a problem hiding this comment.
As discussed offline, the high level goal is clear. This PR will promote local package build with ccache. However, for the sake of UX, let's add a task as the interface exposed rather a plain bash script
20001020ycx
reviewed
Jul 17, 2026
20001020ycx
reviewed
Jul 17, 2026
added 2 commits
July 17, 2026 02:04
Add a 'package' task to the root taskfile as a thin wrapper over tools/build-packages/build-packages.sh, so local package builds use the same 'task ...' ergonomics as 'task clean' / 'task velox-connector:build'. Args forward via CLI_ARGS (use '--' to pass flags). Document it as the recommended local entry point in tools/build-packages/README.md.
20001020ycx
previously approved these changes
Jul 17, 2026
20001020ycx
left a comment
20001020ycx
left a comment
Collaborator
There was a problem hiding this comment.
Once the changes discussed offline are addressed, e.g. README and taskfile, I am okay with the merge
Per reviewer request. Drop the internal build-cache README and the link to it from tools/build-packages/README.md.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@taskfile.yaml`:
- Around line 31-32: Update the command under cmds to quote the rendered
build-packages.sh path, while keeping {{.CLI_ARGS}} outside the quotes so
arguments continue to split normally.
- Around line 28-30: Update the precondition command to validate Buildx
specifically by invoking docker buildx version. In the package-build task, quote
the ROOT_DIR-based build-packages.sh path while preserving CLI argument
forwarding so workspace paths containing spaces work correctly.
In `@tools/build-packages/README.md`:
- Around line 36-37: Update the cache layout documentation near the build-state
description to include .cache/build/<hash>/ for persisted CMake/build state, and
explicitly distinguish it from the repository’s separate build/ directory.
- Around line 38-39: Update the ownership guidance near the container execution
description to state that the local wrapper runs with the invoking host UID/GID,
so it does not create root-owned files; explain that root-owned files in
.cache/, build/, or presto-connector/target/ may instead remain from earlier
privileged or CI builds, while packages/ remains owned by the invoking user.
🪄 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
Run ID: 3cbb292c-d2ac-4954-966e-4cb0efa26005
📒 Files selected for processing (2)
taskfile.yamltools/build-packages/README.md
…age' Precondition now runs 'docker buildx version' instead of 'command -v docker' so it actually verifies buildx, not just docker. Quote the ROOT_DIR-based build-packages.sh path so workspace paths containing spaces invoke the script correctly; CLI_ARGS stays unquoted to preserve argument forwarding.
…epo build/ Add build/<hash>/ to the cache layout list in the packaging README and clarify that it is the container's build output dir, distinct from the repo's top-level build/ used by non-container local dev builds.
The local wrapper runs the build container with the invoking host UID/GID (--user flag) and refuses sudo, so it does not create root-owned files. Reword the ownership note to reflect that: any root-owned files in .cache/, build/, or presto-connector/target/ are leftovers from earlier privileged or CI builds, while packages/ stays owned by the invoking user.
…d dirs
Switch G_BUILD_DIR, G_DEPS_CPP_DIR, and G_FETCHCONTENT_BASE_DIR from
{{env "VAR"}} to {{.VAR | default ...}}. In Task v3 both forms read the
shell environment ({{.VAR}} falls back to env as its last lookup step), and
since no Taskfile scope defines these names they behave identically.
Adopted for consistency with the Taskfile docs idiom.
Validated via task --dry: G_BUILD_DIR and G_DEPS_CPP_DIR fallback and
env-override both resolve correctly in the real Taskfiles, and the
G_FETCHCONTENT_BASE_DIR override resolves in an isolated probe using the
same pattern.
20001020ycx
approved these changes
Jul 20, 2026
jackluo923
changed the title
jackluo923
changed the title
jackluo923
changed the title
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.
Summary
Adds a user-facing local packaging entry point (
tools/build-packages/build-packages.sh) that resolves the hash-tagged build-env image and runs the same container-side artifact build (internal/container/build-artifacts.sh) used by CI. Brings persistent, shared build caching to local builds, cutting incremental local builds from ~15 minutes down to 15–30 seconds.What's included
build-packages.sh— host-side entry point: resolves output dir, initializes submodules, resolves the build-env image (build-dependency-image.sh), prepares the build cache, and runs the artifact build inside the image.internal/build-cache/— host- and container-side cache configuration (maven/,ccache/,fetchcontent/<hash>/) keyed by the build-env image hash, so cached CMake state stays compatible with the image inputs.Known follow-up (not in this PR)
docker runonly forwardsBUILD_CACHE_KEY, so builds behind a corporate proxy (or other environments needing host network/proxy config inherited by the container) are not yet supported. This will be addressed in a separate follow-up PR.Summary by CodeRabbit
New Features
.deb,.rpm, and.tar.gzpackages.Documentation
Improvements