From be8ef5b23cf62dfec01cbdf30425a1c279ffd13e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marcos=20M=C3=A9ndez?= <mendez.foto@gmail.com>
Date: Sun, 27 Sep 2026 03:11:09 +0000
Subject: [PATCH] Apply the units before the common removelists, and let a
 build select them

A unit is a component: a directory under unit.d with a plan, an overlay and
a conf script, resolved and applied by this file. Two things about the loop
that applies them were wrong for a component.

It ran last, after the common removelists and after the product-local
overlay. So a common removelist could not remove a file a unit brought in,
the way it can for a common overlay, and a recipe's own overlay no longer
won over the component's, which is the precedence the recipes are written
for. The loop now runs where a component belongs: after the common overlays,
the common conf scripts and the common patches, and before the common
removelists. Every unit overlay is applied before any unit conf script, the
same two phases the common overlays and conf scripts go through, so a recipe
composing several units does not depend on which one fab reaches first.

It also applied every unit under unit.d with no way to leave one out. A
build layered on a parent image must not re-run the conf script of a
component the parent already applied: a conf script runs once, and mysql's
does ln -s /etc/init.d/mysql under bash -e, which fails the second time. The
new UNITS variable holds the units to apply and defaults to all of them, so
a layered build overrides it with the difference. The plans are resolved
from unit.d regardless of UNITS, because a plan only names packages and a
package the parent carries resolves to the version that is there.

Each command in the two loops gets || exit. Without it a phase is one shell
line per build, so a unit that fails in the middle of it was swallowed and
the image shipped half applied.

tests/units.sh builds a fake product with three units against this
product.mk with the fab tools stubbed, and asserts on what they were called
with and in which order: 38 checks, and tests/coverage.sh now counts them
together with the SOURCE_DATE_EPOCH suite, 57 of 57.
---
 COVERAGE.md       |  19 +++
 share/product.mk  |  39 ++++--
 tests/coverage.sh |  30 +++--
 tests/units.sh    | 310 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 380 insertions(+), 18 deletions(-)
 create mode 100755 tests/units.sh

diff --git a/COVERAGE.md b/COVERAGE.md
index 4d51d81..aff62f3 100644
--- a/COVERAGE.md
+++ b/COVERAGE.md
@@ -12,6 +12,25 @@ Pull request #1 merged on 2026-09-26 (merge commit e79be43) and brought
 down, and is only ever raised. The sections that follow record the state
 before the merge.
 
+## 2026-09-27: the unit loop and where it runs
+
+`tests/coverage.sh` now runs two suites and counts the checks of both:
+
+| Suite | Checks | What it measures |
+|-------|--------|------------------|
+| tests/source-date-epoch.sh | 19 of 19 | the `SOURCE_DATE_EPOCH` handling |
+| tests/units.sh | 38 of 38 | the unit loop, `UNITS` and the position of the units in `root.patched` |
+
+Total 57 of 57, 100 percent. The gate stays at 100.
+
+`tests/units.sh` does not dry run. It builds a fake product with three
+units against this checkout's `product.mk` with the four fab tools replaced
+by stubs that log their arguments, and asserts on the log: what was
+applied, in which order, and what a failing tool does. That is why it can
+tell that a unit whose `conf` is not executable is skipped, which a
+`make -n` of the same recipe cannot, since the `[ -x ]` guard is resolved
+by the shell and not by make.
+
 ## Baseline before the merge: 0 percent, nothing measured
 
 `tests/` holds `regtest.sh` (77 lines), `override.sh`, `parseopts.py` and
diff --git a/share/product.mk b/share/product.mk
index c89c5d3..1e28c85 100644
--- a/share/product.mk
+++ b/share/product.mk
@@ -104,6 +104,14 @@ ifeq ($(wildcard $(REMOVELIST)),)
 endif
 
 UNIT_DIRS ?= unit.d
