diff --git a/CLAUDE.md b/CLAUDE.md index 1e13508..c98034d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -59,6 +59,20 @@ state transitions, and a silent step is killed by the runner's inactivity timeout. The poll loop prints a heartbeat every interval; that is load-bearing, not decoration. +## The verdict comes from the chroots, not just the build + +A build-level `succeeded` says the build finished, not that every chroot +produced an RPM. The script polls `build-chroot/list` and requires each chroot +to reach `succeeded` (or `skipped`), so one release failing while the others +pass still fails the job. It also keeps waiting while any chroot is unfinished, +which the build-level state alone would let it skip past. + +`copr-cli` has no per-chroot status subcommand, hence the direct API call via +`python3` — which `copr-cli` already depends on, so it is not a new +requirement. Keep the "unfinished is not failed" distinction: reporting a +still-running chroot as a failure is the same mistake this action has now made +in three different guises. + ## Consumers should pin an immutable tag The runner caches actions by ref, so moving the floating `v1` does **not** diff --git a/README.md b/README.md index feaf32b..6d55826 100644 --- a/README.md +++ b/README.md @@ -18,8 +18,11 @@ This action: than from the health of a long-lived connection. - On completion, fetches each chroot's `builder-live.log` via `copr-cli download-build` and emits them as `::group::` blocks. -- Fails CI if the build **fails**. A build merely still running when the wait - budget expires is reported, not failed. +- Waits for **every chroot** to finish, not just the build as a whole, and fails + CI if any of them failed. A build-level "succeeded" is not the same as every + release having produced an RPM. +- Fails CI if the build **fails**. A build or chroot merely still running when + the wait budget expires is reported, not failed. - Returns within `COPR_WAIT_BUDGET` whatever happens, so the step finishes before a CI runner's step limit kills it. @@ -29,6 +32,7 @@ This action: |---|---|---| | `COPR_WAIT_BUDGET` | `2700` | Seconds to wait for the build before returning. A build still running at expiry is reported as such and the step exits 0 — CI runners commonly kill a step long before a large build finishes. | | `COPR_POLL_INTERVAL` | `30` | Seconds between build-state polls. | +| `COPR_API_BASE` | `https://copr.fedorainfracloud.org` | COPR instance queried for per-chroot state. | ## Requirements @@ -36,6 +40,8 @@ The runner must already have: - `copr-cli` on `PATH` (provided by the `copr-cli` RPM on Fedora runners). - `grep -P` (PCRE, default on Fedora). +- `python3`, used to read per-chroot state from COPR's API. Already a hard + dependency of `copr-cli`, so this adds nothing in practice. ## Inputs diff --git a/scripts/copr-build.sh b/scripts/copr-build.sh index 33636b3..5f1542a 100755 --- a/scripts/copr-build.sh +++ b/scripts/copr-build.sh @@ -58,7 +58,38 @@ build_state() { timeout "$STATUS_TIMEOUT" copr-cli status "$1" 2>/dev/null | tail -n1 | tr -d '[:space:]' } -# Poll COPR until the build settles, or until BUILD_TIMEOUT elapses. Echoes the +# Per-chroot states, one " " per line. +# +# The build-level state is not enough on its own: it describes the build as a +# whole, and a green tick there is not the same as "every chroot produced an +# RPM". copr-cli has no per-chroot status subcommand, so this uses COPR's public +# API. python3 is a hard dependency of copr-cli itself, so it adds no new +# requirement, and the endpoint needs no authentication. +chroot_states() { + timeout "$STATUS_TIMEOUT" python3 -c ' +import json, os, sys, urllib.request +base = os.environ.get("COPR_API_BASE", "https://copr.fedorainfracloud.org") +url = f"{base}/api_3/build-chroot/list?build_id={sys.argv[1]}" +with urllib.request.urlopen(url, timeout=20) as r: + for c in json.load(r).get("items", []): + print(c.get("name", "?"), c.get("state", "unknown")) +' "$1" 2>/dev/null +} + +# True when every chroot has finished. Empty or unreadable output is treated as +# "not yet", so a transient API failure retries rather than passing the build. +all_chroots_finished() { + local out name state + out="$(chroot_states "$1")" + [ -n "$out" ] || return 1 + while read -r name state; do + [ -n "$name" ] || continue + is_terminal_state "$state" || return 1 + done <<< "$out" + return 0 +} + +# Poll COPR until the build settles, or until WAIT_BUDGET elapses. Echoes the # terminal state, or "unknown" if we ran out of patience. # Poll until the build settles or the budget runs out. Echoes the last state # seen, which the caller checks for terminality — a non-terminal state means we @@ -69,7 +100,7 @@ wait_for_terminal_state() { started=$(date +%s) deadline=$((started + WAIT_BUDGET)) state="$(build_state "$build_id")" - while ! is_terminal_state "$state"; do + while ! is_terminal_state "$state" || ! all_chroots_finished "$build_id"; do now=$(date +%s) if [ "$now" -ge "$deadline" ]; then echo "${state:-unknown}" @@ -81,8 +112,9 @@ wait_for_terminal_state() { # inactivity timeout before it can finish. Two builds died this way after # ~24-27 minutes of no output, one of them 66 seconds before COPR reported # success. Printing every poll keeps the step alive and shows progress. - printf ' [%3dm %3ds] build %s: %s\n' \ - $(((now - started) / 60)) $(((now - started) % 60)) "$build_id" "${state:-unknown}" >&2 + printf ' [%3dm %3ds] build %s: %s | %s\n' \ + $(((now - started) / 60)) $(((now - started) % 60)) "$build_id" "${state:-unknown}" \ + "$(chroot_states "$build_id" | awk '{printf "%s=%s ", $1, $2}')" >&2 sleep "$POLL_INTERVAL" state="$(build_state "$build_id")" done @@ -114,7 +146,7 @@ echo # the runner killed the step at 08:35:50, which the job then reported as a # build failure. Conflating "the watcher lost its connection" with "the build # failed" turns a green release into a red one and blocks any dependent job. -timeout "$BUILD_TIMEOUT" copr-cli watch-build "$BUILD_ID" & +timeout "$WAIT_BUDGET" copr-cli watch-build "$BUILD_ID" & WATCH_PID=$! # The verdict comes from COPR itself. Polling also means we stop as soon as the @@ -127,9 +159,48 @@ wait "$WATCH_PID" 2>/dev/null || true echo echo "COPR build $BUILD_ID final state: $BUILD_STATE" +CHROOTS="$(chroot_states "$BUILD_ID")" +if [ -n "$CHROOTS" ]; then + echo "Per-chroot results:" + while read -r cname cstate; do + [ -n "$cname" ] || continue + echo " $cname: $cstate" + done <<< "$CHROOTS" +fi + +# A chroot that failed while the build as a whole reads succeeded still means no +# RPM for that release, so the job must not go green on it. A chroot that simply +# has not finished is a different thing and must not be reported as a failure — +# that is the mistake this action has made in every other guise. +FAILED_CHROOTS="$( + while read -r cname cstate; do + [ -n "$cname" ] || continue + case "$cstate" in + succeeded | skipped | importing | pending | starting | running | waiting) ;; + *) printf '%s(%s) ' "$cname" "$cstate" ;; + esac + done <<< "$CHROOTS" +)" +UNFINISHED_CHROOTS="$( + while read -r cname cstate; do + [ -n "$cname" ] || continue + is_terminal_state "$cstate" || printf '%s(%s) ' "$cname" "$cstate" + done <<< "$CHROOTS" +)" + case "$BUILD_STATE" in succeeded) - STATUS=0 + if [ -n "$FAILED_CHROOTS" ]; then + echo "error: build $BUILD_ID reports succeeded but these chroots did not: $FAILED_CHROOTS" >&2 + STATUS=1 + elif [ -n "$UNFINISHED_CHROOTS" ]; then + echo "note: build $BUILD_ID has not finished within our ${WAIT_BUDGET}s wait budget" + echo " (chroots still going: $UNFINISHED_CHROOTS). COPR will finish it on its own." + echo " Follow: https://copr.fedorainfracloud.org/coprs/build/$BUILD_ID" + STATUS=0 + else + STATUS=0 + fi ;; skipped) # COPR already had this exact build; nothing was rebuilt, but nothing is diff --git a/tests/test-copr-build.sh b/tests/test-copr-build.sh index 198e787..6d855b1 100755 --- a/tests/test-copr-build.sh +++ b/tests/test-copr-build.sh @@ -19,6 +19,17 @@ FAIL=0 make_stub() { local dir="$1" mkdir -p "$dir" + # chroot_states shells out to python3; stub it so the per-chroot path is + # exercised without touching COPR. STUB_CHROOTS is "name=state,name=state". + cat > "$dir/python3" <<'PYSTUB' +#!/bin/bash +IFS=',' read -r -a pairs <<< "${STUB_CHROOTS:-}" +for p in "${pairs[@]}"; do + [ -n "$p" ] || continue + echo "${p%%=*} ${p##*=}" +done +PYSTUB + chmod +x "$dir/python3" cat > "$dir/copr-cli" <<'STUB' #!/bin/bash case "$1" in @@ -65,7 +76,8 @@ run_case() { local tmp out rc tmp="$(mktemp -d)" make_stub "$tmp/bin" - out="$(env "$@" STUB_IDX_FILE="$tmp/idx" PATH="$tmp/bin:$PATH" \ + out="$(env STUB_CHROOTS="${STUB_CHROOTS:-f43=succeeded,f44=succeeded,rawhide=succeeded}" \ + "$@" STUB_IDX_FILE="$tmp/idx" PATH="$tmp/bin:$PATH" \ COPR_POLL_INTERVAL=1 COPR_BUILD_TIMEOUT="${CASE_TIMEOUT:-20}" COPR_STATUS_TIMEOUT="${COPR_STATUS_TIMEOUT:-10}" \ bash "$SCRIPT" owner/project test.src.rpm 2>&1)" rc=$? @@ -111,6 +123,22 @@ CASE_TIMEOUT=30 run_case "a hung status call does not stall the poll loop" \ 0 "final state: succeeded" STUB_WATCH_HANGS=1 STUB_STATES="running succeeded" \ STUB_STATUS_HANGS_UNTIL=1 COPR_STATUS_TIMEOUT=2 +# A chroot that failed while the build as a whole reads succeeded still means no +# RPM for that release. The job must not go green on it. +run_case "a failed chroot fails the job even when the build says succeeded" \ + 1 "these chroots did not" STUB_WATCH_HANGS=0 STUB_STATES="succeeded" \ + STUB_CHROOTS="f43=succeeded,f44=failed,rawhide=succeeded" + +# All three green is the ordinary pass, and each is reported. +run_case "all chroots succeeding passes and is reported" \ + 0 "rawhide: succeeded" STUB_WATCH_HANGS=0 STUB_STATES="succeeded" \ + STUB_CHROOTS="f43=succeeded,f44=succeeded,rawhide=succeeded" + +# The build settling before its chroots do must not end the wait early. +CASE_TIMEOUT=4 run_case "build succeeded but a chroot still running keeps waiting" \ + 0 "chroots still going: f44(running)" STUB_WATCH_HANGS=0 STUB_STATES="succeeded" \ + STUB_CHROOTS="f43=succeeded,f44=running,rawhide=succeeded" + # The step limit case: the build is still running when our wait budget expires. # This must NOT be reported as a failure — that is what turned every green # release red — and it must not try to fetch logs that do not exist yet.