Merge TASK-044 review cleanup (27/28 fixed, 1 deferred)

etr · etr · commit bc971466eab2 · 2026-05-27T16:39:09.000-07:00
diff --git a/Makefile.am b/Makefile.am
@@ -42,6 +42,7 @@ EXTRA_DIST = libhttpserver.pc.in $(DX_CONFIG) scripts/extract-release-notes.sh s
 	scripts/check-examples.sh scripts/check-readme.sh scripts/check-release-notes.sh \
 	scripts/check-doxygen.sh scripts/check-hooks-doc-spotcheck.sh \
 	scripts/check-soversion.sh scripts/check-parallel-install.sh \
+	scripts/lib/resolve-prefix.sh \
 	scripts/check-complexity.sh scripts/check-duplication.sh \
 	scripts/check-file-size.sh \
 	scripts/verify-installed-examples.sh \
diff --git a/configure.ac b/configure.ac
@@ -323,7 +323,11 @@ DX_PDF_FEATURE(OFF)
 DX_PS_FEATURE(OFF)
 DX_INIT_DOXYGEN([$PACKAGE_NAME],[doxyconfig.in])
 
-LDFLAGS="$LDFLAGS -version-number libhttpserver_LDF_VERSION"
+# Export the libtool version-number triple so src/Makefile.am can scope it
+# to libhttpserver_la_LDFLAGS rather than injecting it into the global LDFLAGS
+# (which would affect all link steps, not just the shared library).
+LHT_LDF_VERSION="libhttpserver_LDF_VERSION"
+AC_SUBST(LHT_LDF_VERSION)
 
 AC_SUBST(LHT_LIBDEPS)
 AC_SUBST(AM_CXXFLAGS)
diff --git a/scripts/check-parallel-install.sh b/scripts/check-parallel-install.sh
@@ -6,7 +6,7 @@
 # installed side-by-side into the same prefix without their shared-library
 # artefacts colliding. The promise is on the SONAME / runtime-library side —
 # v1's `libhttpserver.so.0` and v2's `libhttpserver.so.2` (Linux) or
-# `libhttpserver.0.dylib` and `libhttpserver.2.0.0.dylib` (Darwin) MUST
+# `libhttpserver.0.dylib` and `libhttpserver.2.dylib` (Darwin) MUST
 # coexist after installing both into the same DESTDIR.
 #
 # The dev-time artefacts (libhttpserver.la, libhttpserver.a, the .pc file,
@@ -50,6 +50,8 @@
 # install is well-formed; the on-disk filenames don't clash) is verified
 # either way.
 
+# -e is intentionally omitted: every significant command uses explicit
+# '|| fail/skip' error handling for clear diagnostic messages.
 set -uo pipefail
 
 REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)"
@@ -73,48 +75,57 @@ pass() {
     echo "  PASS: $*"
 }
 
-cleanup() {
-    # Tear down the ephemeral worktree if we created one. Don't touch
-    # $SHARED_STAGE — the maintainer may want to inspect it.
+remove_master_worktree() {
+    # Tear down the ephemeral worktree if it exists.  Used by both the
+    # pre-add stale-cleanup guard and the EXIT trap.
+    # `git worktree remove --force` may exit non-zero on shallow clones;
+    # ignore errors — the directory cleanup is sufficient for re-run safety.
     if [ -d "$MASTER_WORKTREE" ]; then
-        # `git worktree remove --force` may exit non-zero on shallow clones;
-        # ignore errors — the directory cleanup is sufficient for re-run safety.
         git -C "$REPO_ROOT" worktree remove --force "$MASTER_WORKTREE" 2>/dev/null || true
         rm -rf "$MASTER_WORKTREE"
     fi
 }
+
+cleanup() {
+    # Don't touch $SHARED_STAGE — the maintainer may want to inspect it.
+    remove_master_worktree
+}
 trap cleanup EXIT
 
 [ -d "$BUILD_DIR" ] || skip "BUILD_DIR=$BUILD_DIR does not exist (run ./configure first)"
 
-# Resolve libdir from config.status (same trick as check-soversion.sh).
+# Resolve libdir from config.status via the shared helper.
 CONFIG_STATUS="$BUILD_DIR/config.status"
 [ -x "$CONFIG_STATUS" ] || skip "$CONFIG_STATUS missing (run ./configure first)"
-RESOLVED_PREFIX="$("$CONFIG_STATUS" --config 2>/dev/null \
-    | tr ' ' '\n' | grep -E "^'?--prefix=" | head -1 | sed "s/^'\?--prefix=//;s/'\$//")"
-[ -z "$RESOLVED_PREFIX" ] && RESOLVED_PREFIX="/usr/local"
+# shellcheck source=scripts/lib/resolve-prefix.sh
+source "$(dirname "$0")/lib/resolve-prefix.sh"
 LIBDIR="$RESOLVED_PREFIX/lib"
 STAGE_LIB="$SHARED_STAGE$LIBDIR"
 
 case "$PLATFORM" in
     Linux)
         V2_FULL_BASENAME="libhttpserver.so.2.0.0"
