Repository navigation
Extension system-dep checks, ephemeral per-shell use, and download/extraction robustness - #15
Merged
Conversation
Recommend missing system dev-libraries (e.g. postgresql-libs for pdo_pgsql/pgsql on Arch) during extension add and doctor. Extension->system-lib knowledge moves from a hardcoded switch in the terminal layer into RequiresPackages on the ExtensionDef, served through the GraphRepository so it lives in one swappable file and can be updated without a binary change (memory now, REST/PostgreSQL later). - domain: add ExtensionDef.RequiresPackages (plural, system-check path) - memory/graph.go: populate RequiresPackages for gd, intl, libxml, pdo_pgsql, pgsql, zip - system/packages.go: add libpq mapping for arch/ubuntu/alpine/fedora - terminal: checkSystemDeps reads GetExtensionDef().RequiresPackages; extension add gains --auto-deps/--dry-run/--no-system and runs the dep + build-tool checks before building - doctor: inject graph service, check deps of installed extensions only (new Repository.ListInstalledExtensions)
extension add/remove default to the active PHP version instead of requiring a positional version. Use --version to target a specific one: phpv extension add pdo_pgsql phpv extension add pdo_pgsql --version 8.4 pecl install/uninstall gain the same --version flag while keeping the legacy positional [version] arg (both at once is an error). 'phpv use' becomes ephemeral and per-shell. 'phpv init' now emits a phpv() shell function that intercepts 'use' and exports PHPV_CURRENT in the current shell only, so terminal A can use 8 while terminal B uses 7 without touching the global default. A hidden 'use --print' primitive emits the export statement; bare 'use' without shell integration instructs the user instead of silently writing the global default (--global remains opt-in). install.sh next-steps hint updated.
TestResolveActivePHP_NoPHP assumed resolveActivePHP errors when no managed PHP is installed, but the function intentionally falls back to the system php on PATH (php.go:768), so the test was environment- dependent and failed on machines with a system PHP. Split into two deterministic tests: - TestResolveActivePHP_SystemFallback: no managed version, system php on PATH -> returns the system binary - TestResolveActivePHP_NoPHP: no managed version and no php on PATH -> errors
phpv's tar extractor wrote regular files without restoring the archive's mtime, so every file got the current wall-clock time in tar-stream order. A shipped generated source (.c) therefore appeared slightly older than its generator (.re), making make regenerate it during extension builds. pdo_pgsql's shared-extension Makefile never sets RE2C (its config.m4 doesn't reference re2c), so the regeneration rule collapsed to '-o ...' -> '/bin/sh: line 3: -o: command not found', failing 'phpv extension add pdo_pgsql'. Restore header.ModTime via os.Chtimes in both Extract (gz/bz2) and extractXz paths. With mtimes preserved, shipped generated sources are treated as up-to-date and no spurious re2c/bison regeneration occurs. Add TestSiloRepository_Extract_PreservesMtime covering a .c/.re pair with identical archive mtimes.
phar install/update/list now target the active PHP version by default and accept an explicit --version flag, matching extension/pecl: phpv phar install composer # active version phpv phar install composer --version 8.4 The legacy positional [version] argument (previously advertised but silently ignored by install/update, and un-resolved by list) now actually works: 8.4 resolves to the installed 8.4.0. - Rename resolvePeclVersion -> resolveOptionalVersion(cmd, positional) and reuse it from both pecl and phar. Precedence: --version flag, positional, active; both set is an error. - phar which stays active-only. - Add phar tests covering list resolution (positional, --version, both, unknown, active-default) and install/update version precedence.
A fresh 'phpv install' (after rm -rf ~/.phpv) downloads all dependency packages + PHP source at once. Two issues made this appear stuck: - silo.Download used http.Get (Go default client, no timeout), so a stalled/unreachable download URL hung forever. Use a client with a 5-minute timeout so a stalled download fails instead of blocking. - downloadAll ran the downloads in parallel goroutines but emitted no per-package progress, so the spinner showed only 'Downloading and extracting 12 packages...' for the whole phase. Emit per-package messages (Downloading/Extracting/Downloaded <pkg> <ver>) so the spinner shows live activity during the download phase.
extractXz streamed xz's stdout through a pipe into Go's tar reader. If xz filled the 64KB pipe buffer faster than Go drained it, xz blocked on write() forever (observed as xz processes in S+ state with 0:00 CPU), stalling 'phpv install' indefinitely on .tar.xz deps like m4 and libtool. Decompress with the xz binary to a temp file via cmd.Run (no pipe), then extract the tar stream with Go's tar reader. Refactor the duplicated tar extraction loop into a shared extractTarEntries helper used by the gz/bz2 and xz paths. downloadAll now appends a completed-count (N/M) to each per-package 'Downloaded' message so the spinner shows progress during slow downloads. Add TestSiloRepository_ExtractXz_TempFile (xz-compressed archive) and verify the shared helper preserves mtimes for the xz path.
phpv install downloaded and extracted every package in the build plan, even when the system already had a compatible package or the dep was a build tool that is never compiled. On a system with all dev packages present, 11 deps + PHP were fetched when only 1 (e.g. zlib) was built. Move the mustBuild decision before downloadAll (it only needs the build plan and the already-available system package map) and filter downloads through shouldDownloadDep: - build tools and optional no-version deps are never downloaded - system-compatible deps are skipped (system package used instead) - static / --no-system installs still download everything The 'Downloading and extracting N packages' message now reflects the actual download count. Build tools and system-compatible libs are no longer fetched unnecessarily. Add TestShouldDownloadDep and TestComputeMustBuild.
Reverting the ephemeral-by-default flip broke the fresh-install flow: after 'rm -rf ~/.phpv && make install' (no shell integration loaded), 'phpv use 8' errored with a shell-integration guidance message instead of setting the global default, and then 'phpv extension add ctype' failed because no active version was set. Restore --global as the default for 'use' so bare 'phpv use 8' sets the global default out of the box (old behavior, no regression). The per-shell ephemeral switch still works when the phpv() function from 'phpv init' is loaded — it intercepts 'use' and calls the hidden --print mode, so it never touches the global default. --global remains as an explicit override for users with shell integration. Update useCmd help text and install.sh next-steps hint; replace the bare-use guidance test with TestBareUse_DefaultsToGlobal.
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.
Summary
A series of fixes and improvements to the dependency detection, version
selection, and download/extraction pipeline — all centered on the install
and extension management UX. 8 commits, 22 files changed.
System dependency checks (data-driven)
phpv extension add pdo_pgsqlon Arch failed because phpv never recommendedpostgresql-libs. The extension→system-lib knowledge was a hardcoded switch inthe terminal layer; it now lives in
ExtensionDef.RequiresPackagesservedthrough the
GraphRepository, so it can be updated in one file without a binarychange (memory now, REST/PostgreSQL later).
domain/extension.go: addRequiresPackages []string(plural, system-check path)memory/graph.go: populateRequiresPackagesfor gd, intl, libxml, pdo_pgsql, pgsql, zipsystem/packages.go: addlibpqmapping (postgresql-libs / libpq-dev / postgresql-devel)terminal:checkSystemDepsreadsGetExtensionDef().RequiresPackagesdoctor: inject graph service, check deps of installed extensions onlyVersion selection (active by default,
--versionflag)extension add/remove,pecl install/uninstall, andphar install/update/listnow default to the active PHP version and accept a
--versionflag:phpv extension add pdo_pgsql
phpv extension add pdo_pgsql --version 8.4
Legacy positional
[version]is kept forpecl/phar(both sources = error).The
pharpositional was previously advertised but silently ignored byinstall/update and un-resolved by list — now actually works.
Shared helper
resolveOptionalVersion(cmd, positional)with precedence:--versionflag > positional > active.Ephemeral per-shell
phpv usephpv use 8becomes ephemeral and per-shell.phpv initnow emits aphpv()shell function (bash/zsh/ksh + fish + pwsh) that intercepts
useand exportsPHPV_CURRENTin the current shell only, so terminal A can use 8 while terminalB uses 7 without touching the global default. A hidden
use --printprimitiveemits the export; bare
usewithout shell integration instructs the user(
--globalremains opt-in).install.shnext-steps hint updated.Download & extraction robustness
mtime preservation — phpv's tar extractor never restored archive mtimes, so
a shipped
.cappeared older than its.re, triggering spurious re2cregeneration. pdo_pgsql's extension Makefile never sets
RE2C, so the rulecollapsed to
-o: command not found. Fix:os.Chtimesin both gz/bz2 and xz paths.xz pipe deadlock —
extractXzstreamed xz's stdout through a pipe intoGo's tar reader; xz blocked on
write()once the 64KB pipe filled,stalling installs indefinitely on .tar.xz deps (m4, libtool). Fix:
decompress to a temp file via
cmd.Run()(no pipe), then extract. Refactoredthe duplicated tar loop into a shared
extractTarEntrieshelper.Download timeout —
silo.Downloadusedhttp.Get(no timeout); a stalledURL hung forever. Now uses a 5-minute-timeout client.
Per-package progress —
downloadAllemitted no per-package progress; thespinner showed only "Downloading and extracting 12 packages..." for the whole
phase. Now emits per-package messages with a completion count
(N/M).Only download deps that will be compiled — moved the
mustBuilddecisionbefore
downloadAlland filter throughshouldDownloadDep: build tools andsystem-compatible deps are no longer fetched. On a system with all dev packages
present, downloads drop from 11 → 2 (PHP + zlib).
Tests
TestResolveActivePHP_*→ deterministic (split env-dependent NoPHP intoSystemFallback + NoPHP)
TestShouldDownloadDep,TestComputeMustBuild,TestSiloRepository_Extract_PreservesMtime,TestSiloRepository_ExtractXz_TempFile,TestGraphRepository_RequiresPackages_DataDriven,TestPackagesForDistro_LibPQMapping,TestInstalledExtensions_*, extension/pecl/phar version-resolution tests,TestUsePrint_*,TestBareUse_*,TestInitShell_EmitsShellFunctionmake build,go vet ./...,go test ./...all pass.