Bundle H3 source in release tarballs#209
Conversation
📝 WalkthroughWalkthroughAdds a source bundle target and helper script that create a release tarball, stage bundled H3 sources when available, and update the bundle script, release text, and ignore rules. ChangesSource bundle workflow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 `@CMakeLists.txt`:
- Around line 84-92: The source_bundle target is referencing h3_SOURCE_DIR from
the top-level CMakeLists.txt, but that variable is only set inside
cmake/h3/CMakeLists.txt and does not propagate out of the subdirectory scope.
Promote h3_SOURCE_DIR to a CACHE PATH variable in cmake/h3/CMakeLists.txt after
FetchContent_MakeAvailable(h3) so it is visible to the parent scope, and then
keep the source_bundle command using ${h3_SOURCE_DIR} as the source path for
BundleSource.cmake.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 968abe1e-0937-4557-ba14-6b9fc4e192f1
📒 Files selected for processing (5)
CMakeLists.txtcmake/BundleSource.cmakecmake/h3/CMakeLists.txtscripts/bundlescripts/release
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb145e58ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| COMMAND ${CMAKE_COMMAND} | ||
| -DARCHIVE_OUTPUT=${CMAKE_CURRENT_SOURCE_DIR}/h3-${INSTALL_VERSION}.tar.gz | ||
| -DARCHIVE_PREFIX=h3-${INSTALL_VERSION} | ||
| -DH3_SOURCE_DIR=${h3_SOURCE_DIR} |
There was a problem hiding this comment.
When running scripts/bundle, this expands to -DH3_SOURCE_DIR= because h3_SOURCE_DIR is populated by FetchContent_MakeAvailable(h3) inside cmake/h3/CMakeLists.txt; CMake variables set in a subdirectory do not propagate back to the parent. BundleSource.cmake treats an empty H3_SOURCE_DIR as fatal, so the new source_bundle target fails before producing the release tarball. Export the path from the subdirectory scope before passing it here.
Useful? React with 👍 / 👎.
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 `@scripts/bundle`:
- Around line 10-15: The cleanup gate in require_clean_tracked_tree only checks
tracked and staged changes, so it can still proceed when untracked files exist
and are about to be removed by the later git clean -xdf step. Update
require_clean_tracked_tree in scripts/bundle to also detect untracked content
(or otherwise require explicit opt-in before destructive cleanup) so the script
fails before deleting local inputs or artifacts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0373d9d-8557-42dd-ac0d-7378b9e46f71
📒 Files selected for processing (2)
cmake/h3/CMakeLists.txtscripts/bundle
🚧 Files skipped from review as they are similar to previous changes (1)
- cmake/h3/CMakeLists.txt
| require_clean_tracked_tree() { | ||
| git diff --quiet || die "tracked worktree changes are present" | ||
| git diff --cached --quiet || die "staged changes are present" | ||
| } | ||
|
|
||
| require_clean_tracked_tree |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Also guard against untracked files before cleaning.
This only checks tracked/staged edits, but the next git clean -xdf will still delete untracked/ignored files after the script reports the tree as acceptable. That is an easy way to lose local packaging inputs or release artifacts. Either fail on untracked content here as well, or make the destructive clean step explicitly opt-in.
Suggested change
require_clean_tracked_tree() {
git diff --quiet || die "tracked worktree changes are present"
git diff --cached --quiet || die "staged changes are present"
+ git status --porcelain --untracked-files=all | grep -q '^\?\?' &&
+ die "untracked files are present"
}📝 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.
| require_clean_tracked_tree() { | |
| git diff --quiet || die "tracked worktree changes are present" | |
| git diff --cached --quiet || die "staged changes are present" | |
| } | |
| require_clean_tracked_tree | |
| require_clean_tracked_tree() { | |
| git diff --quiet || die "tracked worktree changes are present" | |
| git diff --cached --quiet || die "staged changes are present" | |
| git status --porcelain --untracked-files=all | grep -q '^\?\?' && | |
| die "untracked files are present" | |
| } | |
| require_clean_tracked_tree |
🤖 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 `@scripts/bundle` around lines 10 - 15, The cleanup gate in
require_clean_tracked_tree only checks tracked and staged changes, so it can
still proceed when untracked files exist and are about to be removed by the
later git clean -xdf step. Update require_clean_tracked_tree in scripts/bundle
to also detect untracked content (or otherwise require explicit opt-in before
destructive cleanup) so the script fails before deleting local inputs or
artifacts.
1 participant
Summary
h3-<version>.tar.gzalongside the PGXN zipcmake/h3/upstream/in that tarballFixes #206.
Test
scripts/bundletar -tzf h3-unreleased.tar.gz | rg '^(h3-unreleased/(cmake/h3/upstream/CMakeLists.txt|cmake/h3/upstream/src/h3lib/include/linkedGeo.h|cmake/BundleSource.cmake|META.json))$'cmake -S /tmp/h3pg-offline-tarball/h3-unreleased -B /tmp/h3pg-offline-tarball/build -DCMAKE_BUILD_TYPE=Release -DFETCHCONTENT_FULLY_DISCONNECTED=ONcmake --build /tmp/h3pg-offline-tarball/build -j32