Fix make install for source builds - #345
Conversation
|
Added two maintainer follow-up commits after reviewing the original
The key reason for the change is PGXN's execution model: it runs The revised flow is deliberately two-phase:
The follow-up also:
Local validation completed:
The dedicated |
There was a problem hiding this comment.
Added a fourth commit, 4dd59a6, covering the uninstall gap and the packaging-drift risk raised in review.
(Apologies for the preceding one-line review — the App cannot post through the issue-comments endpoint, so that was a probe to find a path that works. It cannot be edited or deleted through the API available to me.)
pgxn uninstall pg_durable currently fails. pgxnclient's Uninstall command runs make uninstall under the same sudo rules as install, and no such target exists. The new target is the inverse of install and follows the same two-phase reasoning: pgxnclient unpacks a distribution and runs uninstall directly, without building first, so it must not read the package tree or invoke Cargo. It is idempotent, honors DESTDIR, and is excluded alongside install when installcheck is also requested, since PGXS defines its own uninstall recipe too.
On .deb versus source-install divergence. Checked against cargo-pgrx's install_extension, which emits exactly three things: the control file, the shared library, and the SQL files. So the packaged tree and what install copies agree today, and no restructuring is warranted. What was missing is anything holding that true: package-deb.sh copies the whole usr/ tree, so a future cargo-pgrx emitting anything more would ship it in the .deb and silently drop it from source installs.
install now fails when the packaged tree contains a file it does not install, naming the file. That turns a silent divergence into a build failure where the decision belongs. Since the release workflow already stages through make install, it is enforced on every release with no extra wiring.
Test coverage, including two invocation shapes that were previously untested:
| Case | Why |
|---|---|
| uninstall removes exactly what install placed | with Cargo off PATH, caller-supplied PGXS, missing package dir, DESTDIR staging |
| uninstall is idempotent | a partial install must always be cleanable |
| install rejects an unrecognized packaged file | the drift guard, using a stray .bc |
make installcheck invoked directly |
pgxn check calls it with PG_CONFIG on the command line and no wrapper; CI previously reached installcheck only through make test-regress |
| uninstall + installcheck refused together | matches the existing install rule |
The stale assertion for the goal-conflict message was updated to match its new wording.
Verified with PostgreSQL 17.10 and make 4.3: scripts/test-make-install.sh passes, and build, all, package, install, uninstall, installcheck and help each produce their expected recipe with no warnings.
One finding outside this commit: macOS source installs cannot work. cargo-pgrx names the library using DLL_SUFFIX, which is .dylib on macOS for PostgreSQL 16 and later, while install looks for pg_durable.so. Reasonable given the release targets Debian/Linux, but a PGXN listing reaches macOS users, so it may deserve either support or an explicit statement.
Generated with the assistance of GitHub Copilot.
Build cargo-pgrx package artifacts before the privileged install phase so PGXN installs do not depend on Cargo surviving sudo environment filtering. Add copy-only install and uninstall targets with PG17/PG18 validation, DESTDIR support, package drift checks, and conventional pg_config discovery. Exercise the source workflow in CI and release packaging, pin the new action reference, and document direct installcheck as an advanced operation against a disposable configured server.
4dd59a6 to
55d29e7
Compare
|
@waldemort-auto[bot] please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
The bug
make installcannot install pg_durable. PGXS supplies aninstallrecipe that copies only files named by itsDATAandMODULESvariables. Neither is set, so it installs only the uninterpolatedpg_durable.controltemplate: no shared library, no generated SQL, and an invalid@CARGO_VERSION@default version.This becomes user-facing when pg_durable is distributed through PGXN.
pgxn installrunsmakeas the invoking user, then runsmake installthrough sudo when PostgreSQL's library directory is not writable. Compiling during that privileged phase depends on Cargo and the user's cargo-pgrx configuration surviving sudo environment filtering, which is not normally true. PGXN also invokesmake uninstalldirectly, but the repository did not provide that target.This is the companion build fix for #344.
The fix
Source installation now follows a conventional two-phase model:
make uninstallis the copy-only inverse and does not require a package tree, so PGXN can remove an installed source distribution without rebuilding it.Additional behavior:
pg_config; unsupported majors fail before building.PG_CONFIGis honored. Otherwise Make first asks cargo-pgrx forPG_VERSION(defaultpg17) and then follows the conventionalpg_configonPATHfallback.EXTRA_FEATURESadds non-PostgreSQL Cargo features, for exampleEXTRA_FEATURES=http-allow-azure-domains.DESTDIRstages install and uninstall operations for package builders.installcheck, eliminating its conflicting install/uninstall recipes.make package; the standard dedicated output remains rebuildable.alltarget now builds release package artifacts.make build,pg-install,pg-clean, and PGXSinstallcheckretain their explicit roles.Regression tests against PostgreSQL
make test-regressremains the recommended contributor command. It resets and configures the dedicated local cluster before running PGXS regressions.Direct
make installcheckremains available for an already-running disposable test server. PostgreSQL'spg_regressdrops and recreatesCONTRIB_TESTDBunless explicitly placed in--use-existingmode, and pg_durable's current regression initialization is not designed for a pre-populated database. The documentation now provides the requiredPGHOST,PGPORT,PGUSER,PG_CONFIG, preload, worker-database, privilege, and restart requirements and warns not to target valuable data.Automated coverage
scripts/test-make-install.shprovides offline checks for:pg_configdiscoverymaketargetDESTDIR, paths containing spaces, installed file modes, and caller-suppliedPGXSThe release workflow builds through the Makefile for both PG17 and PG18, stages each real cargo-pgrx package through
make install, and verifies the library, control file, install SQL, and upgrade SQL before Debian packaging. The source-install CI job uses the same full-length action SHA pinning convention as the rest of the repository.Verification
Validated locally with PostgreSQL 17.10 and GNU make 4.3:
make package.make install DESTDIR=....pg_durable:0.2.6successfully.make test-regress.installcheckdiscovers a supported PG17pg_configfromPATHwith cargo-pgrx unavailable.The branch is rebased onto current
mainand contains one commit authored by Pino de Candia (pinodeca).Generated with the assistance of GitHub Copilot.