-        # v1 install on disk uses the major from configure.ac's
-        # MAJOR_VERSION — most commonly .so.0 (legacy 0.x line) or
-        # .so.1. We don't pin the v1 SONAME here; we just require
-        # *some* sibling file shaped like libhttpserver.so.<digit>.
-        V1_PATTERN='^libhttpserver\.so\.[01]'
+        # V1_SONAME_PATTERN: fallback pattern when the exact v1 SONAME cannot
+        # be read from the .la file (see Phase 3 below).  [01] covers:
+        #   .so.0 — the legacy 0.x line (master MAJOR_VERSION=0)
+        #   .so.1 — a hypothetical 1.x bump (not shipped but possible)
+        # The exact v1 SONAME is always preferred; this pattern is a safety net.
+        V1_SONAME_PATTERN='^libhttpserver\.so\.[01]'
         ;;
     Darwin)
         # libtool with -version-number on Mach-O produces only the
         # major-numbered .N.dylib (no .N.M.P.dylib intermediate).
         V2_FULL_BASENAME="libhttpserver.2.dylib"
-        V1_PATTERN='^libhttpserver\.[01]\.dylib'
+        # V1_SONAME_PATTERN: fallback pattern — matches .0.dylib (legacy 0.x)
+        # or .1.dylib (hypothetical 1.x).  Same rationale as Linux above.
+        V1_SONAME_PATTERN='^libhttpserver\.[01]\.dylib'
         ;;
     *)
         fail "unsupported platform '$PLATFORM' (need Linux or Darwin)"
         ;;
 esac
+# Alias for backward-compatibility with the fallback grep path below.
+V1_PATTERN="$V1_SONAME_PATTERN"
 
 echo "=== check-parallel-install: v2 + v1 coexistence on the same DESTDIR ==="
 echo "  BUILD_DIR       : $BUILD_DIR"
@@ -144,10 +155,7 @@ if ! git -C "$REPO_ROOT" rev-parse --verify "$MASTER_REF" >/dev/null 2>&1; then
 fi
 
 # Remove a stale worktree from a previous aborted run, if any.
-if [ -d "$MASTER_WORKTREE" ]; then
-    git -C "$REPO_ROOT" worktree remove --force "$MASTER_WORKTREE" 2>/dev/null || true
-    rm -rf "$MASTER_WORKTREE"
-fi
+remove_master_worktree
 
 if ! git -C "$REPO_ROOT" worktree add --detach "$MASTER_WORKTREE" "$MASTER_REF" >"$SHARED_STAGE/.worktree-add.log" 2>&1; then
     cat "$SHARED_STAGE/.worktree-add.log" >&2
@@ -165,17 +173,36 @@ fi
 V1_BUILD="$MASTER_WORKTREE/build"
 mkdir -p "$V1_BUILD"
 echo "  configuring $MASTER_REF"
