-
-
Notifications
You must be signed in to change notification settings - Fork 227
Update nixpkgs #1714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Update nixpkgs #1714
Conversation
a3275e8 to
98633c4
Compare
|
rebased on develop, and will e2e test ami for each major version |
337be4e to
dab9e52
Compare
c061085 to
07fb5e2
Compare
6b51806 to
a33cf5d
Compare
684c4b7 to
21a8f63
Compare
37962e0 to
52722b1
Compare
0ed7d0d to
359ac18
Compare
359ac18 to
d64c0b7
Compare
samrose
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding request changes just to stop merge until fully tested
f470b85 to
dc596ec
Compare
…64-darwin Version 0.19.5 has a dangling pointer issue in src/core.c:177 that causes compilation to fail on aarch64-darwin with newer clang versions. This adds -Wno-error=dangling-assignment to allow compilation to proceed. The flag is restricted to aarch64-darwin only since GCC on Linux doesn't recognize this warning option.
Add -headerpad_max_install_names linker flag to prevent 'install_name_tool: changing install names or rpaths can't be redone' errors on macOS. This allocates sufficient header space for install_name_tool to modify the shared library during post-install processing.
… extensions
Test assertions were inconsistent: expecting {version}.so and {extension_name}-{version}.so
patterns while actual libraries use {lib_name}-{version}.so (e.g. pg_partman_bgw-5.3.1.so,
timescaledb-loader-2.16.1.so). Updated PostgresExtensionTest to accept Optional[str] lib_name
parameter and use consistent {lib_name}-{version}.so assertions throughout.
Added missing timescaledb-loader.so symlink creation and updated the switch script to
maintain both symlinks when switching versions.
Fixes 'Expected timescaledb version 2.16.1, but found timescaledb-loader-2.16.1.so' and
'Expected pg_partman version 5.3.1, but found pg_partman_bgw-5.3.1.so' test failures.
Use overlay instead of passing full nixpkgs-oldstable to avoid exposing entire old nixpkgs attribute set. Now provides specific packages (curl_8_4, v8_oldstable) through overlay.
Keep revision support that was accidentally simplified during nixpkgs update. Restores conditional logic to support building orioledb from specific git revision hashes instead of only tags.
These files were moved to subdirectories and the old .nix files should be removed since they were moved to new directory structure with default.nix files.
Remove nixpkgs-go124 dependency since current nixpkgs has Go 1.25.4 which is sufficient for packer. Remove nix-fast-build input as unused and add follows directives to resolve duplicate dependencies.
Run flake-linter to eliminate duplicate dependencies, and remove unused cargo-pgrx / rustc versions hashes.
Move pgrouting.nix to pgrouting/default.nix and groonga package to pgroonga/groonga.nix. Update imports and fix relative paths for versions.json and patch files.
Replace inline bash script generation with writeShellScriptBin for rustc wrapper that filters empty postmaster_stub.rs arguments. Apply wrapper only for pgrx < 0.12 since issue was fixed upstream in pgrx#1435 and pgrx#1441.
Move nixpkgs-oldstable import to a let binding to avoid importing it three times...
Remove supabase-groonga from global flake packages and import it locally in pgroonga extension instead. Other components access groonga via pgroonga.passthru.groonga since groonga is only used by pgroonga.
The supabase-groonga package was moved to pgroonga extension in commit 1f0ed1f, but Ansible was still trying to install it separately causing build failures.
This reverts commit 55474686405394826ad07a032cbcf0e36a924554.
This reverts commit cfa9bcb92219f391469ae743f95c8856b0a99561.
Remove references to darwin.apple_sdk.frameworks.{IOKit,CoreFoundation}
which have been deprecated in nixpkgs. Disable CGO to avoid Darwin
framework dependencies entirely.
With this new nixpkgs update come gcc 15, which introduced stricter C standard compliance. plpgsql-check fails to build due to this. To fix the build, we explicitly set the C standard to gnu17.
acf3ba4 to
42b0c29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @nix/cargo-pgrx/buildPgrxExtension.nix:
- Line 151: The Darwin-specific Security framework was removed from buildInputs
(buildInputs and darwin.apple_sdk.frameworks.Security) without explanation;
either restore it for macOS builds or add a clear comment/commit note proving
it's unnecessary and that RUSTFLAGS-based handling (RUSTFLAGS / stdenv.isDarwin)
still satisfies all macOS link-time requirements. Fix by making buildInputs
conditional on stdenv.isDarwin to include darwin.apple_sdk.frameworks.Security
when building on Darwin, or if you intentionally removed it, add a comment and
update the commit message explaining why macOS linking remains correct without
the framework and reference the RUSTFLAGS changes that replace it.
In @nix/ext/pg_jsonschema/default.nix:
- Around line 83-93: The doCheck conditional currently disables tests for
versions ["0.2.0","0.3.1","0.3.3"], which reduces coverage; inspect the test
harness in those specific releases (look at version tag checks and pgrx test
invocation in the repo) to confirm why they fail, and either (A) add a targeted
alternative test path for those versions (for example implement a custom
installcheck script similar to pg_graphql and wire it into the build for the
affected versions), or (B) modify the Nix build to run pgrx tests outside the
sandbox/with sandbox disabled in CI for those versions, and if neither is
feasible add an explicit NOTE in the commit/PR describing the untested versions
and the mitigation chosen; update the doCheck clause only after implementing one
of these fixes and reference the version list and doCheck symbol in the commit
message.
In @nix/ext/pgrouting/default.nix:
- Around line 61-69: The cmakeFlags array includes an unexplained
"-DCMAKE_POLICY_VERSION_MINIMUM=3.5" which conflicts with pgrouting's own
cmake_minimum_required (3.2 for older releases or 3.12 currently); either remove
that flag from cmakeFlags or replace it with a brief comment explaining why 3.5
is required for this package build and which pgrouting versions need it; locate
and edit the cmakeFlags definition (the cmakeFlags variable and the
"-DCMAKE_POLICY_VERSION_MINIMUM=3.5" entry) or add an adjacent comment
clarifying the rationale and affected pgrouting versions.
In @nix/ext/plv8/default.nix:
- Around line 38-48: The inner per-derivation v8 attribute and duplicate
conditional in buildInputs are dead/redundant because all versions <= 3.1.10, so
remove the conditional v8 attribute inside the build (the v8 = (if
(builtins.compareVersions "3.1.10" version >= 0) then v8 else nodejs_20.libv8)
line and any duplicate conditional in buildInputs) and rely on the outer v8 =
v8_oldstable binding; if you want future-proofing instead of removing logic,
centralize version-specific selection into a single helper (e.g.,
selectV8(version)) used by the build function rather than duplicating
conditionals in stdenv.mkDerivation and buildInputs.
In @testinfra/test_ami_nix.py:
- Around line 790-792: The test currently uses "assert False, (\"PostgREST logs
contain 'session is not read-only' errors even though PostgreSQL is configured
for read-only mode\")" which can be stripped out with Python's -O optimization;
replace that assert with an explicit exception by raising AssertionError with
the same message (i.e., use raise AssertionError("PostgREST logs contain
'session is not read-only' errors even though PostgreSQL is configured for
read-only mode")) so the failure cannot be bypassed by optimizations and still
provides the same diagnostic text.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nix/ext/pg_jsonschema/default.nix (1)
48-57: Fix non-existent versions in previousVersions array.The
previousVersionsarray includes versions that don't exist innix/ext/versions.json: 0.3.0, 0.1.4, 0.1.3, and 0.1.2. These missing versions will cause build failures when the postBuild script attempts to generate migration files.Only these versions are available: 0.1.0, 0.1.1, 0.2.0, 0.3.1 (and 0.3.3, which appears to be the current version).
Update the
previousVersionsarray to include only versions that exist inversions.json.
♻️ Duplicate comments (4)
migrations/schema-15.sql (1)
640-640: LGTM - Already discussed and accepted.The change from
character varying(128)tocharacter varyingis fully explained in the past review comments. This results from updating dbmate (2.14.0 → 2.28.0) as part of the nixpkgs upgrade, and the team has accepted this change. The modification is consistent across all three schema migration files (schema-15.sql, schema-17.sql, and schema-orioledb-17.sql).flake.nix (1)
10-29: Verify no duplicate dependencies after input restructuring.Per previous review discussion, consider running
nix run github:Mic92/flake-linteron this branch to ensure the restructured inputs don't introduce duplicate dependencies in the flake lock.nix/ext/tests/plpgsql_check.nix (1)
22-26: Same withJIT/withoutJIT pattern as other test files.This file exhibits the same pattern where both
withJITandwithoutJITpoint to the samepkg. See the review comment on nix/ext/tests/pgjwt.nix for the detailed concern about JIT differentiation.nix/ext/tests/postgis.nix (1)
22-26: Same withJIT/withoutJIT pattern as other test files.This file exhibits the same pattern where both
withJITandwithoutJITpoint to the samepkg. See the review comment on nix/ext/tests/pgjwt.nix for the detailed concern about JIT differentiation.
🧹 Nitpick comments (5)
nix/packages/lib.nix (1)
118-126: Defensive error handling is a solid improvement.The explicit copy operations with error messages and exit-on-failure make build failures easier to diagnose. The success message on line 126 confirms the copy phase completed.
One minor observation:
pg_ident.conf(line 123) and the extension scripts (line 124) don't receive explicitchmodoperations like the other config files (lines 127-130). If default permissions are sufficient, this is fine; otherwise, consider addingchmod 644for consistency.nix/ext/tests/orioledb.nix (1)
18-22: Clarify JIT toggle implementation.The
installedExtensionsattribute correctly exposes the extension metadata. However, bothwithJITandwithoutJITpoint to the samepkgvariable, which means no actual JIT toggling occurs. If this is a placeholder for future JIT support or if JIT toggling isn't relevant for orioledb tests, please confirm this is the intended behavior.nix/ext/tests/pgmq.nix (1)
22-26: Clarify JIT toggle implementation.The
installedExtensionsattribute correctly exposes the extension metadata. However, bothwithJITandwithoutJITpoint to the samepkgvariable, which means no actual JIT toggling occurs. If this is a placeholder for future JIT support or if JIT toggling isn't relevant for pgmq tests, please confirm this is the intended behavior.nix/postgresql/generic.nix (1)
345-345: Clarify intent of emptyteamsarray.Setting
teams = [ ]appears intentional but unconventional. If this is meant to satisfy a required schema field or explicitly indicate no team ownership, a brief comment would help future maintainers understand the intent.nix/ext/plv8/default.nix (1)
136-158: Consider consolidating duplicate patchelf logic.The patchelf commands in both the PostgreSQL 15 branch (lines 144-146) and the else branch (lines 155-157) are identical. If no differentiation is planned, this could be simplified.
🔎 Suggested simplification
- if [ "${lib.versions.major postgresql.version}" = "15" ]; then - ${lib.optionalString stdenv.isDarwin '' - install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME - install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME - install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME - install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME - ''} - - ${lib.optionalString (!stdenv.isDarwin) '' - ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME - ''} - else - ${lib.optionalString stdenv.isDarwin '' - install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME - install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME - install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME - install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME - ''} - - ${lib.optionalString (!stdenv.isDarwin) '' - ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME - ''} - fi + ${lib.optionalString stdenv.isDarwin '' + install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME + install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME + install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME + install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME + ''} + + ${lib.optionalString (!stdenv.isDarwin) '' + ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME + ''}
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
ansible/vars.ymlflake.nixmigrations/schema-15.sqlmigrations/schema-17.sqlmigrations/schema-orioledb-17.sqlnix/cargo-pgrx/buildPgrxExtension.nixnix/cargo-pgrx/default.nixnix/cargo-pgrx/versions.jsonnix/checks.nixnix/ext/gdal.nixnix/ext/pg_jsonschema/default.nixnix/ext/pg_net.nixnix/ext/pgaudit.nixnix/ext/pgroonga/default.nixnix/ext/pgroonga/groonga.nixnix/ext/pgrouting/default.nixnix/ext/pgrouting/pgrouting-3.4.1-pg17.patchnix/ext/plpgsql-check.nixnix/ext/plv8/default.nixnix/ext/postgis.nixnix/ext/tests/default.nixnix/ext/tests/http.nixnix/ext/tests/lib.pynix/ext/tests/orioledb.nixnix/ext/tests/pg_plan_filter.nixnix/ext/tests/pg_repack.nixnix/ext/tests/pg_safeupdate.nixnix/ext/tests/pgjwt.nixnix/ext/tests/pgmq.nixnix/ext/tests/pgroonga.nixnix/ext/tests/pgrouting.nixnix/ext/tests/pgsodium.nixnix/ext/tests/plpgsql_check.nixnix/ext/tests/plv8.nixnix/ext/tests/postgis.nixnix/ext/tests/timescaledb.nixnix/ext/tests/vault.nixnix/ext/timescaledb.nixnix/ext/vault.nixnix/ext/wal2json.nixnix/ext/wrappers/default.nixnix/nixpkgs.nixnix/overlays/default.nixnix/packages/default.nixnix/packages/groonga/do-not-use-vendored-libraries.patchnix/packages/groonga/fix-cmake-install-path.patchnix/packages/lib.nixnix/packages/packer.nixnix/packages/postgres.nixnix/packages/sfcgal.nixnix/packages/supascan.nixnix/packages/sync-exts-versions.nixnix/postgresql/generic.nixtestinfra/test_ami_nix.py
💤 Files with no reviewable changes (4)
- nix/packages/sfcgal.nix
- nix/overlays/default.nix
- nix/packages/groonga/do-not-use-vendored-libraries.patch
- nix/packages/groonga/fix-cmake-install-path.patch
🧰 Additional context used
🪛 Ruff (0.14.10)
testinfra/test_ami_nix.py
790-790: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🔇 Additional comments (59)
nix/ext/pgroonga/default.nix (1)
60-61: LGTM! Formatting improvement.The reformatting of the
buildInputslist improves readability while maintaining identical semantics.nix/ext/gdal.nix (1)
50-57: LGTM! Formatting improvement.The reformatting of
cmakeFlagsinto a flat list with clearly separated platform-specific options improves readability without altering behavior.nix/ext/pgaudit.nix (1)
187-200: LGTM! Formatting improvement.The reflowed indentation improves readability while preserving the identical logic for conditional migration symlink creation.
nix/ext/vault.nix (1)
47-70: LGTM! Formatting improvement.The reformatted
installPhaseimproves readability through clearer indentation and line breaks while maintaining identical build logic.migrations/schema-orioledb-17.sql (1)
655-655: LGTM - Coordinated schema change across all migration files.The removal of the length constraint on the
versioncolumn (fromcharacter varying(128)tocharacter varying) is consistent across all three schema migration files in this PR. As explained in the past review comments on schema-15.sql, this change results from updating dbmate from version 2.14.0 to 2.28.0, and the team has accepted this change to keep the tool up to date.migrations/schema-17.sql (1)
641-641: LGTM - Coordinated schema change.This change mirrors the identical modification in schema-15.sql and schema-orioledb-17.sql, resulting from the dbmate version update (2.14.0 → 2.28.0) included in the nixpkgs upgrade.
nix/packages/lib.nix (2)
103-104: LGTM!The formatting adjustment and inline comment improve readability without changing behavior.
133-137: LGTM!The multi-line reformatting of the nested
builtinsexpression improves readability without altering the substitution logic.ansible/vars.yml (1)
13-15: These version changes are intentional as part of the nixpkgs update and require no action.The version string updates on lines 13-15 reflect a rebuild with updated nixpkgs dependencies (commit
acf3ba4: feat: update nixpkgs and fix plpgsql-check build with gcc 15). The build numbers (064, 021) represent iteration numbers for the nixpkgs update cycle (indicated by the-update-nixpkgs-5suffix), not a rollback. These versions are generated by the nix build system and used directly in the AMI provisioning pipeline; they will exist after the build completes. No verification of pre-existing artifacts is needed.Likely an incorrect or invalid review comment.
nix/ext/wal2json.nix (1)
13-13: This review comment is incorrect. The versions.json file for wal2json does not containrevfields—onlypostgresqlandhashentries. The parameter rename to_revcorrectly reflects that the passed value is unused, and the revision is deterministically computed from the version string on line 20. No silent behavior change occurs because there were norevvalues in versions.json to ignore in the first place.Likely an incorrect or invalid review comment.
nix/ext/pg_jsonschema/default.nix (1)
45-45: BuildInputs simplification is sound. The change to include onlypostgresqlaligns with the PR's goal of reducing platform-specific dependencies. Darwin-specific build configuration is preserved in theenvblock (lines 60-69) with appropriateRUSTFLAGSfor dynamic linking on macOS.testinfra/test_ami_nix.py (1)
435-437: LGTM: Assertion formatting is clear and concise.The single-line assertion format improves readability while preserving the same truth conditions and error messages.
Also applies to: 560-562, 592-594, 627-629
nix/ext/plpgsql-check.nix (1)
43-44: The gnu17 C standard flag correctly resolves GCC 15 build issues.GCC 15 defaults to C23 (gnu23), which breaks PostgreSQL extensions expecting gnu17 behavior. Setting
-std=gnu17is the recommended approach per GCC 15 porting documentation and is widely used across PostgreSQL extension builds. The fix is appropriate and compatible with all supported versions.nix/packages/sync-exts-versions.nix (1)
14-15: nix_2_29 is available and stable in nixpkgs.Verification confirms that
nixVersions.nix_2_29is available in nixpkgs with multiple stable releases (2.29.0, 2.29.1, 2.29.2, 2.29.3). The version jump fromnix_2_20tonix_2_29is a significant upgrade, but the target version is a documented, stable release suitable for this tooling.nix/ext/tests/pgroonga.nix (1)
20-27: LGTM! Passthru attributes align with the broader test environment pattern.The added
installedExtensions,withJIT, andwithoutJITpassthru attributes provide consistency across PostgreSQL extension test environments. HavingwithJITandwithoutJITpoint to the samepkgis appropriate for this test context where JIT toggling isn't exercised.nix/postgresql/generic.nix (3)
118-126: Good: ICU version pinned to maintain collation stability.Pinning ICU to version 75 with the clear comment about collation version 153.120 is a sound approach to prevent collation mismatch warnings during nixpkgs upgrades. This is important for database integrity.
69-69: Dynamic OrioleDB detection looks correct.The pattern
"[0-9][0-9]_.*"correctly matches OrioleDB version strings like"17_1"while not matching standard PostgreSQL versions like"17.6.1".
159-159: No compatibility issues identified with__structuredAttrs = true.The derivation correctly implements structured attributes: environment variables are properly placed under the
envattribute (e.g.,env.NIX_CFLAGS_COMPILE),configureFlagsandbuildFlagsare defined as lists, and no custom build hooks make assumptions about environment variable exports. PostgreSQL's standard autoconf configure script works correctly with this approach. The implementation follows nixpkgs best practices.nix/ext/tests/lib.py (2)
24-42: Good addition oflib_nameparameter for flexible library naming.The
lib_nameparameter with fallback toextension_namemaintains backward compatibility while supporting extensions where the shared library name differs from the extension name (e.g., timescaledb-loader). Clean implementation.
169-171: Address inconsistent lib_name parameter passing across test files.The pattern change from
{version}.soto{lib_name}-{version}.sois correct. However, test files use inconsistent approaches:
default.nixandtimescaledb.nixexplicitly passlib_nametoPostgresExtensionTest- Individual test files (
pgjwt.nix,plv8.nix,pgmq.nix, etc.) omit thelib_nameparameter, relying on the default wherelib_name = extension_nameThis works only when the library name matches the extension name. For consistency and to support extensions with different library names (like
timescaledb-loader), all callers should explicitly passlib_namefrom extension metadata, following thedefault.nixpattern.nix/packages/supascan.nix (2)
1-4: Good simplification of Go toolchain binding.Using
pkgs.go_1_24directly instead of a separate nixpkgs input aligns with the broader flake input consolidation in this PR.
41-42: Disabling CGO is appropriate for supascan.
CGO_ENABLED = "0"correctly removes Darwin framework dependencies while maintaining full functionality. Supascan is a Linux infrastructure tool that reads/proc,/sys, and filesystem metadata—all pure-Go operations. The gopsutil library used for optional process/port scanning is implemented in pure Go on Linux without requiring CGO. No functionality is lost with this change.flake.nix (1)
22-25: Good: nixpkgs-oldstable pin with clear documentation.The comment at lines 22-24 clearly explains why this specific nixpkgs version is pinned (compatible v8 and curl versions for extensions). This provides important context for future maintainers.
nix/nixpkgs.nix (1)
13-25: Good implementation addressing previous review feedback.The overlay correctly uses a let-binding to import
nixpkgs-oldstableand exposes only the specific packages needed (curl_8_6,v8_oldstable) rather than the full attribute set. This follows the suggestions from previous reviews.nix/checks.nix (1)
4-10: LGTM! Cosmetic reordering of perSystem arguments.Moving
libafterpkgsin the destructuring pattern is a stylistic change with no functional impact.nix/ext/pg_net.nix (2)
10-15: LGTM! Curl versioning approach addresses past feedback.The curl_8_6 parameter with let-binding follows the overlay-based approach suggested in the past review comment, providing a cleaner interface than passing the full nixpkgs-oldstable attributeset.
52-59: LGTM! Version and platform-specific compiler flag handling is appropriate.The multi-branch conditional correctly handles compiler warning suppression for different versions and platforms. The Darwin aarch64-specific case is well-documented.
nix/ext/tests/pgjwt.nix (1)
22-26: Verify withJIT and withoutJIT implementation.Both
withJITandwithoutJITattributes point to the samepkg, which suggests JIT compilation is not yet differentiated. This pattern appears across multiple test files (plpgsql_check.nix, postgis.nix, and others mentioned in the AI summary).Please confirm whether:
- This is an intentional placeholder awaiting future JIT support implementation, or
- JIT-enabled and JIT-disabled PostgreSQL builds should reference distinct packages
If these are intended as placeholders, consider adding a TODO comment to track the future implementation of proper JIT toggle support.
nix/cargo-pgrx/versions.json (1)
1-119: Cargo hash updates look correct.The
cargoHashvalues have been updated across multiple cargo-pgrx and Rust version combinations, which is expected when rebuilding with updated nixpkgs. No structural changes were made to the JSON.nix/cargo-pgrx/default.nix (2)
73-73: Cargo hash updates are correct.The
cargoHashvalues for versions 0.12.6, 0.12.9, and 0.14.3 match the corresponding updates inversions.json.Also applies to: 78-78, 83-83
36-36: Remove this comment — the premises are factually incorrect.The Security framework is not present in the current code and has no history of being in this file. Additionally, lines 47–50 contain
checkFlagswith test directives, not Darwin-specific environment variables. The OPENSSL variables (lines 38–41) are set unconditionally for all platforms, not Darwin-specific.Likely an incorrect or invalid review comment.
nix/ext/pgrouting/pgrouting-3.4.1-pg17.patch (1)
1-39: PostgreSQL 17 compatibility patch is correct.The patch correctly addresses the PostgreSQL 17 API change where
SPI_getvaluenow returnschar*directly instead ofDatum. Removing theDatumGetCStringwrapper resolves the compilation error and aligns with the PG17 API.nix/ext/wrappers/default.nix (1)
39-42: Verify Darwin builds are supported without frameworks in buildInputs.The
wrapperspackage does not include Darwin frameworks inbuildInputs—onlyopensslandpostgresql. However, Darwin-specific support is properly maintained through environment variables at lines 47-50 (POSTGRES_LIBandRUSTFLAGSwithdynamic_lookup), which are the correct approach for pgrx extensions on Darwin. Verify that the build succeeds on Darwin with this configuration.nix/ext/pgroonga/groonga.nix (1)
1-1: groonga.nix is a necessary dependency file and should not be removed.This file builds the Groonga library package (
supabase-groonga) that is then consumed bynix/ext/pgroonga/default.nix, which builds the PGroonga PostgreSQL extension. These are two distinct, complementary files serving different purposes:
groonga.nix: Builds the Groonga C/C++ library using CMakedefault.nix: Builds the PGroonga PostgreSQL extension that depends on the Groonga libraryThe file is actively referenced in
nix/packages/default.nix:44and cannot be merged or removed without breaking the build chain.Likely an incorrect or invalid review comment.
nix/ext/tests/pgsodium.nix (1)
23-23: LGTM with a note on JIT attributes.The addition of
installedExtensions,withJIT, andwithoutJITpassthru attributes follows a consistent pattern applied across multiple test files in this PR.Note: Both
withJITandwithoutJITcurrently point to the samepkg. If these attributes are intended to provide distinct PostgreSQL builds with different JIT configurations, this may need adjustment. However, if JIT toggling occurs elsewhere or these are placeholders for future functionality, the current implementation is acceptable.Also applies to: 26-27
nix/ext/tests/http.nix (1)
22-22: LGTM - consistent passthru attributes.The passthru attributes (
installedExtensions,withJIT,withoutJIT) match the pattern consistently applied across extension test files in this PR.Also applies to: 25-26
nix/ext/tests/plv8.nix (1)
23-23: LGTM - consistent passthru attributes.The passthru attributes align with the systematic changes applied across all extension test files.
Also applies to: 26-27
nix/ext/pgrouting/default.nix (2)
52-54: Patch file exists and is correctly referenced.The conditional patch for pgrouting 3.4.1 with PostgreSQL 17 is correctly scoped to the specific version combination. The patch file exists at
nix/ext/pgrouting/pgrouting-3.4.1-pg17.patchas expected.
15-15: The versions.json path is correct.The relative path
../versions.jsonfromnix/ext/pgrouting/default.nixcorrectly resolves tonix/ext/versions.json, which exists in the repository. The path change is aligned with the actual file location.nix/ext/postgis.nix (1)
19-19: sfcgal integration is correct. Thesfcgaldependency is properly added as a function parameter (line 19), included in buildInputs (line 81), and passed to the PostGIS configure script (line 94 with--with-sfcgal). The implementation correctly aligns with the PR goal of using nixpkgs packages as external dependencies. No version incompatibility issues are evident from the codebase.nix/ext/tests/pg_plan_filter.nix (1)
22-22: LGTM! Passthru standardization.The addition of
installedExtensions,withJIT, andwithoutJITattributes standardizes the public interface for PostgreSQL extension tests, aligning with the pattern applied across multiple test files in this PR.Also applies to: 25-26
nix/ext/tests/vault.nix (1)
23-23: LGTM! Passthru standardization.The addition of
installedExtensions,withJIT, andwithoutJITattributes standardizes the public interface, consistent with the pattern applied across extension test files.Also applies to: 26-27
nix/ext/tests/default.nix (2)
32-34: LGTM! Passthru standardization.The addition of
withJIT,withoutJIT, andinstalledExtensionsattributes aligns with the standardization pattern applied across extension test files.
171-171: PostgresExtensionTest signature correctly accepts lib_name parameter.The call at line 171 passes
lib_nameas the seventh argument, which matches the constructor signature innix/ext/tests/lib.py. The parameter is defined aslib_name: Optional[str] = Nonewith a default value, and the implementation is correct.nix/ext/tests/pg_repack.nix (1)
22-22: LGTM! Passthru standardization.The addition of
installedExtensions,withJIT, andwithoutJITattributes completes the standardization pattern applied consistently across PostgreSQL extension test files.Also applies to: 25-26
nix/packages/packer.nix (1)
9-9: This review comment is based on an incorrect premise about the change.The file
nix/packages/packer.nixwas newly created in this commit, not modified to remove a Go version override. There is no evidence of a Go 1.24 override being removed—the file was added fresh and contains no Go version specification. The derivation does use the default Go from nixpkgs viabuildGoModule = pkgs.buildGoModule, but this was not a result of removing an override; it's simply how the package was initially defined.If the intent is to verify that Packer 1.14.1 builds successfully with the updated nixpkgs, that is a reasonable verification step, but the comment's explanation about removing an override is incorrect.
Likely an incorrect or invalid review comment.
nix/ext/tests/pgrouting.nix (1)
26-26: LGTM: Passthru attributes extended consistently.The addition of
installedExtensions,withJIT, andwithoutJITto the passthru interface aligns with the PR's systematic pattern across multiple extension tests to expose extension metadata and JIT-toggle attributes.Also applies to: 29-30
nix/ext/tests/pg_safeupdate.nix (1)
22-22: LGTM: Passthru attributes extended consistently.The passthru additions mirror the pattern established across other extension tests in this PR, providing a uniform interface for extension metadata.
Also applies to: 25-26
nix/cargo-pgrx/buildPgrxExtension.nix (1)
88-108: Rustc wrapper implementation aligns with upstream fix backport.The rustc wrapper correctly filters empty arguments and empty
postmaster_stub.rsfiles for cargo-pgrx < 0.12.0, addressing build failures fixed upstream in pgcentralfoundation/pgrx#1435 and #1441. The version-gated activation (needsRustcWrapper) ensures the workaround only applies to affected versions.Based on past review comments, this is a known workaround for an issue already fixed in pgrx >= 0.12.
Also applies to: 117-119, 173-177
nix/ext/timescaledb.nix (2)
42-43: Darwin-specific linter flag looks appropriate.Conditionally disabling the linter on Darwin with
-DLINTER=OFFavoids potential Darwin-specific build issues while preserving linter functionality on other platforms.
111-111: Loader naming changes coordinate consistently across build and runtime contexts.The additions establish dual loader naming:
- Line 111 creates
timescaledb-loader.sosymlink in the version-switching script (runtime context)- Lines 129-130 create the same symlink during postBuild (build-time/test context)
- Line 150 exposes
libName = "timescaledb-loader"in passthru for programmatic referenceThis aligns with the test changes in
nix/ext/tests/timescaledb.nixthat reference the loader by this explicit name.Also applies to: 129-130, 150-150
nix/ext/tests/timescaledb.nix (2)
22-22: LGTM: Passthru attributes extended consistently.The passthru additions maintain consistency with other extension test files in this PR.
Also applies to: 25-26
91-91: Test invocation correctly aligned with loader naming changes.The expanded
PostgresExtensionTestcall with"public"and"timescaledb-loader"arguments (line 91) and the updated switch path totimescaledb-loader.so(line 97) correctly coordinate with the loader naming changes innix/ext/timescaledb.nix.Also applies to: 97-97
nix/packages/default.nix (2)
44-44: LGTM!The path change relocates
supabase-groongato../ext/pgroonga/groonga.nix, which logically groups the groonga dependency with its related pgroonga extension.
51-51: Code consolidation correctly implemented.The
pg-backrestpackage is now properly sourced from the maininputs.nixpkgs(revision c12c63cd6c5e, Jan 2025) with no separatenixpkgs-pgbackrestinput remaining. This aligns with the PR's goal of reducing multiple nixpkgs inputs.nix/ext/plv8/default.nix (4)
57-65: LGTM!The patches structure is clear: the system v8 patch is always applied, and the GUC redefinition fix is conditionally applied for versions ≤ 3.1.10.
67-78: LGTM on build inputs structure.The Darwin-specific native build inputs (
clang,xcbuild) are appropriately conditionally included. ThebuildInputscorrectly applies version-conditional v8 selection (though the consistency issue with other v8 references was flagged above).
82-93: LGTM on makeFlags structure.The Darwin-specific
SHLIB_LINKwith-headerpad_max_install_namesis correct for enablinginstall_name_toolmodifications post-build. The non-Darwin path uses the simpler-lv8linkage.
95-110: LGTM!The Darwin-specific
NIX_LDFLAGScorrectly includes framework dependencies and the-headerpad_max_install_namesflag to allow post-buildinstall_name_toolmodifications.
| buildInputs = | ||
| (args.buildInputs or [ ]) | ||
| ++ lib.optionals stdenv.hostPlatform.isDarwin [ darwin.apple_sdk.frameworks.Security ]; | ||
| buildInputs = (args.buildInputs or [ ]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for Darwin-specific build requirements or issues in cargo-pgrx usage
# Check for Darwin-specific configurations or dependencies in cargo-pgrx overlays
rg -n -C3 -g '*.nix' 'cargo-pgrx.*[Dd]arwin|[Dd]arwin.*cargo-pgrx' nix/
# Look for Darwin platform checks in cargo-pgrx related files
rg -n 'isDarwin' nix/cargo-pgrx/
# Check if there are any Darwin-specific frameworksInputs or apple_sdk references removed
rg -n -C3 'apple_sdk|frameworksInputs' nix/cargo-pgrx/ nix/overlays/Repository: supabase/postgres
Length of output: 250
🏁 Script executed:
#!/bin/bash
# Check git history for the buildInputs change
git log --oneline -p nix/cargo-pgrx/buildPgrxExtension.nix | head -200
# Also check the full context around line 151
sed -n '145,160p' nix/cargo-pgrx/buildPgrxExtension.nix
# And the context around line 179 to understand the Darwin handling
sed -n '175,185p' nix/cargo-pgrx/buildPgrxExtension.nixRepository: supabase/postgres
Length of output: 9069
🏁 Script executed:
#!/bin/bash
# Check if this file existed in different locations before
git log --all --full-history -- nix/cargo-pgrx/buildPgrxExtension.nix | head -20
# Also check the parent commit to see what was there before
git show acf3ba4^:nix/cargo-pgrx/ 2>/dev/null | head -50
# Look for any other buildPgrxExtension files in history
find . -name "*buildPgrxExtension*" -o -name "*cargo-pgrx*" 2>/dev/null | grep -E "\.(nix|md)$"Repository: supabase/postgres
Length of output: 707
🏁 Script executed:
#!/bin/bash
# Get the full diff for this specific file in the commit
git show acf3ba4 -- nix/cargo-pgrx/buildPgrxExtension.nix | grep -A20 -B5 "buildInputs"
# Also check the previous version of the file
git show acb688ad:nix/cargo-pgrx/buildPgrxExtension.nix 2>/dev/null | grep -A5 -B5 "buildInputs"Repository: supabase/postgres
Length of output: 1238
🏁 Script executed:
#!/bin/bash
# Check the commit message and see if there's any explanation for removing Darwin frameworks
git show acf3ba4 | head -50
# Also check if there were any test results or notes about Darwin compatibility
git log --all --grep="Darwin\|darwin\|buildInputs" --oneline | head -20
# Look at the broader diff to understand the context of changes
git show acf3ba4 --statRepository: supabase/postgres
Length of output: 46696
🏁 Script executed:
#!/bin/bash
# Check if there are Darwin-specific tests or CI configurations
rg -n "isDarwin" nix/cargo-pgrx/ -A3 -B3
# Check if there are any test results or comments about Darwin builds
git log --all --oneline | grep -i "darwin\|macos\|apple"
# Look for CI configuration that might test Darwin builds
find .github/workflows -name "*.yml" -type f | xargs grep -l "darwin\|macos" 2>/dev/null | head -5Repository: supabase/postgres
Length of output: 802
Clarify why darwin.apple_sdk.frameworks.Security was removed from buildInputs.
The change removes the Darwin-specific darwin.apple_sdk.frameworks.Security framework that was previously included in buildInputs (line 151), but the commit message doesn't explain this removal. While Darwin-specific RUSTFLAGS handling remains (line 179), the Security framework is typically required for macOS system integrations. Either restore the framework with justification or confirm that Darwin builds continue to work without it.
🤖 Prompt for AI Agents
In @nix/cargo-pgrx/buildPgrxExtension.nix around line 151, The Darwin-specific
Security framework was removed from buildInputs (buildInputs and
darwin.apple_sdk.frameworks.Security) without explanation; either restore it for
macOS builds or add a clear comment/commit note proving it's unnecessary and
that RUSTFLAGS-based handling (RUSTFLAGS / stdenv.isDarwin) still satisfies all
macOS link-time requirements. Fix by making buildInputs conditional on
stdenv.isDarwin to include darwin.apple_sdk.frameworks.Security when building on
Darwin, or if you intentionally removed it, add a comment and update the commit
message explaining why macOS linking remains correct without the framework and
reference the RUSTFLAGS changes that replace it.
| cmakeFlags = [ | ||
| "-DPOSTGRESQL_VERSION=${postgresql.version}" | ||
| "-DCMAKE_POLICY_VERSION_MINIMUM=3.5" | ||
| ] | ||
| ++ lib.optionals (stdenv.isDarwin && lib.versionAtLeast postgresql.version "16") [ | ||
| "-DCMAKE_MACOSX_RPATH=ON" | ||
| "-DCMAKE_SHARED_MODULE_SUFFIX=.dylib" | ||
| "-DCMAKE_SHARED_LIBRARY_SUFFIX=.dylib" | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
CMAKE_POLICY_VERSION_MINIMUM CMake purpose and usage
💡 Result:
Purpose
- CMAKE_POLICY_VERSION_MINIMUM lets an external user or parent project request a minimum CMake "policy version" for a build tree without changing the project’s CMakeLists (i.e., it affects which CMPxxxx policies default to NEW). It was added in CMake 4.0 and is intended for external/parent use, not for a project to set its own policy version. [1]
When to use it
- As a user trying to configure a project that hasn’t declared a recent policy version (e.g., force at least policy version 3.5).
- As a parent project before add_subdirectory() to set the policy version used by a third‑party subproject without editing its files. [1]
How it interacts with cmake_minimum_required / cmake_policy
- cmake_minimum_required(...) implicitly calls cmake_policy(VERSION ...). The effective policy version is the project’s cmake_minimum_required value (or explicit cmake_policy(VERSION)), unless CMAKE_POLICY_VERSION_MINIMUM (if higher) overrides it for the build tree. [2]
How to set it
- On the command line / cache: -DCMAKE_POLICY_VERSION_MINIMUM=3.5
- As an environment variable (to initialize new build trees) or set in the cache before configuring. Use it before add_subdirectory() if you want to affect a nested project. [1]
Notes / best practice
- Projects should use cmake_minimum_required() or cmake_policy(VERSION) themselves; CMAKE_POLICY_VERSION_MINIMUM is intended as an external override (or for parent projects to influence third‑party code). For fine control of individual policies use CMAKE_POLICY_DEFAULT_CMP or explicit cmake_policy(SET ...). [1][2]
Sources
[1] CMake variable CMAKE_POLICY_VERSION_MINIMUM documentation.
[2] cmake_minimum_required / policy behavior documentation.
🏁 Script executed:
# Check the pgrouting default.nix file context
cd nix/ext/pgrouting && cat -n default.nix | head -80Repository: supabase/postgres
Length of output: 3016
🏁 Script executed:
# Look for CMakeLists.txt in the repo that might document CMake requirements
find . -name CMakeLists.txt -type f | head -10Repository: supabase/postgres
Length of output: 43
🏁 Script executed:
# Search for any comments or context around the CMAKE_POLICY flag
cd nix/ext/pgrouting && cat -n default.nix | sed -n '50,75p'Repository: supabase/postgres
Length of output: 1304
🏁 Script executed:
# Check git history for this line to understand why it was added
cd nix/ext/pgrouting && git log --oneline -p default.nix | grep -A5 -B5 CMAKE_POLICY_VERSION_MINIMUM | head -30Repository: supabase/postgres
Length of output: 560
🌐 Web query:
pgrouting CMake minimum required version cmake_minimum_required
💡 Result:
Use the CMake minimum required that matches the pgRouting version:
- Current pgRouting (repo REQUIREMENTS / main) — cmake_minimum_required(VERSION 3.12). [1]
- Older pgRouting 3.0–3.3 series — CMake >= 3.2. [2][3]
- Very old pgRouting 2.0 series — CMake >= 2.8.8. [4]
Sources: pgRouting GitHub REQUIREMENTS (CMake >= 3.12). [1] Installation docs for pgRouting 3.0 / 3.3 (CMake >= 3.2). [2][3] Build guide for pgRouting 2.0 (CMake >= 2.8.8). [4]
Remove or document the CMAKE_POLICY_VERSION_MINIMUM flag.
The flag is set to 3.5, but pgrouting's actual cmake_minimum_required() is 3.2 (for versions 3.0–3.3) or 3.12 (current). While CMAKE_POLICY_VERSION_MINIMUM is the correct mechanism for external policy override in nixpkgs, the arbitrary value of 3.5 lacks documented justification and doesn't align with pgrouting's actual requirements. Either remove it (since pgrouting declares its own minimum), or add a comment explaining why 3.5 is necessary for this build.
🤖 Prompt for AI Agents
In @nix/ext/pgrouting/default.nix around lines 61-69, The cmakeFlags array
includes an unexplained "-DCMAKE_POLICY_VERSION_MINIMUM=3.5" which conflicts
with pgrouting's own cmake_minimum_required (3.2 for older releases or 3.12
currently); either remove that flag from cmakeFlags or replace it with a brief
comment explaining why 3.5 is required for this package build and which
pgrouting versions need it; locate and edit the cmakeFlags definition (the
cmakeFlags variable and the "-DCMAKE_POLICY_VERSION_MINIMUM=3.5" entry) or add
an adjacent comment clarifying the rationale and affected pgrouting versions.
| # plv8 3.1 requires an older version of v8 (we cannot use nodejs.libv8) | ||
| v8 = v8_oldstable; | ||
|
|
||
| # Build function for individual versions | ||
| build = | ||
| version: hash: | ||
| stdenv.mkDerivation (finalAttrs: { | ||
| inherit pname version; | ||
| #version = "3.1.10"; | ||
|
|
||
| v8 = (if (builtins.compareVersions "3.1.10" version >= 0) then v8 else nodejs_20.libv8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check plv8 versions in versions.json
if [ -f "nix/ext/versions.json" ]; then
cat "nix/ext/versions.json" | jq '.plv8 // empty'
else
echo "versions.json not found, searching for it..."
find . -name "versions.json" -type f
fiRepository: supabase/postgres
Length of output: 430
Dead code and inconsistent v8 selection should be cleaned up for maintainability.
All plv8 versions in versions.json (3.1.4, 3.1.5, 3.1.10) are ≤ 3.1.10, so the conditional logic at line 48 always evaluates to v8_oldstable, making it redundant with the outer binding. This doesn't cause a functional bug for current versions, but the inconsistent structure is confusing:
- Line 39: Outer
v8 = v8_oldstable - Line 48: Unused derivation attribute
v8with conditional (always equalsv8_oldstablefor all current versions) - Line 76: Duplicate conditional in
buildInputs(also alwaysv8_oldstable) - All other references use outer
v8
Simplify by removing the unused attribute and inline conditional since no versions > 3.1.10 are currently configured. If future versions may require version-specific v8 selection, refactor to use a consistent approach throughout rather than splitting the logic across multiple locations.
🤖 Prompt for AI Agents
In @nix/ext/plv8/default.nix around lines 38-48, The inner per-derivation v8
attribute and duplicate conditional in buildInputs are dead/redundant because
all versions <= 3.1.10, so remove the conditional v8 attribute inside the build
(the v8 = (if (builtins.compareVersions "3.1.10" version >= 0) then v8 else
nodejs_20.libv8) line and any duplicate conditional in buildInputs) and rely on
the outer v8 = v8_oldstable binding; if you want future-proofing instead of
removing logic, centralize version-specific selection into a single helper
(e.g., selectV8(version)) used by the build function rather than duplicating
conditionals in stdenv.mkDerivation and buildInputs.
| assert False, ( | ||
| "PostgREST logs contain 'session is not read-only' errors even though PostgreSQL is configured for read-only mode" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace assert False with raise AssertionError().
Using assert False is problematic because Python's -O optimization flag removes all assert statements, which would silently skip this error condition in production or optimized environments.
🔎 Recommended fix
- assert False, (
- "PostgREST logs contain 'session is not read-only' errors even though PostgreSQL is configured for read-only mode"
- )
+ raise AssertionError(
+ "PostgREST logs contain 'session is not read-only' errors even though PostgreSQL is configured for read-only mode"
+ )🧰 Tools
🪛 Ruff (0.14.10)
790-790: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🤖 Prompt for AI Agents
In @testinfra/test_ami_nix.py around lines 790-792, The test currently uses
"assert False, (\"PostgREST logs contain 'session is not read-only' errors even
though PostgreSQL is configured for read-only mode\")" which can be stripped out
with Python's -O optimization; replace that assert with an explicit exception by
raising AssertionError with the same message (i.e., use raise
AssertionError("PostgREST logs contain 'session is not read-only' errors even
though PostgreSQL is configured for read-only mode")) so the failure cannot be
bypassed by optimizations and still provides the same diagnostic text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @nix/cargo-pgrx/default.nix:
- Line 36: The build config is inconsistent: openssl is only added to
buildInputs/nativeBuildInputs via lib.optionals stdenv.hostPlatform.isLinux, but
the OPENSSL_* environment variables are set unconditionally; update the file so
the OPENSSL_* environment variables are wrapped with the same
stdenv.hostPlatform.isLinux guard (or alternatively make openssl unconditional
in buildInputs/nativeBuildInputs) to ensure Darwin builds don’t reference
missing openssl paths; locate the OPENSSL_* settings near the
buildInputs/nativeBuildInputs change and apply the same conditional (or remove
the conditional from buildInputs), keeping compatibility with the existing
buildPgrxExtension.nix Darwin support.
In @nix/ext/pgroonga/groonga.nix:
- Line 48: The buildInputs entry is using lib.optional with a list which creates
a nested list when zlibSupport is true; replace the lib.optional call with
lib.optionals (plural) so the zlib entry is flattened like the other
conditionals (match usage on surrounding lines), e.g. change the lib.optional
zlibSupport [ zlib ] expression to use lib.optionals so buildInputs remains a
flat list.
In @nix/ext/tests/lib.py:
- Around line 169-171: The assertion error messages reference
self.extension_name but the checks validate filenames built from self.lib_name;
update the messages to mention self.lib_name for clarity. Locate the three
assertions that test ext_version.endswith(f"{self.lib_name}-{last_version}.so")
(and the similar checks at the other two places) and change the f-string in
their exception text to reference self.lib_name (e.g., "Expected {self.lib_name}
version {last_version}...") so the logged name matches the filename being
validated.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nix/ext/pgroonga/groonga.nix (1)
1-106: This file should have been removed.Per the previous review comment, this file was moved to
nix/ext/pgroonga/default.nixand should no longer exist. The presence of modifications here suggests a rebase issue. Please verify:
- Whether this file should be deleted from the PR
- Whether the buildInputs changes (lines 39-52) need to be applied to
nix/ext/pgroonga/default.nixinsteadBased on past review comments indicating this file should be removed.
nix/ext/tests/orioledb.nix (1)
14-14: Consider usingmajorVersionvariable instead of hardcoding "17".Lines 14 and 18 hardcode
"psql_orioledb-17"in the extension path, but line 8 already extractsmajorVersionfrompostgresql.version. This creates a mismatch where thepostgresqlWithExtensionfunction appears generic but only works correctly with PostgreSQL version 17.If orioledb supports multiple PostgreSQL major versions, consider using the
majorVersionvariable to make the function more maintainable and version-agnostic. This would also eliminate the code duplication between lines 14 and 18.🔎 Suggested refactor
+ extensionPkg = self.legacyPackages.${pkgs.system}."psql_orioledb-${majorVersion}".exts.orioledb; pkg = pkgs.buildEnv { name = "postgresql-${majorVersion}-${pname}"; paths = [ postgresql postgresql.lib - (self.legacyPackages.${pkgs.system}."psql_orioledb-17".exts.orioledb) + extensionPkg ]; passthru = { inherit (postgresql) version psqlSchema; - installedExtensions = [ (self.legacyPackages.${pkgs.system}."psql_orioledb-17".exts.orioledb) ]; + installedExtensions = [ extensionPkg ];Also applies to: 18-18
♻️ Duplicate comments (4)
testinfra/test_ami_nix.py (1)
790-792: Replaceassert Falsewithraise AssertionError().This issue was flagged in a previous review but remains unresolved. Using
assert Falseis problematic because Python's-Ooptimization flag removes all assert statements, which would silently skip this error condition.🔎 Recommended fix
- assert False, ( - "PostgREST logs contain 'session is not read-only' errors even though PostgreSQL is configured for read-only mode" - ) + raise AssertionError( + "PostgREST logs contain 'session is not read-only' errors even though PostgreSQL is configured for read-only mode" + )nix/ext/pg_jsonschema/default.nix (1)
83-93: Verify version "0.3.3" should be excluded from previousVersions.The doCheck conditional is well-documented and addresses the sandbox limitation. However, version "0.3.3" appears in the doCheck disabled list but is absent from the previousVersions array (lines 48-57). Versions "0.2.0" and "0.3.1" appear in both lists.
If "0.3.3" is not the latest version in
../versions.json, it should be added to previousVersions to ensure upgrade scripts are generated correctly.#!/bin/bash # Verify if 0.3.3 should be in previousVersions by checking versions.json echo "Checking pg_jsonschema versions defined in versions.json:" jq -r '.pg_jsonschema | keys[]' nix/ext/versions.json | sort -V echo -e "\nVersions with tests disabled:" echo "0.2.0, 0.3.1, 0.3.3" echo -e "\nVersions in previousVersions array:" echo "0.3.1, 0.3.0, 0.2.0, 0.1.4, 0.1.3, 0.1.2, 0.1.1, 0.1.0" echo -e "\nIf 0.3.3 is NOT the latest version, it should be added to previousVersions."nix/ext/plv8/default.nix (1)
38-48: The dead code and inconsistent v8 selection flagged in the previous review remains unaddressed.As noted in the prior review, line 48 defines a per-derivation
v8attribute that is never referenced (all usage points to the outerv8from line 39), and line 76 duplicates the conditional logic in buildInputs while the rest of the code uses the outer binding. This creates maintenance confusion and potential bugs if versions > 3.1.10 are added.nix/ext/pgrouting/default.nix (1)
61-69: CMAKE_POLICY_VERSION_MINIMUM still lacks justification.As flagged in a previous review, the
-DCMAKE_POLICY_VERSION_MINIMUM=3.5flag remains undocumented. Either remove it (pgrouting declares its own minimum) or add a comment explaining why 3.5 is required for this build.
🧹 Nitpick comments (4)
nix/ext/plv8/default.nix (1)
136-158: Simplify installPhase by removing redundant conditional.The if/else branching on PostgreSQL version 15 executes identical
install_name_toolandpatchelfcommands in both branches. Since the logic is the same regardless of PostgreSQL version, the conditional can be removed.🔎 Proposed simplification
- if [ "${lib.versions.major postgresql.version}" = "15" ]; then - ${lib.optionalString stdenv.isDarwin '' - install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME - install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME - install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME - install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME - ''} - - ${lib.optionalString (!stdenv.isDarwin) '' - ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME - ''} - else - ${lib.optionalString stdenv.isDarwin '' - install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME - install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME - install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME - install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME - ''} - - ${lib.optionalString (!stdenv.isDarwin) '' - ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME - ''} - fi + ${lib.optionalString stdenv.isDarwin '' + install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME + install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME + install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME + install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME + ''} + + ${lib.optionalString (!stdenv.isDarwin) '' + ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME + ''}nix/ext/pg_net.nix (1)
13-15: Consider removing the curl alias indirection.The
curl = curl_8_6alias adds an extra level of indirection without clear benefit. Consider usingcurl_8_6directly inbuildInputsfor simplicity.🔎 Proposed simplification
Remove the alias and use
curl_8_6directly:-let - curl = curl_8_6; -in let pname = "pg_net"; build =Then update line 24 to reference
curl_8_6directly:buildInputs = [ - curl + curl_8_6 postgresql ]flake.nix (1)
22-24: Consider enhancing the nixpkgs-oldstable comment.The comment explains why the pin exists, but per a previous review suggestion, consider adding the date or nixpkgs version for this commit, and guidance on when/whether to update it (e.g., "update only if needed for specific extension compatibility").
nix/postgresql/generic.nix (1)
345-345: Clarify the empty teams list.The
teams = [ ]field has been added to metadata. If this is a placeholder for future team assignment, consider adding a comment. Otherwise, if no teams are responsible, this can remain empty.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
ansible/vars.ymlflake.nixmigrations/schema-15.sqlmigrations/schema-17.sqlmigrations/schema-orioledb-17.sqlnix/cargo-pgrx/buildPgrxExtension.nixnix/cargo-pgrx/default.nixnix/cargo-pgrx/versions.jsonnix/checks.nixnix/ext/gdal.nixnix/ext/pg_jsonschema/default.nixnix/ext/pg_net.nixnix/ext/pgaudit.nixnix/ext/pgroonga/default.nixnix/ext/pgroonga/groonga.nixnix/ext/pgrouting/default.nixnix/ext/pgrouting/pgrouting-3.4.1-pg17.patchnix/ext/plpgsql-check.nixnix/ext/plv8/default.nixnix/ext/postgis.nixnix/ext/tests/default.nixnix/ext/tests/http.nixnix/ext/tests/lib.pynix/ext/tests/orioledb.nixnix/ext/tests/pg_plan_filter.nixnix/ext/tests/pg_repack.nixnix/ext/tests/pg_safeupdate.nixnix/ext/tests/pgjwt.nixnix/ext/tests/pgmq.nixnix/ext/tests/pgroonga.nixnix/ext/tests/pgrouting.nixnix/ext/tests/pgsodium.nixnix/ext/tests/plpgsql_check.nixnix/ext/tests/plv8.nixnix/ext/tests/postgis.nixnix/ext/tests/timescaledb.nixnix/ext/tests/vault.nixnix/ext/timescaledb.nixnix/ext/vault.nixnix/ext/wal2json.nixnix/ext/wrappers/default.nixnix/nixpkgs.nixnix/overlays/default.nixnix/packages/default.nixnix/packages/groonga/do-not-use-vendored-libraries.patchnix/packages/groonga/fix-cmake-install-path.patchnix/packages/lib.nixnix/packages/packer.nixnix/packages/postgres.nixnix/packages/sfcgal.nixnix/packages/supascan.nixnix/packages/sync-exts-versions.nixnix/postgresql/generic.nixtestinfra/test_ami_nix.py
💤 Files with no reviewable changes (4)
- nix/packages/sfcgal.nix
- nix/overlays/default.nix
- nix/packages/groonga/fix-cmake-install-path.patch
- nix/packages/groonga/do-not-use-vendored-libraries.patch
🚧 Files skipped from review as they are similar to previous changes (28)
- nix/ext/pgroonga/default.nix
- migrations/schema-orioledb-17.sql
- nix/ext/tests/default.nix
- nix/ext/tests/pgsodium.nix
- nix/cargo-pgrx/buildPgrxExtension.nix
- nix/packages/packer.nix
- ansible/vars.yml
- nix/ext/tests/http.nix
- nix/ext/wrappers/default.nix
- nix/nixpkgs.nix
- migrations/schema-17.sql
- nix/ext/tests/plv8.nix
- nix/ext/tests/pg_repack.nix
- nix/ext/tests/plpgsql_check.nix
- nix/ext/tests/timescaledb.nix
- nix/ext/tests/postgis.nix
- nix/packages/default.nix
- nix/ext/vault.nix
- nix/ext/tests/pgjwt.nix
- nix/ext/tests/pgmq.nix
- nix/ext/tests/pgroonga.nix
- nix/ext/pgaudit.nix
- nix/ext/plpgsql-check.nix
- nix/ext/tests/pg_plan_filter.nix
- nix/ext/tests/vault.nix
- nix/ext/tests/pgrouting.nix
- nix/packages/lib.nix
- nix/ext/gdal.nix
🧰 Additional context used
🪛 Ruff (0.14.10)
testinfra/test_ami_nix.py
790-790: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
⏰ 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 (42)
migrations/schema-15.sql (1)
640-640: LGTM—change aligns with updated dbmate output.The removal of the explicit length constraint (varchar(128) → varchar) is safe and non-breaking. As noted in the previous discussion, this change results from updating dbmate from 2.14.0 to 2.28.0, and the team has already decided to accept the newer tool's schema output format.
testinfra/test_ami_nix.py (1)
435-437: LGTM: Formatting changes to assert statements.The reformatting of these multi-line assert statements to single-line format is acceptable. These are all conditional asserts with proper error messages.
Also applies to: 560-562, 592-594, 627-629
nix/checks.nix (1)
8-8: LGTM: Parameter reordering.Reordering the
libparameter to appear afterpkgsin the perSystem function signature has no functional impact. This is a style adjustment that may align with project conventions.nix/ext/tests/pg_safeupdate.nix (1)
22-26: LGTM! Consistent interface standardization.The new passthru attributes (
installedExtensions,withJIT,withoutJIT) provide a uniform interface for extension test modules, matching the pattern applied across other test files in this PR.Note: Both
withJITandwithoutJITcurrently point to the samepkg, which is acceptable if pg_safeupdate doesn't require JIT-specific build variants. This provides interface consistency even when functional variants aren't needed.nix/ext/tests/orioledb.nix (1)
21-22: Verify thatwithJITandwithoutJITpointing to the same package is intentional.Both
withJITandwithoutJITattributes reference the samepkgvariable. This appears to mirror the pattern from other extension tests according to the AI summary, but it's worth confirming this is the intended behavior for the test environment rather than an oversight.nix/ext/tests/lib.py (3)
8-8: LGTM!The addition of
Optionalto the imports is necessary for the newlib_nameparameter type hint.
24-24: LGTM!The new
lib_nameparameter is well-designed with a sensible fallback toextension_name. The implementation correctly handles the case wherelib_nameis not provided.Note: The fallback
self.lib_name = lib_name or extension_namewill useextension_nameiflib_nameisNone, empty string, or any other falsy value. This is likely the intended behavior.Also applies to: 34-34, 42-42
99-101: LGTM!The assertion reformatting maintains the same logic while being more compact.
nix/ext/pg_jsonschema/default.nix (2)
45-45: LGTM! Darwin removal aligns with PR cleanup.The removal of
darwinfrom buildInputs is appropriate—Darwin-specific configuration is already handled through theenvattribute (lines 60-69). This aligns with the broader nixpkgs update that removes Darwin-specific platform augmentation from buildInputs across multiple derivations.
48-57: LGTM! previousVersions expansion is well-documented.The expanded previousVersions list is properly ordered and will correctly generate extension update scripts through the preBuild logic (lines 95-98).
nix/packages/supascan.nix (2)
41-43: Good approach for cross-platform compatibility.Disabling CGO is a reasonable trade-off to eliminate Darwin framework dependencies while maintaining cross-platform builds. For a system scanner tool, this should work well.
One consideration: with
CGO_ENABLED=0, Go uses its pure-Go DNS resolver instead of the system resolver. This typically works fine but may behave slightly differently in edge cases (e.g., certain nsswitch.conf configurations on Linux). Given supascan's use case as a validation tool, this shouldn't be an issue.
1-5: LGTM! Clean simplification of Go 1.24 sourcing.The removal of the
inputsparameter and sourcinggo_1_24directly frompkgssimplifies the derivation by eliminating the dependency on a separate flake input. The updated nixpkgs (nixos-unstable) providesgo_1_24, and thebuildGoModule124override is correctly applied to the supascan package at line 31.nix/ext/pg_net.nix (2)
10-10: Good alignment with past feedback.The addition of
curl_8_6as a dedicated input parameter addresses the previous review comment about declaring a specific curl version in the overlay rather than passing the full attributeset.
52-59: No actionable issues found. The version check using exact equality (version == "0.19.5") is correct: 0.19.5 is the only supported version ≥ 0.19.1 in the codebase. The curl_8_6 dependency is applied uniformly across all versions with no obvious incompatibilities evident in the code.nix/cargo-pgrx/versions.json (1)
1-119: LGTM - Hash updates verified by successful e2e testing.All cargoHash updates appear correct and have been validated by the comprehensive e2e testing reported in PR comments (engines tests passed 3/3 across PG versions 15, 17, and 17-oriole). The hash updates are mechanical and result from the nixpkgs upgrade and related toolchain changes.
nix/cargo-pgrx/default.nix (1)
73-83: LGTM - cargoHash values are consistent with versions.json.The updated cargoHash values for cargo-pgrx versions 0.12.6, 0.12.9, and 0.14.3 match the corresponding entries in
nix/cargo-pgrx/versions.jsonand have been validated by successful e2e testing.nix/packages/sync-exts-versions.nix (1)
14-15: Version bump to Nix 2.29 is safe.The script uses
nix-prefetch-urlfor hash extraction (viacut),nix hash to-srifor conversion, andnix evalwithout JSON flags. The identified breaking change (JSON prettification in Nix 2.29) does not apply here since the script avoids--jsonflags and parses only yq output with jq. The fundamental nix-prefetch-url output format remains stable across the versions.nix/ext/wal2json.nix (1)
13-13: LGTM: Unused parameter clearly marked.The underscore prefix correctly signals that the
_revparameter is unused (line 20 constructs the revision fromversioninstead). This improves code clarity.nix/ext/pgrouting/default.nix (2)
52-54: LGTM: Patch applied conditionally.The patch is correctly applied only to pgrouting 3.4.1 running on PostgreSQL 17+, matching the scope of the compatibility fix.
15-15: Path change to versions.json is correct.The file exists at
nix/ext/versions.jsonand contains the pgrouting version data as expected.flake.nix (2)
25-25: LGTM: Using stable channel tarball URL.The nixpkgs URL now points to the nixos-unstable channel tarball, which provides stable snapshot URLs appropriate for flake inputs.
10-21: Follow declarations correctly prevent duplicate dependencies.The new inputs properly declare
followsto avoid duplicate nixpkgs instances: git-hooks, nix-editor, nix-eval-jobs, and nix2container all follow the primary nixpkgs. Additionally, nix-eval-jobs follows treefmt-nix and flake-parts, ensuring consistent input versions throughout the flake without duplication.nix/packages/postgres.nix (1)
28-28: LGTM: Import path updated for directory structure.The pgrouting import changed from a file reference (
../ext/pgrouting.nix) to a directory reference (../ext/pgrouting), aligning with the restructuring of pgrouting as a directory withdefault.nix. This is consistent with the path changes observed innix/ext/pgrouting/default.nix.nix/ext/pgrouting/pgrouting-3.4.1-pg17.patch (1)
1-39: Patch correctly addresses PostgreSQL 17 SPI_getvalue API change.The patch accurately reflects that
SPI_getvaluereturnschar*directly in PostgreSQL 17, notDatum. Removing theDatumGetCStringwrapper is correct—the function expects aDatumbut receives achar*, causing a type mismatch. The changes in bothget_check_data.candtrsp.cfix this compilation issue.nix/ext/postgis.nix (2)
82-83: Conditional Darwin dependency is appropriate.The conditional inclusion of
libiconvfor Darwin and the formatting adjustment are correct.
19-19: This concern is not applicable. Thesfcgalparameter is automatically resolved by thecallPackageWithmechanism in Nix. SinceextCallPackageis defined withpkgs(which includes nixpkgs'sfcgalpackage), all calls topostgis.nixwill receive thesfcgalparameter automatically without requiring explicit parameter passing at each call site. The migration from a localsfcgal.nixto using nixpkgs'sfcgalis handled correctly.nix/ext/timescaledb.nix (4)
42-43: LGTM: Formatting adjustment.The CMake flags formatting change improves readability without altering behavior.
129-130: Document the loader symlink purpose.The comment on line 129 indicates this symlink is "used by background worker tests," which is helpful context.
150-150: LGTM: Consistent libName exposure.The
libNamepassthru field correctly exposes the loader library name for external use.
111-111: Symlink consistency verified.The loader symlink created at line 111 (
$EXT_NAME-loader${postgresql.dlSuffix}) is correctly aligned with thelibName = "timescaledb-loader"passthru (line 150) and the postBuild symlink (line 130, which includes a comment confirming it's for background worker tests). Tests innix/ext/tests/timescaledb.nixconfirm that both the test harness (line 91) and background worker checks (line 97) properly reference thetimescaledb-loaderlibrary.nix/postgresql/generic.nix (12)
15-15: LGTM: ICU version pinning is well-documented.Pinning ICU to version 75 to maintain collation version 153.120 is appropriate for preventing collation mismatch warnings during nixpkgs upgrades. The comment clearly explains the rationale.
Also applies to: 123-125
109-110: LGTM: Hardening flag comment update.The comment correctly notes that PIE is now enabled by default in compilers, and the
hardeningEnableline is appropriately commented out rather than removed.
118-144: LGTM: buildInputs restructuring.The buildInputs have been reformatted for clarity with proper conditional logic for:
- Legacy libxcrypt support (< v13)
- JIT support with LLVM
- LZ4 (v14+) and ZSTD (v15+) compression
- Platform-specific dependencies
The logic is correct and well-organized.
146-154: LGTM: nativeBuildInputs restructuring.The conditional inclusion of JIT-specific native build inputs (LLVM dev, nukeReferences, patchelf) is properly structured.
167-184: LGTM: configureFlags restructuring.The configure flags have been reformatted consistently with proper conditional logic. All necessary flags are present.
185-207: LGTM: Patches restructuring with proper musl handling.The patches section correctly:
- Applies version-specific patches for v16+ vs earlier
- Uses
replaceVarsfor locale binary path substitution- Handles musl patches using
fetchurl(per comment about nixpkgs issue #240141)- Applies Linux-specific socket directory patches
211-220: LGTM: postPatch with JIT support.The postPatch logic correctly substitutes paths for JIT components when JIT support is enabled.
222-269: LGTM: Comprehensive postInstall with JIT handling.The extensive postInstall section properly:
- Moves pgxs and libraries to appropriate outputs
- Removes static libraries when dynamic versions exist
- Handles JIT-specific bitcode and library relocation
- Removes retained dependencies on clang-wrapper and LLVM dev outputs
- Uses
nuke-refsto eliminate unnecessary references- Conditionally patches rpath on non-Darwin platforms
This is thorough cleanup necessary for minimal closure sizes.
323-337: LGTM: Tests section restructuring.The tests are properly structured with:
- Standard tests (wal-receiver, pkg-config)
- Conditional JIT-specific test
24-24:replaceVarsusage for locale patch is correct.The
replaceVarsfunction is properly imported (line 24) and correctly applied to substitute the locale binary path in the patch. The template variable@locale@in the patch matches thelocalekey provided toreplaceVars(lines 197-199), with the substitution correctly resolving todarwin.adv_cmdson macOS and the standard C library on Linux.
69-69: Pattern match logic is correct and safe.The
[0-9][0-9]_.*pattern correctly identifies OrioleDB versions. OrioleDB versions are versioned as two-digit major version followed by underscore and patchset number (e.g.,17_15), while standard PostgreSQL uses semantic versioning with dots (e.g.,17.6). Standard PostgreSQL versions cannot match this pattern and no false positives are possible.
159-159: No action required —__structuredAttrsis correctly implemented.The code already follows best practices for
__structuredAttrs = true:
- Environment variables are correctly exported via
env.NIX_CFLAGS_COMPILE(line 165), which is the documented pattern when using structured attributes withmkDerivation- PostgreSQL's standard autoconf/make build system is fully compatible with structured attributes
- nixpkgs-unstable has supported this feature since Nix 2.16+
- Extensions and downstream consumers are unaffected—structured attributes are transparent to users of the derivation's outputs and passthru attributes
Likely an incorrect or invalid review comment.
| buildInputs = | ||
| lib.optionals stdenv.hostPlatform.isLinux [ openssl ] | ||
| ++ lib.optionals stdenv.hostPlatform.isDarwin [ darwin.apple_sdk.frameworks.Security ]; | ||
| buildInputs = lib.optionals stdenv.hostPlatform.isLinux [ openssl ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, check for Darwin-specific configurations in cargo-pgrx files
rg -n "darwin|Darwin|hostPlatform.isDarwin" nix/cargo-pgrx/ -iRepository: supabase/postgres
Length of output: 250
🏁 Script executed:
# Also check the full default.nix to understand the buildInputs context
cat -n nix/cargo-pgrx/default.nix | head -60Repository: supabase/postgres
Length of output: 2448
🏁 Script executed:
# Look for git history to see what was removed (if available)
git log --oneline -10 nix/cargo-pgrx/default.nix 2>/dev/null || echo "Git history not available"Repository: supabase/postgres
Length of output: 129
🏁 Script executed:
# Check if there are other buildInputs configurations in the broader nix directory
rg -n "buildInputs.*lib\.optionals" nix/ -A 2 -B 2Repository: supabase/postgres
Length of output: 433
🏁 Script executed:
# Check how OPENSSL variables are used - see if they're critical for the build
rg -n "OPENSSL_DIR|OPENSSL_INCLUDE|OPENSSL_LIB" nix/cargo-pgrx/ -A 2 -B 2Repository: supabase/postgres
Length of output: 560
🏁 Script executed:
# Check if there's a separate Darwin configuration or if this is already broken
rg -n "openssl" nix/cargo-pgrx/default.nix -B 5 -A 5Repository: supabase/postgres
Length of output: 885
Address inconsistent Darwin build configuration: conditional buildInputs with unconditional OPENSSL environment variables.
The buildInputs and nativeBuildInputs are now Linux-only (via lib.optionals stdenv.hostPlatform.isLinux), but the OPENSSL_* environment variables on lines 38-41 are set unconditionally. This inconsistency can cause Darwin builds to fail since buildInputs will be empty while still trying to reference openssl paths. Darwin support is still intended elsewhere in the codebase (buildPgrxExtension.nix), so either the OPENSSL environment variables should also be conditional, or openssl should remain in buildInputs for all platforms.
🤖 Prompt for AI Agents
In @nix/cargo-pgrx/default.nix around line 36, The build config is inconsistent:
openssl is only added to buildInputs/nativeBuildInputs via lib.optionals
stdenv.hostPlatform.isLinux, but the OPENSSL_* environment variables are set
unconditionally; update the file so the OPENSSL_* environment variables are
wrapped with the same stdenv.hostPlatform.isLinux guard (or alternatively make
openssl unconditional in buildInputs/nativeBuildInputs) to ensure Darwin builds
don’t reference missing openssl paths; locate the OPENSSL_* settings near the
buildInputs/nativeBuildInputs change and apply the same conditional (or remove
the conditional from buildInputs), keeping compatibility with the existing
buildPgrxExtension.nix Darwin support.
| msgpack-c | ||
| ] | ||
| ++ lib.optionals lz4Support [ lz4 ] | ||
| ++ lib.optional zlibSupport [ zlib ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: lib.optional creates nested list.
Using lib.optional with a list argument will create a nested list [[ zlib ]] when zlibSupport is true, breaking the buildInputs array. Use lib.optionals (plural) for consistency with lines 47 and 49.
🔎 Proposed fix
- ++ lib.optional zlibSupport [ zlib ]
+ ++ lib.optionals zlibSupport [ zlib ]📝 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.
| ++ lib.optional zlibSupport [ zlib ] | |
| + lib.optionals zlibSupport [ zlib ] |
🤖 Prompt for AI Agents
In @nix/ext/pgroonga/groonga.nix around line 48, The buildInputs entry is using
lib.optional with a list which creates a nested list when zlibSupport is true;
replace the lib.optional call with lib.optionals (plural) so the zlib entry is
flattened like the other conditionals (match usage on surrounding lines), e.g.
change the lib.optional zlibSupport [ zlib ] expression to use lib.optionals so
buildInputs remains a flat list.
| assert ext_version.endswith(f"{self.lib_name}-{last_version}.so"), ( | ||
| f"Expected {self.extension_name} version {last_version}, but found {ext_version}" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider updating error messages to reference lib_name for clarity.
The assertions now check for files named {lib_name}-{version}.so, but the error messages reference self.extension_name. When lib_name differs from extension_name, this could make debugging confusing.
For example, if extension_name="pg_cron" and lib_name="pgcron", the error message will say "Expected pg_cron version 1.0" but the actual file being checked is pgcron-1.0.so.
🔎 Suggested fix for clearer error messages
- assert ext_version.endswith(f"{self.lib_name}-{last_version}.so"), (
- f"Expected {self.extension_name} version {last_version}, but found {ext_version}"
- )
+ assert ext_version.endswith(f"{self.lib_name}-{last_version}.so"), (
+ f"Expected {self.lib_name}-{last_version}.so, but found {ext_version}"
+ )Apply similar changes to all three assertions (lines 169-171, 179-181, 187-189).
Also applies to: 179-181, 187-189
🤖 Prompt for AI Agents
In @nix/ext/tests/lib.py around lines 169-171, The assertion error messages
reference self.extension_name but the checks validate filenames built from
self.lib_name; update the messages to mention self.lib_name for clarity. Locate
the three assertions that test
ext_version.endswith(f"{self.lib_name}-{last_version}.so") (and the similar
checks at the other two places) and change the f-string in their exception text
to reference self.lib_name (e.g., "Expected {self.lib_name} version
{last_version}...") so the logged name matches the filename being validated.
nix 2.29 doesn't exist in the updated nixpkgs anymore
Replace deprecated system attribute accesses throughout the codebase with stdenv.hostPlatform.system to resolve Nix deprecation warnings. Changes: - Replace inherit (stdenv) system with system = stdenv.hostPlatform.system - Replace pkgs.system with pkgs.stdenv.hostPlatform.system (55 occurrences) - Replace system function parameters with stdenv to avoid auto-injection - Replace final.system in overlay with final.stdenv.hostPlatform.system Affected files: - nix/postgresql/generic.nix - nix/packages/default.nix - nix/ext/tests/*.nix (16 files) - nix/packages/lib.nix - nix/packages/dbmate-tool.nix - nix/packages/show-commands.nix - nix/cargo-pgrx/mkPgrxExtension.nix - nix/overlays/default.nix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @nix/postgresql/generic.nix:
- Around line 197-199: The patch uses bare stdenv in the replaceVars call;
change both occurrences to the JIT-aware stdenv' used elsewhere: replace
stdenv.isDarwin with stdenv'.isDarwin and stdenv.cc.libc with stdenv'.cc.libc
inside the replaceVars invocation (the expression building locale in the
locale-binary-path.patch replacement) so the platform checks and libc reference
are consistent with the rest of the file.
- Line 15: The pinned dependency icu75 in the Nix expression is flagged for
CVE-2025-5222; locate the icu75 symbol in this file and verify whether the Nix
package includes the upstream security fix or local backport; if it does not,
update the pin to a patched upstream version (e.g., a fixed minor release) or
apply the vendor security patch as a patch file in the derivation (add the patch
and include it in the src/patches list), then rebuild and run tests to confirm
collations still match and the CVE is mitigated.
🧹 Nitpick comments (2)
nix/ext/tests/pgrouting.nix (1)
49-49: LGTM: All PostgreSQL package references correctly migrated.All references to PostgreSQL packages throughout the test configuration (pg_regress, services.postgresql.package, specialisation configurations, and migration scripts) now correctly use
self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_*pattern. The migration is thorough and consistent.Optional: Consider extracting the platform system pattern
The pattern
self.packages.${pkgs.stdenv.hostPlatform.system}is repeated many times. For improved readability, you could optionally extract it into a let-binding:let platformPackages = self.packages.${pkgs.stdenv.hostPlatform.system}; in # Then use: platformPackages.postgresql_15, platformPackages.postgresql_17, etc.This is purely optional and the current implementation is acceptable for a test file.
Also applies to: 76-76, 81-83, 97-102, 126-128, 156-158
nix/postgresql/generic.nix (1)
345-345: Consider removing the empty teams list if not required.The empty
teams = [ ]attribute doesn't provide value. If this isn't required by nixpkgs conventions, consider removing it to reduce clutter.🔎 Proposed change
- teams = [ ];
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
nix/cargo-pgrx/mkPgrxExtension.nixnix/ext/tests/default.nixnix/ext/tests/http.nixnix/ext/tests/orioledb.nixnix/ext/tests/pg_plan_filter.nixnix/ext/tests/pg_repack.nixnix/ext/tests/pg_safeupdate.nixnix/ext/tests/pgjwt.nixnix/ext/tests/pgmq.nixnix/ext/tests/pgroonga.nixnix/ext/tests/pgrouting.nixnix/ext/tests/pgsodium.nixnix/ext/tests/plpgsql_check.nixnix/ext/tests/plv8.nixnix/ext/tests/postgis.nixnix/ext/tests/timescaledb.nixnix/ext/tests/vault.nixnix/overlays/default.nixnix/packages/dbmate-tool.nixnix/packages/default.nixnix/packages/lib.nixnix/packages/show-commands.nixnix/postgresql/generic.nix
🚧 Files skipped from review as they are similar to previous changes (5)
- nix/ext/tests/timescaledb.nix
- nix/ext/tests/pg_plan_filter.nix
- nix/packages/lib.nix
- nix/packages/default.nix
- nix/overlays/default.nix
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-06T01:52:31.331Z
Learnt from: soedirgo
Repo: supabase/postgres PR: 1987
File: migrations/db/migrations/20251215120934_supabase_superuser.sql:10-10
Timestamp: 2026-01-06T01:52:31.331Z
Learning: For the supabase/postgres repository, do not recommend adding down migrations or flag empty down migrations as issues.
Applied to files:
nix/postgresql/generic.nix
⏰ 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). (3)
- GitHub Check: no packages to build (aarch64-linux)
- GitHub Check: no packages to build (x86_64-linux)
- GitHub Check: no packages to build (aarch64-darwin)
🔇 Additional comments (33)
nix/packages/dbmate-tool.nix (1)
3-3: LGTM! Migration tostdenv.hostPlatform.systemis correct.The addition of
stdenvto the function parameters and the update to usestdenv.hostPlatform.systemfor theCURRENT_SYSTEMsubstitution properly migrates away from the deprecatedsystemattribute.Also applies to: 31-31
nix/packages/show-commands.nix (1)
5-5: LGTM! Platform reference correctly updated.The migration from the
systemparameter tostdenvand the corresponding update to access apps viastdenv.hostPlatform.systemproperly aligns with Nix best practices for platform handling.Also applies to: 17-17
nix/ext/tests/pgroonga.nix (5)
7-9: LGTM: Platform system migration.The migration from deprecated
pkgs.systemtopkgs.stdenv.hostPlatform.systemaligns with Nix best practices and is consistent with the PR's objective to remove deprecated system attribute usages.
24-24: LGTM: Useful API addition.The
installedExtensionspassthru attribute improves the package API by allowing consumers to programmatically query which extensions are bundled.
44-45: LGTM: Platform system migration.The migration to
pkgs.stdenv.hostPlatform.systemfor accessing PostgreSQL packages is correct and consistent with the deprecated system attribute removal across the codebase.
86-86: LGTM: Platform system migration.The environment path references correctly use
pkgs.stdenv.hostPlatform.system, ensuring proper package resolution for the PostgreSQL service environment.Also applies to: 90-90
27-28: No action needed. BothwithJITandwithoutJITcorrectly point to the samepkgin test files. This is intentional design: in the production PostgreSQL package (nix/postgresql/generic.nix), these attributes provide different builds (one with JIT, one without). However, test files are mock environments that only need to satisfy the passthru API expected by extensions—they don't require actual JIT variants. This pattern is consistent across all 16 test files innix/ext/tests/, confirming it's deliberate test scaffolding.nix/ext/tests/pgmq.nix (3)
7-9: LGTM: Correct migration to hostPlatform.system.The replacement of
pkgs.systemwithpkgs.stdenv.hostPlatform.systemcorrectly addresses the deprecation of thesystemattribute and aligns with Nix best practices for cross-platform builds.
44-45: LGTM: Package selections correctly use hostPlatform.system.The psql_15 and psql_17 package selections now reference
pkgs.stdenv.hostPlatform.system, ensuring consistent platform-specific resolution across the test suite.
24-24: This aliasing is intentional for test environments.Both
withJITandwithoutJITare aliased to the same package throughout all extension test files. Unlike the canonical PostgreSQL derivation innix/postgresql/generic.nixwhere these attributes conditionally select different builds based on JIT support, test environments provide a unified wrapper that doesn't require separate JIT variants. The tests focus on extension compatibility, not JIT functionality.nix/ext/tests/postgis.nix (1)
7-9: LGTM: Consistent platform and passthru updates.The migration to
pkgs.stdenv.hostPlatform.systemand the addition ofinstalledExtensions,withJIT, andwithoutJITpassthru attributes are consistent with the PR's objectives and match the pattern across other test files.Note: As flagged in pgmq.nix, verify whether
withJITandwithoutJITaliasing the same package is intentional for the test environment.Also applies to: 24-24, 27-28, 66-66, 71-73, 87-92
nix/ext/tests/http.nix (1)
7-9: LGTM: Consistent platform and passthru updates.The changes mirror the updates in pgmq.nix and postgis.nix, correctly migrating to
hostPlatform.systemand extending the passthru surface withinstalledExtensions,withJIT, andwithoutJIT.Also applies to: 24-24, 27-28, 66-66, 71-73, 87-92
nix/ext/tests/pg_repack.nix (1)
7-9: LGTM: Consistent platform and passthru updates.The changes are consistent with the other test files, correctly applying the hostPlatform.system migration and passthru attribute extensions.
Also applies to: 24-24, 27-28, 66-66, 88-90, 104-109
nix/ext/tests/default.nix (2)
15-17: LGTM: Consistent platform and passthru updates.The migration to
hostPlatform.systemand the addition of passthru attributes (withJIT,withoutJIT,installedExtensions) are consistent with the other test files and correctly implement the PR's objectives.Also applies to: 34-36, 52-53
163-163: No action required —PostgresExtensionTestalready accepts and properly handles thelib_nameparameter.The
PostgresExtensionTestclass inlib.py(line 24) acceptslib_nameas an optional parameter, documented as "Name of the shared library (defaults to extension_name)" (line 34), and correctly assigns it with a fallback to the extension name (line 42). The parameter is actively used in version validation assertions throughout the test class (lines 169, 179, 187).nix/ext/tests/orioledb.nix (2)
14-24: LGTM! Platform and passthru changes are correct.The migration from
pkgs.systemtopkgs.stdenv.hostPlatform.systemcorrectly addresses the deprecated API. The hardcodedpsql_orioledb-17reference is appropriate since OrioleDB only supports PostgreSQL 17. The newinstalledExtensions,withJIT, andwithoutJITpassthru attributes extend the package interface consistently with other extension tests.
40-42: LGTM!The
psql_orioledbbinding correctly useshostPlatform.systemfor the platform-specific package lookup.nix/ext/tests/pg_safeupdate.nix (1)
7-9: LGTM! Consistent platform migration and passthru extension.The changes correctly migrate from the deprecated
pkgs.systemtopkgs.stdenv.hostPlatform.systemacross all package lookups (installedExtension,psql_15,psql_17). The new passthru attributes (installedExtensions,withJIT,withoutJIT) align with the pattern established across other extension test files.Also applies to: 24-24, 27-28, 44-45
nix/ext/tests/pgsodium.nix (2)
7-9: LGTM! Platform migration applied consistently including dependencies.The changes correctly update all platform-specific lookups including the
hypopgdependency at line 21. The passthru additions are consistent with other extension tests.Also applies to: 21-21, 25-25, 28-29
69-69: LGTM!All service package references and migration script bindings correctly use
hostPlatform.system.Also applies to: 78-80, 94-99
nix/ext/tests/vault.nix (1)
7-9: LGTM! Platform migration and passthru additions are correct.All platform-specific lookups including the
pgsodiumdependency have been correctly migrated to usehostPlatform.system. The passthru interface extensions are consistent with the other test files.Also applies to: 21-21, 25-25, 28-29, 50-51
nix/ext/tests/plv8.nix (1)
8-10: LGTM! Platform migration correctly applied to PG15-only extension.The changes correctly migrate to
hostPlatform.system. The absence ofpsql_17is intentional as noted in the file comment (line 1) — plv8 doesn't support PostgreSQL 17. The passthru additions are consistent with other test files.Also applies to: 25-25, 28-29, 45-45, 68-68
nix/ext/tests/pgjwt.nix (1)
7-9: LGTM! Standard platform migration pattern applied.The
installedExtensionlookup andpsql_15/psql_17bindings correctly usehostPlatform.system. The passthru additions are consistent with the established pattern.Also applies to: 24-24, 27-28, 44-45
nix/ext/tests/plpgsql_check.nix (1)
7-9: LGTM! Platform migration and passthru extension complete.All platform-specific lookups correctly migrated to
hostPlatform.system. The passthru attributes (installedExtensions,withJIT,withoutJIT) are consistent with the other extension test files.Also applies to: 24-24, 27-28, 44-45
nix/ext/tests/pgrouting.nix (3)
7-9: LGTM: Deprecated system attribute correctly replaced.The migration from
pkgs.systemtopkgs.stdenv.hostPlatform.systemis correct and aligns with the PR's objective to eliminate deprecated system attribute usage.
17-25: LGTM: Path resolution correctly updated to use hostPlatform.The buildEnv paths now correctly reference extensions using
hostPlatform.system, maintaining consistency with the platform migration. The conditional OrioleDB inclusion logic is sound.
28-28: LGTM: Passthru surface expanded appropriately.The
installedExtensionsfield provides extension metadata and aligns with the standardized passthru interface introduced across test files in this PR.nix/cargo-pgrx/mkPgrxExtension.nix (2)
36-53: Correct system derivation in fetchTarball.The explicit derivation of
systemfromstdenv.hostPlatform.systemat line 45 is correct and ensures the imported nixos-22.11 packages match the host platform. This properly replaces the previous deprecated pattern while maintaining the bindgenHook workaround for older pgrx versions.
7-7: Modern Nix pattern correctly implemented.The migration from
systemparameter tostdenvfollows current Nixpkgs conventions. All call sites (nix/ext/wrappers/default.nix,nix/ext/pg_jsonschema/default.nix, andnix/ext/pg_graphql/default.nix) havestdenvavailable in scope, andcallPackageswill automatically inject it tomkPgrxExtension.nix.nix/postgresql/generic.nix (4)
159-159: __structuredAttrs enables structured attribute handling.The addition of
__structuredAttrs = trueis likely required for compatibility with the updated nixpkgs version. This changes how attributes are passed to build scripts but the extensive E2E testing confirms compatibility.
323-337: Correct migration to hostPlatform.system.The tests section correctly uses
stdenv.hostPlatform.systeminstead of the deprecated system attribute references. This aligns with the PR's objective of replacing deprecated system attributes throughout the codebase.
109-110: Good documentation of the removed hardening flag.The comment clearly explains why the
piehardening flag was removed (now enabled by default in compilers). This helps future maintainers understand the change.
69-69: TheisOrioleDBpattern fallback is defensive but never executes in practice.The default parameter
isOrioleDB ? (builtins.match "[0-9][0-9]_.*" version) != nullonly applies ifisOrioleDBis not explicitly provided. However,default.nixalways explicitly passesisOrioleDBbased on the flavor definition (eitherfalsefor standard PostgreSQL ortruefor OrioleDB). Consequently, the pattern matching fallback is never used and poses no risk of unintended matches.
| readline, | ||
| openssl, | ||
| icu, | ||
| icu75, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the relevant sections
git ls-files nix/postgresql/generic.nixRepository: supabase/postgres
Length of output: 87
🏁 Script executed:
# Read the file to verify the ICU 75 changes and documentation
cat -n nix/postgresql/generic.nix | head -160Repository: supabase/postgres
Length of output: 5433
🌐 Web query:
ICU 75 security vulnerabilities advisories
💡 Result:
Brief summary:
- ICU 75 (release 75.1) was published April 17, 2024. [1]
- A notable recent security advisory for ICU is CVE-2025-5222: a stack-buffer-overflow in the genrb tool (SRBRoot::addTag) that can cause memory corruption / local code execution; NVD entry and vendor advisories list affected ICU releases (fixed in later releases). [2][3]
- Multiple Linux vendors have published security updates addressing CVE-2025-5222 (examples: SUSE, Debian/Ubuntu advisories and package updates). Apply vendor patches or upgrade libicu to a fixed version. [4][3]
- Note: “ICU” can also mean Johnson Controls’ iSTAR Configuration Utility (ICU) — that is a different product with its own CISA advisory (stack-based buffer overflow, April 2025). If you meant that ICU, see the CISA advisory. [5]
Sources:
[1] ICU 75 release page.
[2] NVD — CVE-2025-5222.
[3] Debian/Ubuntu security trackers (CVE-2025-5222).
[4] SUSE security update for icu (CVE-2025-5222).
[5] CISA ICS advisory for Johnson Controls iSTAR Configuration Utility (different “ICU”).
Address CVE-2025-5222 vulnerability in pinned ICU 75.
ICU 75 has a documented security vulnerability (CVE-2025-5222: stack-buffer-overflow in genrb tool) that can cause memory corruption or local code execution. While the ICU 75 pinning is well-documented here for collation stability, confirm whether this vulnerability is patched in the Nix package or has been otherwise mitigated. If not patched, consider upgrading to a fixed ICU version or applying the vendor security patch.
🤖 Prompt for AI Agents
In @nix/postgresql/generic.nix at line 15, The pinned dependency icu75 in the
Nix expression is flagged for CVE-2025-5222; locate the icu75 symbol in this
file and verify whether the Nix package includes the upstream security fix or
local backport; if it does not, update the pin to a patched upstream version
(e.g., a fixed minor release) or apply the vendor security patch as a patch file
in the derivation (add the patch and include it in the src/patches list), then
rebuild and run tests to confirm collations still match and the CVE is
mitigated.
| (replaceVars ./patches/locale-binary-path.patch { | ||
| locale = "${if stdenv.isDarwin then darwin.adv_cmds else lib.getBin stdenv.cc.libc}/bin/locale"; | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check uses of stdenv vs stdenv' throughout the file
echo "=== All uses of stdenv' ==="
rg -n "stdenv'" nix/postgresql/generic.nix | head -30
echo -e "\n=== All uses of bare stdenv.* (checking after line 85) ==="
rg -n "stdenv\." nix/postgresql/generic.nix | head -30
echo -e "\n=== Context around line 85 (stdenv' definition) ==="
sed -n '80,90p' nix/postgresql/generic.nix
echo -e "\n=== Context around line 198 (the questioned line) ==="
sed -n '195,202p' nix/postgresql/generic.nixRepository: supabase/postgres
Length of output: 2799
Use stdenv' instead of bare stdenv for consistency.
At line 198, the code uses stdenv.isDarwin while the entire rest of the file uses stdenv' (defined at line 85 as JIT-aware). All other platform checks in the patches section and throughout the derivation use stdenv'. Change both stdenv.isDarwin and stdenv.cc.libc to use stdenv' for consistency.
🤖 Prompt for AI Agents
In @nix/postgresql/generic.nix around lines 197 - 199, The patch uses bare
stdenv in the replaceVars call; change both occurrences to the JIT-aware stdenv'
used elsewhere: replace stdenv.isDarwin with stdenv'.isDarwin and stdenv.cc.libc
with stdenv'.cc.libc inside the replaceVars invocation (the expression building
locale in the locale-binary-path.patch replacement) so the platform checks and
libc reference are consistent with the rest of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @ansible/vars.yml:
- Around line 13-15: The version values for keys postgresorioledb-17, postgres17
and postgres15 include a test suffix "-update-nixpkgs-6" that will break the AMI
build; revert those three strings to their canonical base versions by removing
the "-update-nixpkgs-6" suffix so they become "17.6.0.024-orioledb" for
postgresorioledb-17, "17.6.1.067" for postgres17 and "15.14.1.067" for
postgres15.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ansible/vars.yml
⏰ 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 (1)
ansible/vars.yml (1)
13-15: Clarify the nixpkgs commit IDs and verify IPv6 failures independently.The specific nixpkgs commit IDs referenced (a76c4553d7e741e17f289224eda135423de0491d → f61125a668a320878494449750330ca58b78c557) cannot be located in public nixpkgs repositories, which prevents verification of whether IPv6/DNS changes were introduced.
Before attributing the "Network is unreachable" errors to the nixpkgs update, confirm:
- The commit IDs belong to a public repository, or provide the full compare URL/diff
- The failures reproduce consistently with the previous nixpkgs pin
- Other upgrade tests with the new nixpkgs version pass without IPv6 errors
If the commits are correct and contain actual changes, ensure IPv6 stack tests pass independently to rule out system-level networking configuration issues.
| postgresorioledb-17: "17.6.0.024-orioledb-update-nixpkgs-6" | ||
| postgres17: "17.6.1.067-update-nixpkgs-6" | ||
| postgres15: "15.14.1.067-update-nixpkgs-6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for references to these version strings and postgres_release usage
echo "=== Searching for '-update-nixpkgs-6' references ==="
rg -n "update-nixpkgs-6" --type yaml --type yml
echo "=== Searching for 'postgres_release' usage ==="
rg -n "postgres_release" --type yaml --type yml -A 2
echo "=== Searching for version strings in shell/deployment scripts ==="
rg -n "postgresorioledb-17|postgres17|postgres15" --type sh --type yaml --type yml | head -30
echo "=== Looking for related version identifiers with '-update-nixpkgs' ==="
rg -n "update-nixpkgs" --type yaml --type yml
echo "=== Checking for any release or version documentation ==="
fd -e md -e txt | xargs rg -l "postgre.*version|release.*version" 2>/dev/null | head -10Repository: supabase/postgres
Length of output: 763
🏁 Script executed:
#!/bin/bash
# Read the ansible/vars.yml file to see context
echo "=== Contents of ansible/vars.yml ==="
cat -n ansible/vars.yml 2>/dev/null || echo "File not found"
echo ""
echo "=== Searching for postgres_release (all file types) ==="
rg -n "postgres_release" -A 2 -B 2 2>/dev/null | head -40
echo ""
echo "=== Checking git status to see if file is new/modified ==="
git log --oneline -n 1 -- ansible/vars.yml 2>/dev/null || echo "Not in git"
echo ""
echo "=== Searching for any YAML variable definitions ==="
fd -e yaml -o -e yml 2>/dev/null | head -10
echo ""
echo "=== Looking for vars files broadly ==="
find . -name "*vars*" -type f 2>/dev/null | grep -E "\.(yml|yaml|sh)$" | head -15Repository: supabase/postgres
Length of output: 3679
🏁 Script executed:
#!/bin/bash
echo "=== Git history of ansible/vars.yml (last 10 commits) ==="
git log --oneline -n 10 -- ansible/vars.yml
echo ""
echo "=== Recent changes to postgres_release versions ==="
git log -p -n 5 -- ansible/vars.yml | head -100
echo ""
echo "=== Examining build-test-ami.nix usage ==="
cat -n nix/packages/build-test-ami.nix | sed -n '90,110p'
echo ""
echo "=== Search for other references to postgres_release ==="
rg -n "postgres_release" -B 2 -A 2
echo ""
echo "=== Check for any deployment/provisioning scripts that use these versions ==="
fd -type f -name "*.sh" -o -name "*.nix" | xargs rg -l "PG_VERSION|postgres.*version" | head -10Repository: supabase/postgres
Length of output: 2449
🏁 Script executed:
#!/bin/bash
echo "=== Search for version format documentation ==="
rg -n "version.*format|version.*scheme|postgres.*version" nix/docs/ -A 2 -B 2
echo ""
echo "=== Check README files for version info ==="
rg -n "postgres_release|version.*string" README.md CONTRIBUTING.md 2>/dev/null || echo "Not found in main docs"
echo ""
echo "=== Look for examples of postgres versions without test suffixes ==="
rg -n "postgres.*[0-9]+\.[0-9]+\.[0-9]+" --no-heading | grep -v "update-nixpkgs" | head -20
echo ""
echo "=== Check git diff to see what changed in this commit ==="
git show fa26b67 --stat
echo ""
echo "=== Full diff of the version bump commit ==="
git show fa26b67Repository: supabase/postgres
Length of output: 50378
Revert test version suffixes before merge.
The version strings in lines 13-15 contain the -update-nixpkgs-6 suffix, which the git commit message confirms are test identifiers ("chore: bump versions for testing"). These versions are consumed by the AMI build pipeline (nix/packages/build-test-ami.nix) and will cause build failures if the suffixed versions don't exist in artifact registries. Revert to base version strings:
"17.6.0.024-orioledb"(from"17.6.0.024-orioledb-update-nixpkgs-6")"17.6.1.067"(from"17.6.1.067-update-nixpkgs-6")"15.14.1.067"(from"15.14.1.067-update-nixpkgs-6")
🤖 Prompt for AI Agents
In @ansible/vars.yml around lines 13 - 15, The version values for keys
postgresorioledb-17, postgres17 and postgres15 include a test suffix
"-update-nixpkgs-6" that will break the AMI build; revert those three strings to
their canonical base versions by removing the "-update-nixpkgs-6" suffix so they
become "17.6.0.024-orioledb" for postgresorioledb-17, "17.6.1.067" for
postgres17 and "15.14.1.067" for postgres15.
|
The engines smoke test which brings up a Supabase project with each Postgres engine at the AMI versions in this PR and then runs the engine-appropriate tests from nix/tests against them all passed The pause/restore smoke tests which bring up a Supabase project, pause it (taking a backup, retiring the VM), and then restore it (to a new VM, restoring the backup) for every Postgres engine all passed. The 15->17 and 17->17 upgrade smoke tests both passed (output truncated) |
Use one recent version of nixpkgs in the flake inputs.
a76c4553d7e741e17f289224eda135423de0491d -> f61125a668a320878494449750330ca58b78c557
Summary by CodeRabbit
New Features
Bug Fixes
Updates
Database
✏️ Tip: You can customize this high-level summary in your review settings.