+
+# The units this build applies, as directory paths. Every unit under
+# UNIT_DIRS by default. A build layered on a parent image overrides it with
+# the units the parent has not applied yet, because a unit's conf script
+# runs once and is not written to run twice; the plans of every unit are
+# still resolved (root.spec below), since the plan only names packages.
+UNITS ?= $(sort $(wildcard $(UNIT_DIRS)/*))
+
 CONF_SCRIPTS ?= conf.d
 PATCHES_PATH ?= patches.d
 
@@ -191,6 +199,7 @@ define help/body
 	@echo '  PLAN                       $(value PLAN)'
 	@echo '  REMOVELIST                 $(value REMOVELIST)'
 	@echo '  UNIT_DIRS                  $(value UNIT_DIRS)/'
+	@echo '  UNITS                      $(value UNITS)'
 	@echo '  ROOT_OVERLAY               $(value ROOT_OVERLAY)/'
 	@echo '  CONF_SCRIPTS               $(value CONF_SCRIPTS)/'
 	@echo '  PATCHES_PATH               $(value PATCHES_PATH)/'
@@ -282,6 +291,10 @@ endef
 
 # target: root.spec
 root.spec/deps ?= $(STAMPS_DIR)/bootstrap $(wildcard plan/*)
+# every unit's plan is resolved, including the plans of the units UNITS
+# leaves out: a plan names packages, and a package the parent image already
+# carries resolves to the version that is there rather than being installed
+# a second time
 define root.spec/body
 	unit_plans="$(wildcard $(UNIT_DIRS)/*/plan)"; \
 	fab-plan-resolve $(PLAN) $$unit_plans $(EXTRA_PLAN) --bootstrap=$(BOOTSTRAP) --output=$O/root.spec $(foreach var,$(_CONF_VARS_BUILTIN),-D '$(var)=$($(var))')
@@ -375,23 +388,31 @@ define root.patched/body
 	  fi
 	  )
 