+# Build Darwin-specific include/lib hints using the active Homebrew prefix
+# (if brew is on PATH) so that arm64 (/opt/homebrew) and x86_64 (/usr/local)
+# Homebrew installs are both handled correctly.  On Linux these variables
+# remain empty and configure falls back to its own search paths.
+_HOMEBREW_CPPFLAGS=""
+_HOMEBREW_LDFLAGS=""
+if [ "$PLATFORM" = "Darwin" ]; then
+    if command -v brew >/dev/null 2>&1; then
+        _BREW_PREFIX="$(brew --prefix 2>/dev/null || true)"
+    else
+        _BREW_PREFIX="/opt/homebrew"   # best-effort fallback for arm64
+    fi
+    if [ -n "$_BREW_PREFIX" ]; then
+        _HOMEBREW_CPPFLAGS="-I${_BREW_PREFIX}/include -I${_BREW_PREFIX}/opt/gnutls/include"
+        _HOMEBREW_LDFLAGS="-L${_BREW_PREFIX}/lib -L${_BREW_PREFIX}/opt/gnutls/lib"
+    fi
+fi
 if ! ( cd "$V1_BUILD" && \
-        CPPFLAGS="${CPPFLAGS:-} -I/opt/homebrew/include -I/opt/homebrew/opt/gnutls/include" \
-        LDFLAGS="${LDFLAGS:-} -L/opt/homebrew/lib -L/opt/homebrew/opt/gnutls/lib" \
+        CPPFLAGS="${CPPFLAGS:-} ${_HOMEBREW_CPPFLAGS}" \
+        LDFLAGS="${LDFLAGS:-} ${_HOMEBREW_LDFLAGS}" \
         LIBS="${LIBS:--lgnutls}" \
         ../configure --prefix="$RESOLVED_PREFIX" ) >"$SHARED_STAGE/.v1-configure.log" 2>&1; then
     tail -20 "$SHARED_STAGE/.v1-configure.log" >&2
     skip "v1 configure failed (treating as environment limitation)"
 fi
 
-echo "  building $MASTER_REF"
-if ! ( cd "$V1_BUILD" && make -j4 ) >"$SHARED_STAGE/.v1-make.log" 2>&1; then
+# Use all available cores; mirror what the parent make would use.
+_NJOBS="$(nproc 2>/dev/null || sysctl -n hw.logicalcpu 2>/dev/null || echo 4)"
+echo "  building $MASTER_REF (make -j${_NJOBS})"
+if ! ( cd "$V1_BUILD" && make -j"${_NJOBS}" ) >"$SHARED_STAGE/.v1-make.log" 2>&1; then
     tail -30 "$SHARED_STAGE/.v1-make.log" >&2
     skip "v1 make failed (treating as environment limitation; e.g. v1 sources may not build on this toolchain)"
 fi
@@ -191,17 +218,42 @@ pass "v1 ($MASTER_REF) install succeeded into shared stage"
 [ -f "$STAGE_LIB/$V2_FULL_BASENAME" ] \
     || fail "v2 library $V2_FULL_BASENAME disappeared from $STAGE_LIB after v1 install"
 
-# Look for any v1-style sibling that is NOT the v2 file we just confirmed.
-v1_hits="$(ls "$STAGE_LIB" 2>/dev/null | grep -E "$V1_PATTERN" | grep -v "^$V2_FULL_BASENAME\$" || true)"
-if [ -z "$v1_hits" ]; then
-    fail "no v1-style library file (matching $V1_PATTERN, excluding $V2_FULL_BASENAME) found in $STAGE_LIB; install contents: $(ls "$STAGE_LIB" 2>/dev/null)"
+# Derive the expected v1 runtime library basename from the v1 build's .la
+# file (the `library_names` field), falling back to the broad V1_PATTERN
+# grep when the .la is absent (older libtool layouts).
+V1_LA="$V1_BUILD/.libs/libhttpserver.la"
+V1_EXPECTED_BASENAME=""
+if [ -f "$V1_LA" ]; then
+    # `library_names='libhttpserver.so.0.x.y libhttpserver.so.0 libhttpserver.so'`
+    # The last token in the library_names value is the dev link; the first is
+    # the versioned runtime file.  Extract the first token.
+    _raw="$(grep '^library_names=' "$V1_LA" | head -1 | sed "s/^library_names='*//;s/'*$//")"
+    V1_EXPECTED_BASENAME="${_raw%% *}"   # first whitespace-delimited token
 fi
 
