feat: add older versions of the wrappers extension
#1748
Conversation
yvan-sraka
changed the title
wrappers extensionwrappers extension
yvan-sraka
force-pushed
the
multi-version-ext/wrappers-v0.2.0
branch
from
ecf641f to
ff4cef2
Compare
yvan-sraka
force-pushed
the
custom-github-runners
branch
16 times, most recently
from
1eb74b8 to
db1e5e4
Compare
jfroche
force-pushed
the
custom-github-runners
branch
8 times, most recently
from
0878bd2 to
b842e89
Compare
samrose
force-pushed
the
update-nixpkgs
branch
from
771718f to
4e32d5b
Compare
jfroche
force-pushed
the
update-nixpkgs
branch
from
acf3ba4 to
42b0c29
Compare
samrose
force-pushed
the
update-nixpkgs
branch
from
fa26b67 to
0131d17
Compare
jfroche
force-pushed
the
multi-version-ext/wrappers-v0.2.0
branch
from
afb2c34 to
c719c72
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (17)
📒 Files selected for processing (11)
WalkthroughUpdates PostgreSQL release identifiers, extends cargo-pgrx version records, adds a new Python tool and docs to automate nix extension version tracking, enriches nix/ext/versions.json with many wrapper releases, and adjusts wrapper Nix build logic plus multiple wrapper dependency-bump patches. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Script as "update_versions_json.py"
participant Git as "Remote Git Repo"
participant Prefetch as "nix-prefetch-git"
participant NixHash as "nix hash --to-base64"
participant FS as "nix/ext/versions.json"
User->>Script: run(name, repo_url [, --ignore])
Script->>Git: fetch tags
Git-->>Script: tags list
loop for each new tag
Script->>Prefetch: nix-prefetch-git --url <repo> --rev <tag>
Prefetch-->>Script: commit_hash
Script->>NixHash: nix hash --to-base64 <commit_hash>
NixHash-->>Script: base64_sri
Script->>FS: load existing versions.json
Script->>FS: update/add entry (version → hash/meta)
end
Script->>FS: write sorted versions.json
Script-->>User: exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nix/ext/wrappers/default.nix (1)
86-100: Duplicate condition for version 0.5.4 will cause unreachable code.Lines 86-90 and 91-95 both check for version
"0.5.4". The second block (lines 91-95) will never be reached. Based on the pattern, this appears to be a copy-paste error.Looking at the sequence: 0.5.2, 0.5.3, 0.5.4, 0.5.4 (duplicate), 0.5.5, 0.5.6, 0.5.7 — the second 0.5.4 block should likely be removed or updated to a different version if there's a missing version in the chain.
🐛 Proposed fix - remove duplicate condition
else if builtins.compareVersions "0.5.4" version == 0 then { "clickhouse-rs-1.1.0-alpha.1" = "sha256-nKiGzdsAgJej8NgyVOqHaD1sZLrNF1RPfEhu2pRwZ6o="; "iceberg-catalog-s3tables-0.5.1" = "sha256-1JkB2JExukABlbW1lZPolNQCYb9URi8xNYY3APmiGq0="; } - else if builtins.compareVersions "0.5.4" version == 0 then - { - "clickhouse-rs-1.1.0-alpha.1" = "sha256-nKiGzdsAgJej8NgyVOqHaD1sZLrNF1RPfEhu2pRwZ6o="; - "iceberg-catalog-s3tables-0.5.1" = "sha256-1JkB2JExukABlbW1lZPolNQCYb9URi8xNYY3APmiGq0="; - } else if builtins.compareVersions "0.5.5" version == 0 then
🤖 Fix all issues with AI agents
In `@nix/ext/scripts/update_versions_json.py`:
- Around line 1-2: The shebang on the first line has an extra space ("#!
/usr/bin/env ...") which can break execution; edit the top of the script to
remove the space so the first line reads "#!/usr/bin/env nix-shell" (leave the
second nix-shell directive with flags unchanged) to ensure the script is
executable by the system interpreter lookup.
- Around line 22-37: The get_tags function currently ignores lines containing
"^{ }" and therefore uses tag-object hashes for annotated tags; update get_tags
to prefer peeled commit hashes by collecting both normal and peeled entries from
git ls-remote --tags output and, when a peeled line (ref ending with "^{ }")
exists for a tag, use its commit hash instead of the tag-object hash; keep the
existing behavior for lightweight tags by falling back to the non-peeled hash
when no peeled entry is present and ensure you still validate tags via
parse_version/InvalidVersion as before.
🧹 Nitpick comments (1)
nix/ext/scripts/update_versions_json.py (1)
13-15: MakeVERSIONS_JSON_PATHlocation-independent.Line 14 and the load/save logic are CWD-relative; running from repo root would write a stray
versions.json. Resolving the path relative to this script avoids accidental miswrites.♻️ Proposed refactor
-POSTGRES_VERSIONS: List[str] = ["15", "17"] -VERSIONS_JSON_PATH = "versions.json" +POSTGRES_VERSIONS: List[str] = ["15", "17"] +VERSIONS_JSON_PATH = Path(__file__).resolve().parents[1] / "versions.json" @@ - if not Path(VERSIONS_JSON_PATH).exists(): + if not VERSIONS_JSON_PATH.exists(): return {} - with open(VERSIONS_JSON_PATH, "r", encoding="utf-8") as f: + with VERSIONS_JSON_PATH.open("r", encoding="utf-8") as f: return json.load(f) @@ - with open(VERSIONS_JSON_PATH, "w", encoding="utf-8") as f: + with VERSIONS_JSON_PATH.open("w", encoding="utf-8") as f: json.dump(sorted_data, f, indent=2) f.write("\n")Also applies to: 46-50, 74-75
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
nix/ext/wrappers/v0.1.10/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.11/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.12/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.14/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.15/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.16/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.17/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.18/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.19/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.4/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.5/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.6/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.7/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.8/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.9/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.2.0/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.3.0/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
ansible/vars.ymlnix/cargo-pgrx/versions.jsonnix/docs/update-extension.mdnix/ext/scripts/update_versions_json.pynix/ext/versions.jsonnix/ext/wrappers/default.nixnix/ext/wrappers/v0.1.10/0001-bump-aws-config-to-0.55.3.patchnix/ext/wrappers/v0.1.4/0001-bump-gcp-bigquery-client-to-0.16.8.patchnix/ext/wrappers/v0.1.4/0002-bump-pgrx-to-0.6.1.patchnix/ext/wrappers/v0.2.0/0001-bump-pgrx-to-0.11.3.patchnix/ext/wrappers/v0.3.0/0001-bump-pgrx-to-0.11.3.patch
🧰 Additional context used
🪛 Ruff (0.14.11)
nix/ext/scripts/update_versions_json.py
1-1: Shebang should contain python, pytest, or uv run
(EXE003)
2-2: Shebang should be at the beginning of the file
(EXE005)
18-18: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: nix-eval / eval
🔇 Additional comments (14)
nix/ext/wrappers/v0.1.10/0001-bump-aws-config-to-0.55.3.patch (2)
1172-1186: Cargo.toml dependency updates are correct.The AWS SDK dependency bumps are straightforward and the
optionalflags are preserved for thes3_fdwfeature.
1-1171: Cargo.lock updates are safe and appropriate for the AWS SDK dependency bump.The lockfile changes correctly reflect the version upgrades from the patch:
- aws-config: 0.54.1 → 0.55.3 (no CVE)
- aws-sdk-s3: 0.24.0 → 0.25.1 (no CVE)
- tokio: 1.25.0 → 1.29.1 (no CVE; CVE-2024-27308 only affects 1.30.0+)
- hyper: 0.14.20 → 0.14.32 (safe; older CVEs fixed by 0.14.10)
The removal of test-only dependencies (
aws-smithy-protocol-test,pretty_assertions) reduces the attack surface. The addition ofsyn 2.xalongsidesyn 1.xmaintains compatibility with both old and new macro crates. No security advisories were found for the versions introduced in this patch.nix/cargo-pgrx/versions.json (1)
2-35: Confirm the new cargo hashes are reproducible.Line 2-35 introduces new versions and toolchain mappings; a stale
hash/cargoHashwill break Nix builds. Please re-run your hash generation workflow to validate these values.ansible/vars.yml (1)
13-15: Verify the-old-wrappers-extsuffix matches published artifacts.Line 13-15 now appends the suffix to release identifiers; confirm downstream package names or download lookups are produced with the same suffix to avoid missing binaries.
nix/ext/versions.json (1)
584-773: Validate new wrappers version entries against real tags and assets.Line 584-773 adds many historical wrappers releases. Please ensure each hash corresponds to the correct tag and that the per-version patch/lockfile artifacts expected by the Nix build exist for those versions.
nix/ext/wrappers/v0.2.0/0001-bump-pgrx-to-0.11.3.patch (1)
8-218: Confirm the pgrx/bindgen bump still builds with the pinned toolchain.Line 8-218 updates pgrx to 0.11.3 and refreshes the lockfile (bindgen 0.69.5, etc.). These bumps can change MSRV/clang requirements, so please run the v0.2.0 wrapper build with rust 1.76.0 to ensure the Nix env still satisfies dependencies.
nix/ext/scripts/update_versions_json.py (1)
17-19: Core CLI flow and hashing logic look solid.Also applies to: 40-43, 53-66, 79-87
nix/docs/update-extension.md (1)
228-245: LGTM!Clear documentation for the new automation script with a practical two-step workflow. The example commands and verification steps are helpful.
nix/ext/wrappers/v0.1.4/0001-bump-gcp-bigquery-client-to-0.16.8.patch (1)
1-936: LGTM!The patch correctly bumps
gcp-bigquery-clientfrom a git source to version 0.16.8 on crates.io, which improves reproducibility. The transitive dependency updates are consistent with this change, and the addition of theall_fdwsfeature is useful for building all FDWs together.nix/ext/wrappers/v0.1.4/0002-bump-pgrx-to-0.6.1.patch (1)
1-1137: LGTM!The patch correctly upgrades from pgx 0.5.6 to 0.6.1 with appropriate API changes:
Datumimport path updated topgx::pg_sys::Datum- Logging helpers updated to use
ereport!macro andlog!macro- Default PostgreSQL version bumped from pg14 to pg15
The removal of pg10 support and shutdown_hooks dependency aligns with pgrx 0.6.x requirements.
nix/ext/wrappers/default.nix (4)
57-66: LGTM!The postPatch and lockFile resolution logic correctly handles version-specific Cargo.lock files. This is necessary because patches may modify dependencies, causing the original lockfile to mismatch. The conditional copy in postPatch ensures the patched lockfile is used when available.
118-119: LGTM!The
pgrxBinaryNameattribute correctly handles the pgx → pgrx binary rename that occurred around version 0.7.4. For older pgrx versions (< 0.7.4), the binary is named "pgx"; for newer versions, it's "pgrx".
181-187: Version-specific patches look appropriate.Each version has isolated patches in its own directory, which is maintainable. The comment on lines 181-182 noting the Rust 1.80.0 inference issue with
timecrate is helpful context.
201-205: LGTM!The updated filter correctly excludes versions marked with
ignore: truein versions.json, which is useful for temporarily disabling versions (e.g., for CI as mentioned in the PR objectives) without removing them entirely.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/ext/scripts/update_versions_json.py`:
- Around line 46-49: The for-loop over tags uses an unused variable commit_hash;
change the loop to for tag, _ in tags.items(): (or for tag in tags:) so the
intent is clear and the unused variable is removed—update the loop that
references tags and peeled_tags (the block starting with "for tag, commit_hash
in tags.items():") to use an underscore or iterate only over tag when checking
and assigning peeled_tags[tag].
🧹 Nitpick comments (1)
nix/ext/scripts/update_versions_json.py (1)
17-19: Consider adding a timeout to prevent indefinite hangs.Network operations like
git ls-remoteandnix-prefetch-gitcould hang indefinitely on network issues. Adding a timeout improves robustness for this maintenance tool.♻️ Proposed fix
-def run(cmd: List[str]) -> str: - result = subprocess.run(cmd, capture_output=True, text=True, check=True) +def run(cmd: List[str], timeout: int = 300) -> str: + result = subprocess.run(cmd, capture_output=True, text=True, check=True, timeout=timeout) return result.stdout.strip()
... which is handy when it comes to compute hashes for every source tarball in a bulk :)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
yvan-sraka
force-pushed
the
multi-version-ext/wrappers-v0.2.0
branch
from
bb1f5aa to
8e8b98e
Compare
This PR add a few commits on top of #1743
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.