-	# apply the common removelists
-	$(foreach removelist,$(_COMMON_REMOVELISTS),
-	  fab-apply-removelist $(removelist) $O/root.patched; \
-	  )
-
-	# apply the product-local units
-	$(foreach unit,$(wildcard $(UNIT_DIRS)/*), \
+	# apply the unit overlays, in unit name order and all of them before any
+	# unit conf script, the order the common overlays and the common conf
+	# scripts above are applied in: a unit is a component of the same kind,
+	# and a recipe that composes several must not depend on which one of
+	# them fab happens to reach first
+	$(foreach unit,$(UNITS), \
 	  if [ -d $(unit)/overlay ]; then \
 		echo fab-apply-overlay $(unit)/overlay $O/root.patched; \
-		fab-apply-overlay $(unit)/overlay $O/root.patched; \
+		fab-apply-overlay $(unit)/overlay $O/root.patched || exit; \
 	  fi; \
+	  )
+
+	# run the unit configuration scripts
+	$(foreach unit,$(UNITS), \
 	  if [ -x $(unit)/conf ]; then \
 		echo fab-chroot $O/root.patched --script $(unit)/conf; \
-		fab-chroot $O/root.patched --script $(unit)/conf; \
+		fab-chroot $O/root.patched --script $(unit)/conf || exit; \
 	  fi; \
 	  )
 
+	# apply the common removelists, after the units, so that a removelist
+	# can remove a file a unit brought in
+	$(foreach removelist,$(_COMMON_REMOVELISTS),
+	  fab-apply-removelist $(removelist) $O/root.patched; \
+	  )
 
 	# apply the product-local root overlay
 	if [ -d $(ROOT_OVERLAY) ]; then \
diff --git a/tests/coverage.sh b/tests/coverage.sh
index 9c62427..80e006b 100755
--- a/tests/coverage.sh
+++ b/tests/coverage.sh
@@ -1,10 +1,14 @@
 #!/bin/bash
-# Coverage of the SOURCE_DATE_EPOCH code in share/product.mk.
+# Coverage of the project-authored code in share/product.mk:
+#   tests/source-date-epoch.sh   the SOURCE_DATE_EPOCH handling
+#   tests/units.sh               the unit loop and UNITS
 #
 # make has no line coverage tool, so the number here is the share of the
-# checks in tests/source-date-epoch.sh that pass: one check per effect of
-# the touched code (both arms of the ifneq, both xorriso recipes, isohybrid,
-# FAB_CHROOT_ENV, fab-plan-resolve and the untouched mksquashfs options).
+# checks in those scripts that pass: one check per effect of the touched
+# code (both arms of the ifneq, both xorriso recipes, isohybrid,
+# FAB_CHROOT_ENV, fab-plan-resolve and the untouched mksquashfs options for
+# the first; each unit input, each position in root.patched, each value of
+# UNITS and each failure path for the second).
 # Exits 1 when that share is below the threshold (default 95).
 #
 #   tests/coverage.sh [THRESHOLD]     (or COVERAGE_THRESHOLD in the environment)
@@ -13,13 +17,21 @@ set -euo pipefail
 here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 threshold="${1:-${COVERAGE_THRESHOLD:-95}}"
 
-tap="$("$here/source-date-epoch.sh" || true)"
-printf '%s\n' "$tap"
-total="$(grep -cE '^(not )?ok [0-9]+ ' <<<"$tap")"
-passed="$(grep -cE '^ok [0-9]+ ' <<<"$tap")"
+suites=(source-date-epoch units)
+
+total=0
+passed=0
+for suite in "${suites[@]}"; do
+    echo "# $suite"
+    tap="$("$here/$suite.sh" || true)"
+    printf '%s\n' "$tap"
+    total=$((total + $(grep -cE '^(not )?ok [0-9]+ ' <<<"$tap")))
+    passed=$((passed + $(grep -cE '^ok [0-9]+ ' <<<"$tap")))
+done
+
 percent="$(awk -v p="$passed" -v t="$total" 'BEGIN { printf "%.2f", (t ? 100 * p / t : 0) }')"
 
-echo "product.mk SOURCE_DATE_EPOCH: $percent percent ($passed of $total checks) covered, threshold $threshold"
+echo "product.mk: $percent percent ($passed of $total checks) covered, threshold $threshold"
 if ! awk -v p="$percent" -v t="$threshold" 'BEGIN { exit !(p + 0 >= t + 0) }'; then
     echo "coverage below threshold" >&2
     exit 1
diff --git a/tests/units.sh b/tests/units.sh
new file mode 100755
index 0000000..1c1d4b8
--- /dev/null
+++ b/tests/units.sh
@@ -0,0 +1,310 @@
+#!/bin/bash
+# make-level test for the unit loop of share/product.mk: which units a build
+# applies (UNITS) and where in root.patched they are applied.
+#
+# A minimal fake product with three units is built under a scratch FAB_PATH
+# against the product.mk of this checkout. The four fab tools the recipe
+# calls (fab-apply-overlay, fab-chroot, fab-apply-removelist,
+# fab-plan-resolve) are stubs that append their arguments to a log, and the
+# init step that would deck a real filesystem is overridden away, so the
+# recipe runs for real and the log says what it did and in which order.
+# Output is TAP; the exit status is the number of failed checks.
+#
+# product.mk includes /tmp/.build_env, written by share/load_env; the test
+# runs load_env from the fake product, so that file is overwritten for the
+# duration of the run and restored afterwards. Nothing else outside the
+# scratch directory is touched and no real fab tool is executed.
+#
+#   tests/units.sh
+
+set -uo pipefail
+
+here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+share="$(cd "$here/.." && pwd)/share"
+work="$(mktemp -d)"
+log="$work/calls.log"
+
+restore_build_env() {
+    if [[ -e "$work/build_env.saved" ]]; then
+        cp -p "$work/build_env.saved" /tmp/.build_env
+    else
+        rm -f /tmp/.build_env
+    fi
+}
+cleanup() {
+    restore_build_env
+    rm -rf "$work"
+}
+trap cleanup EXIT
+[[ ! -e /tmp/.build_env ]] || cp -p /tmp/.build_env "$work/build_env.saved"
+
+# --- stub fab tools -----------------------------------------------------------
+
+mkdir -p "$work/stubs"
+for tool in fab-apply-overlay fab-chroot fab-apply-removelist fab-plan-resolve \
+        fab-apply-patch; do
+    cat > "$work/stubs/$tool" <<EOF
+#!/bin/sh
+echo "$tool \$*" >> "$log"
+if [ -n "\${STUB_FAIL:-}" ] && echo "$tool \$*" | grep -qE "\$STUB_FAIL"; then
+    echo "$tool: refusing on purpose" >&2
+    exit 1
+fi
+exit 0
+EOF
+    chmod 755 "$work/stubs/$tool"
+done
+export PATH="$work/stubs:$PATH"
+
+# --- fake product with three units ---------------------------------------------
+
+export FAB_PATH="$work/fab"
+export FAB_SHARE_PATH="$share"
+export RELEASE=debian/trixie
+export FAB_ARCH=amd64
+export NO_PROXY=true
+unset SOURCE_DATE_EPOCH BOOTSTRAP O ISOLABEL CONF_VARS CHROOT_ONLY UNITS STUB_FAIL
+
+product="$work/fakeprod"
+mkdir -p "$FAB_PATH/bootstraps/trixie-amd64" \
+    "$FAB_PATH/common/overlays/shared" \
+    "$FAB_PATH/common/conf" \
+    "$FAB_PATH/common/removelists" \
+    "$product/plan" "$product/overlay"
+: > "$product/plan/main"
+: > "$FAB_PATH/common/conf/shared"
+: > "$FAB_PATH/common/removelists/shared"
+
+# unit "alpha" carries all of plan, overlay and conf; "beta" carries a plan
+# and an overlay only; "gamma" carries a conf script that is not executable
+for unit in alpha beta gamma; do
+    mkdir -p "$product/unit.d/$unit/overlay"
+    : > "$product/unit.d/$unit/plan"
+done
+printf '#!/bin/sh\ntrue\n' > "$product/unit.d/alpha/conf"
+chmod 755 "$product/unit.d/alpha/conf"
+printf '#!/bin/sh\ntrue\n' > "$product/unit.d/gamma/conf"
+chmod 644 "$product/unit.d/gamma/conf"
+
+cat > "$product/Makefile" <<'EOF'
+COMMON_OVERLAYS = shared
+COMMON_CONF = shared
+COMMON_REMOVELISTS = shared
+include $(FAB_SHARE_PATH)/product.mk
+EOF
+
+# The stamped rule is <init> <pre> <body> <post> <cleanup>; only the body is
+# under test, and root.patched/init would deck a real filesystem.
+neutral=(
+    root.patched/init=
+    root.patched/deps=
+    root.spec/deps=
+)
+
+# build TARGET [VAR=VALUE ...]: run the recipe, leaving its calls in $log
+build() {
+    rm -f "$log"
+    rm -rf "$product/build"
+    (
+        cd "$product" || exit 1
+        "$share/load_env" 2>/dev/null
+        make "$@" "${neutral[@]}" 2>&1
+    ) > "$work/make.out"
+    local rc=$?
+    touch "$log"
+    return $rc
+}
+
+# make_var VAR: the value make computes for VAR
+make_var() {
+    (
+        cd "$product" || exit 1
+        "$share/load_env" 2>/dev/null
+        make debug V="$1" 2>&1 >/dev/null
+    ) | sed -n "s|^.*: $1 = ||p" | head -n 1
+}
+
+# --- TAP helpers ------------------------------------------------------------
+
+count=0
+failed=0
+
+ok() {
+    count=$((count + 1))
+    echo "ok $count - $1"
+}
+
+not_ok() {
+    count=$((count + 1))
+    failed=$((failed + 1))
+    echo "not ok $count - $1"
+    printf '%s\n' "$2" | sed 's/^/# /'
+}
+
+is() {
+    if [[ "$2" == "$3" ]]; then
+        ok "$1"
+    else
+        not_ok "$1" "got:  $2
+want: $3"
+    fi
+}
+
+# builds DESC TARGET [VAR=VALUE ...]: the recipe runs to the end
+builds() {
+    local desc=$1
+    shift
+    if build "$@"; then
+        ok "$desc"
+    else
+        not_ok "$desc" "$(cat "$work/make.out")"
+    fi
+}
+
+# build_fails DESC TARGET [VAR=VALUE ...]: the recipe stops with an error
+build_fails() {
+    local desc=$1
+    shift
+    if build "$@"; then
+        not_ok "$desc" "make returned 0
+$(cat "$work/make.out")"
+    else
+        ok "$desc"
+    fi
+}
+
+# called DESC REGEX: the log has a line matching REGEX
+called() {
+    if grep -qE -e "$2" "$log"; then
+        ok "$1"
+    else
+        not_ok "$1" "no call matches: $2
+$(cat "$log")"
+    fi
+}
+
+# not_called DESC REGEX: no line of the log matches REGEX
+not_called() {
+    if grep -qE -e "$2" "$log"; then
+        not_ok "$1" "$(grep -E -e "$2" "$log")"
+    else
+        ok "$1"
+    fi
+}
+
+# before DESC REGEX_A REGEX_B: the first call matching A precedes the first
+# matching B, and both are there
+before() {
+    local a b
+    a=$(grep -nE -e "$2" "$log" | head -n 1 | cut -d : -f 1)
+    b=$(grep -nE -e "$3" "$log" | head -n 1 | cut -d : -f 1)
+    if [[ -z "$a" || -z "$b" ]]; then
+        not_ok "$1" "not both called: $2 and $3
+$(cat "$log")"
+    elif [[ "$a" -lt "$b" ]]; then
+        ok "$1"
+    else
+        not_ok "$1" "line $a is not before line $b
+$(cat "$log")"
+    fi
+}
+
+alpha_overlay='^fab-apply-overlay unit\.d/alpha/overlay build/root\.patched$'
+beta_overlay='^fab-apply-overlay unit\.d/beta/overlay build/root\.patched$'
+gamma_overlay='^fab-apply-overlay unit\.d/gamma/overlay build/root\.patched$'
+alpha_conf='^fab-chroot build/root\.patched --script unit\.d/alpha/conf$'
+common_overlay='^fab-apply-overlay .*/common/overlays/shared '
+common_conf='^fab-chroot build/root\.patched --script .*/common/conf/shared$'
+common_removelist='^fab-apply-removelist .*/common/removelists/shared '
+local_overlay='^fab-apply-overlay overlay build/root\.patched$'
+
+# --- every unit is applied by default -----------------------------------------
+
+is "UNITS defaults to every unit directory, sorted" \
+    "$(make_var UNITS)" "unit.d/alpha unit.d/beta unit.d/gamma"
+
+builds "the default build succeeds" build/stamps/root.patched
+called "the overlay of a unit is applied" "$alpha_overlay"
+called "the conf script of a unit is run" "$alpha_conf"
+called "a unit without a conf script contributes its overlay" "$beta_overlay"
+not_called "a unit conf script that is not executable is not run" \
+    'script unit\.d/gamma/conf'
+called "a unit whose conf is not executable still contributes its overlay" \
+    "$gamma_overlay"
+
+# --- the position in root.patched ---------------------------------------------
+
+before "unit overlays come after the common overlays" \
+    "$common_overlay" "$alpha_overlay"
+before "unit overlays come after the common conf scripts" \
+    "$common_conf" "$alpha_overlay"
+before "every unit overlay comes before any unit conf script" \
+    "$gamma_overlay" "$alpha_conf"
+before "unit conf scripts come before the common removelists" \
+    "$alpha_conf" "$common_removelist"
+before "unit overlays come before the product-local overlay" \
+    "$alpha_overlay" "$local_overlay"
+before "the common removelists come before the product-local overlay" \
+    "$common_removelist" "$local_overlay"
+
+# --- a failing unit stops the build ---------------------------------------------
+# Without the "|| exit" the loop is one shell line per phase, so a unit that
+# fails in the middle of it is swallowed and the layer ships half applied.
+
+export STUB_FAIL='overlay unit\.d/alpha/overlay'
+build_fails "a unit overlay that fails stops the build" \
+    build/stamps/root.patched
+not_called "nothing after a failed unit overlay runs" "$local_overlay"
+
+export STUB_FAIL='script unit\.d/alpha/conf'
+build_fails "a unit conf script that fails stops the build" \
+    build/stamps/root.patched
+not_called "nothing after a failed unit conf script runs" "$local_overlay"
+unset STUB_FAIL
+
+# --- a layered build applies only the units the parent has not applied ---------
+
+builds "a build with a selected unit succeeds" \
+    build/stamps/root.patched UNITS=unit.d/beta
+called "a selected unit is applied" "$beta_overlay"
+not_called "a unit left out of UNITS is not applied" 'unit\.d/alpha/overlay'
+not_called "the conf script of a unit left out of UNITS is not run" \
+    'unit\.d/alpha/conf'
+called "the common removelists are applied with a selected unit" \
+    "$common_removelist"
+
+builds "a build with no unit selected succeeds" build/stamps/root.patched UNITS=
+not_called "an empty UNITS applies no unit overlay" 'unit\.d/[a-z]+/overlay'
+not_called "an empty UNITS runs no unit conf script" 'unit\.d/[a-z]+/conf'
+called "an empty UNITS still applies the common removelists" \
+    "$common_removelist"
+called "an empty UNITS still applies the product-local overlay" "$local_overlay"
+
+# --- the plan of every unit is resolved, whatever UNITS says --------------------
+
+builds "root.spec is resolved with every unit" build/stamps/root.spec
+called "every unit plan reaches fab-plan-resolve" \
+    'fab-plan-resolve plan/main unit\.d/alpha/plan unit\.d/beta/plan unit\.d/gamma/plan '
+
+builds "root.spec is resolved with one unit selected" \
+    build/stamps/root.spec UNITS=unit.d/beta
+called "the plan of a unit left out of UNITS is still resolved" \
+    'fab-plan-resolve plan/main unit\.d/alpha/plan unit\.d/beta/plan unit\.d/gamma/plan '
+
+builds "root.spec is resolved with no unit selected" \
+    build/stamps/root.spec UNITS=
+called "an empty UNITS still resolves every unit plan" \
+    'fab-plan-resolve plan/main unit\.d/alpha/plan unit\.d/beta/plan unit\.d/gamma/plan '
+
+# --- a product without units ----------------------------------------------------
+
+rm -rf "$product/unit.d"
+is "UNITS is empty without a unit.d" "$(make_var UNITS)" ""
+builds "a product without unit.d builds" build/stamps/root.patched
+not_called "a product without unit.d applies no unit" 'unit\.d'
+called "a product without unit.d applies the common overlays" "$common_overlay"
+called "a product without unit.d applies the common removelists" \
+    "$common_removelist"
+
+echo "1..$count"
+exit "$failed"