-echo "  v1 artefacts coexisting with v2:"
-for f in $v1_hits; do
-    echo "    $f"
-done
-pass "v1 and v2 SONAMEd libraries coexist in $STAGE_LIB"
+if [ -n "$V1_EXPECTED_BASENAME" ]; then
+    # Strict check: the exact filename must appear in $STAGE_LIB and must
+    # differ from V2_FULL_BASENAME (ensures they coexist, not just that v2
+    # is present).
+    if [ "$V1_EXPECTED_BASENAME" = "$V2_FULL_BASENAME" ]; then
+        fail "v1 and v2 appear to have the SAME versioned filename ($V1_EXPECTED_BASENAME); SONAME collision detected"
+    fi
+    [ -f "$STAGE_LIB/$V1_EXPECTED_BASENAME" ] \
+        || fail "expected v1 library '$V1_EXPECTED_BASENAME' (from $V1_LA) not found in $STAGE_LIB; install contents: $(ls "$STAGE_LIB" 2>/dev/null)"
+    echo "  v1 artefact coexisting with v2: $V1_EXPECTED_BASENAME"
+    pass "v1 ($V1_EXPECTED_BASENAME) and v2 ($V2_FULL_BASENAME) SONAMEd libraries coexist in $STAGE_LIB"
+else
+    # Fallback: broad pattern-match (V1_PATTERN covers .so.0, .so.1, etc.).
+    v1_hits="$(ls "$STAGE_LIB" 2>/dev/null | grep -E "$V1_PATTERN" | grep -v "^$V2_FULL_BASENAME\$" || true)"
+    if [ -z "$v1_hits" ]; then
+        fail "no v1-style library file (matching $V1_PATTERN, excluding $V2_FULL_BASENAME) found in $STAGE_LIB; install contents: $(ls "$STAGE_LIB" 2>/dev/null)"
+    fi
+    echo "  v1 artefacts coexisting with v2:"
+    while IFS= read -r f; do
+        echo "    $f"
+    done <<< "$v1_hits"
+    pass "v1 and v2 SONAMEd libraries coexist in $STAGE_LIB"
+fi
 
 echo "  Note: dev-time files (libhttpserver.la, libhttpserver.a, libhttpserver.pc,"
 echo "        headers, the bare libhttpserver.so/.dylib dev symlink) are LAST-WRITER-WINS"
diff --git a/scripts/check-soversion.sh b/scripts/check-soversion.sh
@@ -41,6 +41,10 @@
 # Exits non-zero on the first violation, with a clear FAIL line that names
 # the failing assertion.
 
+# -e is intentionally omitted: every significant command uses explicit
+# '|| fail ...' error handling so that failures produce a clear, named
+# assertion message rather than a silent non-zero exit.  Adding -e would
+# break the 'true'-guarded grep calls and confuse the intentional design.
 set -uo pipefail
 
 REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)"
@@ -57,6 +61,23 @@ pass() {
     echo "  PASS: $*"
 }
 
