Initialize cargo-pgrx when building without an existing configuration - #370
Merged
Conversation
`pgxn install pg_durable` fails on a clean machine, 16 seconds in:
DEBUG: running: ['gmake', 'PG_CONFIG=/usr/bin/pg_config', 'all']
cargo pgrx package --pg-config "/usr/bin/pg_config" ...
Error:
0: $PGRX_HOME does not exist
(pgrx-pg-config-0.16.1/src/lib.rs:582)
gmake: *** [Makefile:27: package] Error 1
`all` builds `package`, which runs `cargo pgrx package`, and cargo-pgrx refuses
to run without $PGRX_HOME/config.toml. Only `cargo pgrx init` creates that file,
and pgxnclient runs `make all` followed by `make install` -- never init. So a
machine with PostgreSQL, Rust and cargo-pgrx installed still cannot build, and
the error names an environment variable that appears nowhere in our docs.
Only the presence of config.toml matters. Verified against the published 0.2.6
archive with cargo-pgrx 0.16.1:
- $PGRX_HOME absent -> "$PGRX_HOME does not exist"
- $PGRX_HOME present but empty -> "config.toml not found. Have you
run `cargo pgrx init` yet?"
- config.toml naming only a nonexistent
pg18 install -> builds fine against pg17
The recorded entries are never consulted because `package` always passes an
explicit --pg-config, so an existing pgrx configuration for another PostgreSQL
cannot redirect the build. `package` therefore runs init only when config.toml
is missing entirely, which cannot disturb an existing setup. Passing a real
pg_config makes init a validate-and-record step; it does not download or build
a PostgreSQL of its own.
The guard sits after the unowned-package-directory check so a build that is
about to be refused does not first write to the user's home directory.
PGRX_AUTO_INIT=0 opts out and reports the command to run instead.
Also adds the two targets the reference PGXN pgrx distribution provides, so the
documentation has something to name:
make install-pgrx installs the cargo-pgrx release pinned in Cargo.toml
make pgrx-init registers PG_CONFIG with cargo-pgrx
scripts/test-make-install.sh now points PGRX_HOME at its sandbox, so the checks
neither depend on nor disturb the caller's pgrx installation. Without that the
suite passed only on machines that happened to have ~/.pgrx already, which is
exactly why CI never caught this: every runner that builds the extension runs
`cargo pgrx init` first. Five cases cover the new behaviour: auto-init on a
clean home, no re-init when a configuration exists, the PGRX_AUTO_INIT=0
message, pgrx-init deriving the major from pg_config, and install-pgrx using
the pinned version.
Verified end to end on Debian bookworm with PostgreSQL 17 from PGDG and no
~/.pgrx: `pgxn install pg_durable` now builds and installs, and the offline
contract tests pass on a machine with no pgrx configuration.
Refs \#359 (item 1)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Pino de Candia (pinodeca)
September 2, 2026 00:16
View session
There was a problem hiding this comment.
🟡 Changes recommended
The new package init guard currently exits the recipe early when config.toml exists, skipping the actual packaging step.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aims to make PGXN/source installs succeed on a clean machine by ensuring cargo pgrx package can run even when the user has never run cargo pgrx init, and by adding explicit Makefile targets to install/init the pinned cargo-pgrx toolchain.
Changes:
- Add a guarded auto-
cargo pgrx initstep tomake packagewhen$PGRX_HOME/config.tomlis missing (withPGRX_AUTO_INIT=0opt-out messaging). - Add
install-pgrxandpgrx-initMakefile targets for the pinned cargo-pgrx version and one-time initialization. - Update
scripts/test-make-install.shto use an isolatedPGRX_HOMEsandbox and add cases covering the new init behavior and opt-out.
File summaries
| File | Description |
|---|---|
| Makefile | Adds auto-init guard plus install-pgrx / pgrx-init targets to support clean-machine builds. |
| scripts/test-make-install.sh | Sandboxes PGRX_HOME and adds regression coverage for the new Makefile behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Pino de Candia (pinodeca)
approved these changes
Sep 2, 2026
Contributor
There was a problem hiding this comment.
Code Review
Scope: PR 370 (waldemort/pgrx-init-guard vs origin/main)
Intent: Make pgxn install / make package work on a clean machine by auto-running cargo pgrx init only when $PGRX_HOME/config.toml is missing; add install-pgrx / pgrx-init; sandbox Makefile tests so they no longer inherit the caller’s pgrx home.
Mode: Interactive
Reviewers: correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, previous-comments (existing Copilot thread), security (home-dir write from make package)
P1 -- High
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 1 | test-make-install.sh:305 |
Existing-config case asserted only that init was skipped, not that packaging still ran. That would hide a future .ONESHELL / recipe-merge that made exit 0 skip cargo pgrx package. |
testing | 0.92 | safe_auto → review-fixer (applied) |
P2 -- Moderate
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 2 | Makefile:35 |
if test -f "$(PGRX_CONFIG)"; then exit 0; fi is correct without .ONESHELL (later recipe lines still run), but it reads like a full-recipe return. Invert to if test ! -f ...; then init; fi so a later .ONESHELL cannot turn make package into a no-op. |
maintainability | 0.90 | gated_auto → human (applied in 5088079) |
P3 -- Low
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 3 | Makefile:134 |
Make-only PGRX_HOME_DIR is not exported. make package PGRX_HOME=... (Make var, not env) can check a different path than cargo pgrx reads. Tests only use env prefix PGRX_HOME=... make. |
maintainability | 0.74 | advisory → human |
| 4 | Makefile:118 |
Comment block narrates the old PGXN failure and “config.toml is never consulted.” Keep the contract (init iff missing; always --pg-config; PGRX_AUTO_INIT=0 opts out); move the rest to the PR/changelog. |
maintainability | 0.66 | advisory → human |
Applied Fixes
- Strengthened the existing-config case in
test-make-install.shto requirepgrx packageinCARGO_LOGand the packaged.so. - Inverted the
packageinit guard so a presentconfig.tomlskips init instead ofexit 0. Packaging always continues. ./scripts/test-make-install.shpassed.- Landed in
5088079onwaldemort/pgrx-init-guard.
Pre-existing
None.
Learnings & Past Solutions
- No
docs/solutions/catalog. - Contract is issue #359 item 1: clean machine, PG + Rust + cargo-pgrx, no
~/.pgrx,pgxn installmust work (or namemake pgrx-init). Items 2–3 (docs,no_index) are out of scope.
Agent-Native Gaps
None. make help lists the new targets; PGRX_AUTO_INIT=0 prints the exact recovery command.
Coverage
- Suppressed: 0
- Untracked excluded: none
- Failed reviewers: none
- Copilot’s inline comment (
exit 0skips packaging) is wrong under GNU Make without.ONESHELL. Each recipe line is a separate shell;exit 0ends only the init-guard line. Do not treat that thread as unaddressed valid feedback. The P2 invert still removes that footgun. - Residual risks: first
make packagewrites~/.pgrx/config.tomlby design (PGRX_AUTO_INIT=0opts out). Auto-init tests use a fakecargo, not real cargo-pgrx. - Testing gaps: no coverage for
make package PGRX_HOME=...as a Make variable vs env; fallback$(HOME)/.pgrxwhen unset is not exercised.
Verdict: Ready to merge for item 1 of #359. P1 test assertion and P2 init-guard invert are applied. Remaining work is optional P3 (PGRX_HOME export / comment trim).
Invert the cargo-pgrx init guard so a present config.toml simply skips init. `exit 0` was correct without .ONESHELL, but it read like a full recipe return and would skip packaging if those lines were later joined. Assert the existing-config path still runs `cargo pgrx package`.
Pino de Candia (pinodeca)
added a commit
that referenced
this pull request
Sep 2, 2026
… PGXN install (#373) * Keep internal documents out of the PGXN search index and document the PGXN install Addresses items 2 and 3 of \#359 together, because they interlock: the README is what PGXN renders as the distribution landing page AND is itself an indexed document, so adding install docs and changing what gets indexed have to be verified against each other. ## no_index (item 3) PGXN indexes documentation for full-text search. Both published releases index 69 documents, 19 of which are internal: all seven files under prompts/ (including the release runbook that describes our own PGXN publishing procedure), .github/copilot-instructions, the .agents SQL-generation skill, three open-problem notes, the repo TODO, the website sources, and two requirements.txt pip manifests, which Text::Markup treats as documents because of the .txt extension. Nothing here is secret; the repository is public. The problem is signal: a reader searching PGXN for pg_durable currently finds our internal AI development prompts and a list of our open bugs presented as product documentation. This compounds. A PGXN release is immutable, so 0.2.6 and 0.2.7 keep their 69-document indexes permanently, and every further release published without no_index adds another. The PGXN Meta Spec has a field for exactly this, and it is implemented in pgxn-api lib/PGXN/API/Indexer.pm find_docs(). It affects only indexing and search, so the release archive and the GitHub source assets stay byte-identical. .gitattributes export-ignore would also have suppressed these files, but it removes them from the tarball entirely and would silently change the GitHub release archives built by package-release.yml as well. Two matching rules worth knowing before editing the list: - file entries are exact string equality INCLUDING the extension, while the API's docs keys are extension-stripped. Copying a key from api.pgxn.org/dist/pg_durable.json does not work: docs/dep_issues must be written docs/dep_issues.md. - directory entries are an anchored literal prefix match (/^\Q$_/), not a path-component match, so "docs" would also match a hypothetical docs2/. The trailing slash is deliberate. provides.pg_durable.docfile is collected before the skip checks run, so USER_GUIDE.md cannot be excluded by accident. README has no such protection -- its skip check runs before PGXN's README special case -- so it is deliberately absent from the list. Verified by reimplementing find_docs() against the tracked file list: the indexed set drops from 69 to 50, all 19 internal entries are excluded, zero residual prompts/ .github/ .agents/ or docs/website/ entries remain, and README, USER_GUIDE, CHANGELOG, LICENSE, SECURITY, CONTRIBUTING and CODE_OF_CONDUCT all survive. Left indexed deliberately: the docs/spec-* set, ARCHITECTURE, api-reference, grammar, and all 13 examples/ documents. The security review, the design and proposal documents, the test plans, and docs/pg_durable_mvp are arguable either way and are left alone for now; unlike a release, this list is not immutable and can be tightened later at no cost. ## Install documentation (item 2) PGXN renders README.md as the distribution landing page, so a PGXN visitor is already reading it. Until now the entire treatment was one sentence at the end of the Packages section saying PGXN carries the source distribution "built and installed exactly as described above" -- which never showed the pgxn command, and pointed at the tarball recipe, a different entry point. The new section states plainly that PGXN ships source rather than a binary, lists the prerequisites including the compile time, and names the pgxn install and pgxn uninstall commands. It also documents the auto-init behaviour added in \#370 and the make pgrx-init / make install-pgrx targets that go with it. All links in the new section are absolute. Relative links do not resolve when the README is rendered on pgxn.org -- pgxn.org/dist/pg_durable/USER_GUIDE.md is a 404 -- so a relative link would be broken for exactly the readers this section is written for. USER_GUIDE.md is the provides docfile and PGXN links it as "Documentation", but its Prerequisites began after installation, with shared_preload_libraries. It now starts by naming the install channels and linking to Packages, with an absolute URL for the same reason. Refs \#359 (items 2 and 3) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Correct the pgxn install command: --sudo and the -- separator are both required Review of \#373 caught that the documented `pgxn install pg_durable` fails on the exact platform the section names. Confirmed three ways. pgxnclient elevates only when told to. In commands/__init__.py the --sudo option is declared with const='sudo' and nargs='?' but NO default=, so argparse leaves opts.sudo as None when the flag is absent, and SudoInstallUninstall.run() then raises before building: ERROR: PostgreSQL library directory (/usr/lib/x86_64-linux-gnu) not writable: you should run the program as superuser, or specify a 'sudo' program Reproduced as a non-root user against a PGDG PostgreSQL 17: exit 1, no build attempted. Upstream docs/usage.rst says the same. The obvious correction, `pgxn install --sudo pg_durable`, is also broken. Because --sudo takes an optional PROG, argparse consumes pg_durable as that argument and leaves no distribution to install: $ pgxn install --sudo pg_durable usage: pgxn install [--help] ... [--sudo [PROG] | --nosudo] exit 2 The separator is therefore mandatory here, not optional as upstream's note suggests, because --sudo is always the last option before the distribution name: pgxn install --sudo -- pg_durable Verified end to end as a non-root user with PostgreSQL 17 from PGDG and no ~/.pgrx: exit 0, pg_durable.so installed root-owned into /usr/lib/postgresql/17/lib, while ~/.pgrx stays owned by the building user -- confirming the build runs unprivileged and only the install step is elevated, which is what the text now says. uninstall takes the same flags. Also replaces the cargo-pgrx prerequisite. It pointed at `make install-pgrx`, which needs a source checkout that a PGXN user does not have -- `pgxn install` downloads the source itself -- and never named the pinned version. It now gives the direct cargo install command, and mentions the make targets afterwards as the checkout-based alternative. Why the original text was wrong: Install._inun calls run_make('install', sudo=self.get_sudo_prog()), which reads as unconditional elevation. get_sudo_prog() returns opts.sudo, whose default is None. The end-to-end validation missed it because it ran as root, where is_libdir_writable() is true and the sudo branch never executes. Refs \#359 (item 2) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Pino de Candia <pinod@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes item 1 of #359.
The problem
pgxn install pg_durablefails on a clean machine after 16 seconds, even with PostgreSQL, Rust and cargo-pgrx all installed:allbuildspackage, which runscargo pgrx package, and cargo-pgrx refuses to run without$PGRX_HOME/config.toml. Onlycargo pgrx initcreates that file, and pgxnclient runsmake allthenmake install— never init.This is the first thing a PGXN user hits, and the error names an environment variable that appears nowhere in our documentation.
What the fix required — narrower than it looks
Three probes against the published archive with cargo-pgrx 0.16.1:
$PGRX_HOMEstategmake PG_CONFIG=... all$PGRX_HOME does not existconfig.toml not found. Have you run `cargo pgrx init` yet?config.tomlnaming only a nonexistentpg18installSo only the presence of
config.tomlmatters. Its contents are never consulted, becausepackagealways passes an explicit--pg-config. Two consequences that make auto-init safe here:cargo pgrx init --pg<major> <pg_config>is instant when handed an existingpg_config— it validates and records, it does not download or build a PostgreSQL of its own.The change
packageruns init only whenconfig.tomlis missing entirely, so it cannot disturb an existing setup. The guard sits after the unowned-package-directory check, so a build that is about to be refused does not first write to the user's home directory.PGRX_AUTO_INIT=0opts out and reports the command to run instead:Also adds the two targets that
theory/pg-jsonschema-boonprovides — the pgrx extension published on PGXN by PGXN's own author, and the distribution whose metadata shape we already follow — so the documentation for item 2 has something concrete to name:make install-pgrxCargo.tomlmake pgrx-initPG_CONFIGwith cargo-pgrxWhy CI never caught this
scripts/test-make-install.shinherited the caller's$PGRX_HOME, so it passed on any machine that happened to have~/.pgrx— which every runner that builds the extension does, because it runscargo pgrx initfirst.The suite now points
PGRX_HOMEat its own sandbox, so the checks neither depend on nor disturb the caller's pgrx installation. Five cases cover the new behaviour:PGRX_AUTO_INIT=0fails, names the command, and invokes cargo zero timespgrx-initderives the major frompg_config, notPG_VERSIONinstall-pgrxuses the version pinned inCargo.tomlValidation
Debian bookworm, PostgreSQL 17.11 from PGDG, Rust 1.98.0, cargo-pgrx 0.16.1, pgxnclient 1.3.2, no
~/.pgrx— running exactly what pgxnclient runs:Result:
pg_durable.so(11,684,904 bytes) in/usr/lib/postgresql/17/lib/, the control file, and 9 SQL files installed;~/.pgrx/config.tomlcreated withpg17 = "/usr/bin/pg_config". Before this change the same sequence failed atmake all.scripts/test-make-install.shpasses on a machine with no pgrx configuration at all — the condition that would have caught the original bug.Scope
Item 1 of #359 only. Item 2 (documenting the PGXN install path) and item 3 (
no_index) are separate.Generated with the assistance of GitHub Copilot.