+# Portably resolve a chain of symlinks to the final real path.
+# Used instead of `readlink -f` which is not available on all platforms.
+resolve_symlink_chain() {
+    local p="$1"
+    local d
+    local target
+    while [ -L "$p" ]; do
+        target="$(readlink "$p")"
+        d="$(dirname "$p")"
+        case "$target" in
+            /*) p="$target" ;;
+            *)  p="$d/$target" ;;
+        esac
+    done
+    printf '%s\n' "$p"
+}
+
 # Detect the configured libdir/includedir from libtool's view of the build.
 # The Makefile substitutes ${prefix}, so we fish them out of config.status
 # (the canonical record of `./configure`'s decisions).
@@ -69,11 +90,9 @@ if [ ! -x "$CONFIG_STATUS" ]; then
     fail "$CONFIG_STATUS not found; run ./configure in $BUILD_DIR first"
 fi
 
-# Resolve prefix/libdir/includedir by asking config.status. These come back
-# already expanded (no ${prefix} placeholders), suitable for path use.
-RESOLVED_PREFIX="$("$CONFIG_STATUS" --config 2>/dev/null \
-    | tr ' ' '\n' | grep -E "^'?--prefix=" | head -1 | sed "s/^'\?--prefix=//;s/'\$//")"
-[ -z "$RESOLVED_PREFIX" ] && RESOLVED_PREFIX="/usr/local"
+# Resolve prefix by asking config.status (shared helper; see scripts/lib/).
+# shellcheck source=scripts/lib/resolve-prefix.sh
+source "$(dirname "$0")/lib/resolve-prefix.sh"
 
 # libdir & includedir default to $prefix/lib and $prefix/include for our
 # configure setup; configure.ac doesn't override AC_PROG_LIBTOOL defaults.
@@ -89,8 +108,13 @@ echo "  libdir    : $LIBDIR"
 # ---- A1: clean install --------------------------------------------------------
 rm -rf "$STAGE"
 mkdir -p "$STAGE"
-( cd "$BUILD_DIR" && make install DESTDIR="$STAGE" ) >"$STAGE/.install.log" 2>&1 \
-    || { echo "---- install log (tail) ----" >&2; tail -40 "$STAGE/.install.log" >&2; fail "A1: 'make install DESTDIR=$STAGE' failed"; }
+# Use tee so CI consoles see streaming progress while the log is also captured.
+( cd "$BUILD_DIR" && make install DESTDIR="$STAGE" ) 2>&1 | tee "$STAGE/.install.log"
+if [ "${PIPESTATUS[0]}" -ne 0 ]; then
+    echo "---- install log (tail) ----" >&2
+    tail -40 "$STAGE/.install.log" >&2
+    fail "A1: 'make install DESTDIR=$STAGE' failed"
+fi
 pass "A1: staged install succeeded"
 
 STAGE_LIB="$STAGE$LIBDIR"
@@ -143,22 +167,7 @@ fi
 
 # A4: dev symlink — must resolve (possibly through SONAME_LINK) to FULL.
 [ -L "$DEV_LINK" ] || fail "A4: expected symlink at $DEV_LINK"
-# Use shell readlink + manual chase; some systems lack `readlink -f`.
-chase_symlink() {
-    local p="$1"
-    local d
-    local target
-    while [ -L "$p" ]; do
-        target="$(readlink "$p")"
-        d="$(dirname "$p")"
-        case "$target" in
-            /*) p="$target" ;;
-            *)  p="$d/$target" ;;
-        esac
-    done
-    printf '%s\n' "$p"
-}
-DEV_RESOLVED="$(chase_symlink "$DEV_LINK")"
+DEV_RESOLVED="$(resolve_symlink_chain "$DEV_LINK")"
 if [ "$DEV_RESOLVED" != "$FULL" ]; then
     fail "A4: $DEV_LINK ultimately resolves to '$DEV_RESOLVED', expected '$FULL'"
 fi
@@ -172,14 +181,14 @@ case "$PLATFORM" in
             if [ -z "$soname_line" ]; then
                 fail "A5: readelf -d $FULL produced no SONAME line"
             fi
-            case "$soname_line" in
-                *"[$SONAME_BASENAME]"*|*"$SONAME_BASENAME"*)
-                    pass "A5: ELF SONAME = $SONAME_BASENAME"
-                    ;;
-                *)
-                    fail "A5: ELF SONAME mismatch — got: $soname_line"
-                    ;;
-            esac
+            # Extract the bracketed SONAME value for a strict equality check.
+            # readelf format: (SONAME)  Library soname: [libhttpserver.so.2]
+            extracted_soname="$(printf '%s\n' "$soname_line" | sed 's/.*\[\(.*\)\].*/\1/')"
+            if [ "$extracted_soname" = "$SONAME_BASENAME" ]; then
+                pass "A5: ELF SONAME = $SONAME_BASENAME"
+            else
+                fail "A5: ELF SONAME mismatch — got '$extracted_soname', expected '$SONAME_BASENAME'"
+            fi
         else
             echo "  SKIP A5: readelf not on PATH (filename-only verification accepted)"
         fi
@@ -243,11 +252,12 @@ fi
 # ---- A7: libtool .la --------------------------------------------------------
 LA_FILE="$STAGE_LIB/libhttpserver.la"
 [ -f "$LA_FILE" ] || fail "A7: libtool control file missing: $LA_FILE"
-# library_names should list the SONAME symlink (Linux) or 2.dylib (Darwin)
-if ! grep -q "$SONAME_BASENAME" "$LA_FILE"; then
-    fail "A7: $LA_FILE does not mention $SONAME_BASENAME"
+# Verify the SONAME appears specifically in the library_names= field, not just
+# anywhere in the file (e.g. not in a comment or dlname-only reference).
+if ! grep -E "^library_names=.*$SONAME_BASENAME" "$LA_FILE" >/dev/null 2>&1; then
+    fail "A7: $LA_FILE library_names= field does not reference $SONAME_BASENAME"
 fi
-pass "A7: $(basename "$LA_FILE") references $SONAME_BASENAME"
+pass "A7: $(basename "$LA_FILE") library_names references $SONAME_BASENAME"
 
 # ---- A8: static archive -----------------------------------------------------
 STATIC="$STAGE_LIB/libhttpserver.a"
diff --git a/scripts/lib/resolve-prefix.sh b/scripts/lib/resolve-prefix.sh
@@ -0,0 +1,41 @@
+#!/usr/bin/env bash
+#
+# resolve-prefix.sh — shared helper for check-soversion.sh and
+# check-parallel-install.sh.
+#
+# Sourced (not executed) by both gate scripts to avoid copy-pasting the
+# config.status prefix-resolution pipeline in two places.
+#
+# Inputs (must be set by the caller before sourcing):
+#   CONFIG_STATUS — absolute path to the config.status file in BUILD_DIR.
+#   fail          — a shell function that prints a message and exits non-zero.
+#
+# Outputs (set in the caller's environment):
+#   RESOLVED_PREFIX — the configured installation prefix (e.g. /usr/local).
+#
+# After sourcing, callers typically derive:
+#   LIBDIR="$RESOLVED_PREFIX/lib"
+#   INCDIR="$RESOLVED_PREFIX/include"
+#
+# Design note: this helper is intentionally kept standalone (no sourcing of
+# further helpers) so that each gate script remains easy to invoke in
+# isolation without a complex dependency chain.
+
+# Resolve prefix by asking config.status.  The --config flag prints the
+# original configure arguments, one per line after splitting on spaces.  We
+# grep for the --prefix= token, strip surrounding single-quotes (an autoconf
+# quoting convention), and fall back to /usr/local when absent.
+#
+# POSIX-portable sed is used: avoid GNU \? (undefined in POSIX BRE/ERE).
+RESOLVED_PREFIX="$("$CONFIG_STATUS" --config 2>/dev/null \
+    | tr ' ' '\n' \
+    | grep -E "^'?--prefix=" \
+    | head -1 \
+    | sed "s/^'*--prefix=//;s/'*$//")"
+[ -z "$RESOLVED_PREFIX" ] && RESOLVED_PREFIX="/usr/local"
+
+# Guard: an absolute path is required for safe use in rm -rf / mkdir -p.
+case "$RESOLVED_PREFIX" in
+    /*) ;;
+    *) fail "resolved prefix is not absolute: $RESOLVED_PREFIX" ;;
+esac
diff --git a/specs/tasks/_index.md b/specs/tasks/_index.md
@@ -1,7 +1,7 @@
 # libhttpserver v2.0 — Task Plan
 
 **Status:** Draft 1
-**Last updated:** 2026-05-18
+**Last updated:** 2026-05-20
 **Owner:** Sebastiano Merlino
 **Inputs:** [specs/product_specs.md](../product_specs.md), [specs/architecture/](../architecture/)
 
diff --git a/specs/unworked_review_issues/2026-05-20_115013_task-044.md b/specs/unworked_review_issues/2026-05-20_115013_task-044.md
diff --git a/src/Makefile.am b/src/Makefile.am
