From d9f0187ecf09f5f434388e16ede8437ffd7b1801 Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Sat, 1 Aug 2026 06:54:31 -0400 Subject: [PATCH] Stop a failed controller-DB fetch from destroying the tracked fallback Closes Defects -> Known and still open item 12, both halves. include/akgl/SDL_GameControllerDB.h is tracked on purpose: it is the offline fallback that keeps the library buildable when upstream is unreachable. The script that writes it had no set -e and never checked curl, so a failed fetch wrote AKGL_SDL_GAMECONTROLLER_DB_LEN 0 and an empty array over the good copy and exited 0. The safety net destroyed itself, and because CMake re-ran the generator on every build, an offline build was enough to do it. The script now runs under set -euo pipefail, fetches into a temporary directory, and moves the result into place only after checking curl's exit status (with --fail, so an HTTP error is a status rather than an error page in the body), a plausible minimum mapping count, and that no mapping carries a quote or backslash that would break the C string literal it becomes. Any of those failing leaves the tracked header exactly as it was and exits non-zero. Verified against all five failure modes -- unresolvable host, 404, truncated response, empty-but-successful response (the original failure exactly), and a response containing a quote. Each refuses, and the header's checksum is unchanged after every one. AKGL_CONTROLLERDB_URL and AKGL_CONTROLLERDB_MIN_LINES are environment-overridable, which is how. The build no longer runs it at all. The add_custom_command declared a relative OUTPUT, which CMake resolves against the binary directory while the script writes to the source directory, so the declared output never appeared and every build re-ran the command -- needing network access and leaving the tree dirty. It is an explicit `controllerdb` target now, and the header is no longer listed as a library source, which is all it was there for. The empty-initializer problem goes with it: an empty array initializer is a constraint violation in ISO C that compiles only as a GCC extension, and the minimum-count check guarantees at least one entry. Also here, because it rested on a premise that turned out to be false: the character suite is no longer excluded from CI's coverage and memory-check jobs, nor from the mutation harness by default. It was excluded on the grounds that it failed deliberately. It did not -- it was reporting success while running one of its four tests. 25/25 pass, memcheck clean, shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/ci.yaml | 10 ++-- AGENTS.md | 41 ++++++++------ CMakeLists.txt | 24 ++++++-- TODO.md | 57 +++++++++++-------- mkcontrollermappings.sh | 118 ++++++++++++++++++++++++++++++++------- scripts/mutation_test.py | 8 ++- 6 files changed, 184 insertions(+), 74 deletions(-) diff --git a/.gitea/workflows/ci.yaml b/.gitea/workflows/ci.yaml index 47b6509..8da6a11 100644 --- a/.gitea/workflows/ci.yaml +++ b/.gitea/workflows/ci.yaml @@ -42,7 +42,6 @@ jobs: export LD_LIBRARY_PATH="$PWD/build:$PWD/build/deps/SDL:$PWD/build/deps/SDL_image:$PWD/build/deps/SDL_mixer:$PWD/build/deps/SDL_ttf" ctest \ --test-dir build \ - -E '^character$' \ --output-on-failure \ --output-junit "$(pwd)/ctest-junit.xml" - name: Publish test results @@ -161,11 +160,12 @@ jobs: cmake -S . -B build \ -DCMAKE_BUILD_TYPE=RelWithDebInfo cmake --build build --parallel - # The character suite is excluded here for the same reason as in the - # coverage job: it fails deliberately, and a failing suite would be - # reported as "valgrind found nothing but ctest exited non-zero". + # Every suite, character included. It was excluded from both this job + # and the coverage job on the grounds that it failed deliberately -- it + # did not. It was reporting success while running one of its four tests; + # see TODO.md, "Test suites that could not fail". - name: Memory check - run: scripts/memcheck.sh -E '^character$' + run: scripts/memcheck.sh - name: Upload valgrind logs if: always() uses: actions/upload-artifact@v4 diff --git a/AGENTS.md b/AGENTS.md index 4d59229..a9cb1f7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,26 +19,28 @@ Working rules: - **Never hand-edit it.** It is machine-written; changes belong in `mkcontrollermappings.sh`. -- **Do not commit incidental regeneration churn.** The build regenerates the - header on every run, and the script stamps `$(date)` into a comment, so an - ordinary build leaves the file modified with nothing of substance changed. - Revert that before committing: `git checkout -- include/akgl/SDL_GameControllerDB.h`. +- **An ordinary build does not touch it.** Regeneration is + `cmake --build build --target controllerdb`, and nothing else runs the script. + Until 0.5.0 an `add_custom_command` re-ran it on every build, which made every + build need network access and left the file modified with a fresh `$(date)` + stamp and nothing of substance changed. - **Update it as a deliberate, standalone commit** when you actually want newer mappings, so the diff is reviewable and bisectable. -- **Never commit a copy with `AKGL_SDL_GAMECONTROLLER_DB_LEN 0`.** That is the - signature of a failed fetch, not an empty upstream — see the defect note - below. Check the constant before staging this file. +- **A failed fetch cannot reach this file.** The script assembles the header in + a temporary directory and moves it into place only after checking curl's exit + status, a plausible minimum mapping count, and that no mapping contains a + character that would break a C string literal. Any of those failing leaves the + tracked copy untouched and exits non-zero. - Formatting tooling ignores it: `scripts/reindent.sh` and the pre-commit hook both skip it. -> **Known defect (tracked in `TODO.md`).** The safety net is currently -> self-defeating. `mkcontrollermappings.sh` has no `set -e` and does not check -> `curl`'s exit status, so when the fetch fails it writes a header with -> `AKGL_SDL_GAMECONTROLLER_DB_LEN 0` and an empty array **over the good tracked -> copy, and exits 0**. Because CMake re-runs the generator on every build, an -> offline build silently destroys the very fallback the file exists to provide. -> Until that is fixed, treat a dirty `SDL_GameControllerDB.h` after a build as -> suspect and check the length constant before committing it. +> **Fixed in 0.5.0.** The safety net used to be self-defeating: +> `mkcontrollermappings.sh` had no `set -e` and never checked `curl`, so a +> failed fetch wrote `AKGL_SDL_GAMECONTROLLER_DB_LEN 0` and an empty array +> **over the good tracked copy, and exited 0** — and because CMake re-ran the +> generator on every build, an offline build silently destroyed the very +> fallback the file exists to provide. Both halves are closed: the script +> refuses rather than overwrites, and a build does not run it. ## Build, Test, and Development Commands @@ -75,7 +77,7 @@ because the perf suites scale themselves down under valgrind. Suppressions for third-party findings live in `scripts/valgrind.supp`; add one only when the finding genuinely cannot be fixed from this repository. -Run mutation testing with `cmake --build build --target mutation`. For a quick smoke run, use `scripts/mutation_test.py --target src/tilemap.c --max-mutants 10`; the harness mutates only a scratch copy and excludes the intentionally failing character test. +Run mutation testing with `cmake --build build --target mutation`. For a quick smoke run, use `scripts/mutation_test.py --target src/tilemap.c --max-mutants 10`; the harness mutates only a scratch copy and runs every suite. Generate HTML and Cobertura coverage reports with `cmake -S . -B build-coverage -DAKGL_COVERAGE=ON -DCMAKE_BUILD_TYPE=Debug`, then build and run CTest. Reports are written to `build-coverage/coverage/`; this mode requires `gcovr` and GCC or Clang. @@ -91,8 +93,11 @@ because the others cannot do its work: | `memory_check` | RelWithDebInfo | `scripts/memcheck.sh`, every suite under valgrind. Gates: a definite leak, an invalid access or a branch on uninitialised memory fails the build. Keeps the valgrind logs as an artifact. | | `mutation_test` | Debug | One focused source file, so CI stays bounded. | -The `character` suite is excluded wherever a whole run is selected: it fails -deliberately, and pinning current-but-wrong behavior is not what it is for. +Every suite runs in every job. The `character` suite used to be excluded from +the coverage and memory-check jobs on the grounds that it failed deliberately. +It did not: it was reporting success while running one of its four tests, for +two independent reasons, and both are fixed. See TODO.md, "Test suites that +could not fail". ## Coding Style diff --git a/CMakeLists.txt b/CMakeLists.txt index a56664a..71d8586 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -196,10 +196,27 @@ configure_file(${CMAKE_CURRENT_SOURCE_DIR}/include/akgl/version.h.in file(COPY "${CMAKE_CURRENT_SOURCE_DIR}/tests/assets" DESTINATION "${CMAKE_CURRENT_BINARY_DIR}") -add_custom_command( - OUTPUT ${GAMECONTROLLERDB_H} +# Regenerating the controller database is a deliberate act, not part of a build. +# +# This used to be an add_custom_command whose OUTPUT was the *relative* path +# "include/akgl/SDL_GameControllerDB.h", which CMake resolves against the binary +# directory while the script writes to the source directory. The declared output +# therefore never appeared, the command was permanently out of date, and every +# build re-ran it -- so every build needed network access and left the tracked +# header dirty with a fresh $(date) stamp. +# +# `cmake --build build --target controllerdb` when newer mappings are actually +# wanted. The result is its own commit, per AGENTS.md. +if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR) + set(AKGL_CONTROLLERDB_TARGET controllerdb) +else() + set(AKGL_CONTROLLERDB_TARGET akgl_controllerdb) +endif() +add_custom_target(${AKGL_CONTROLLERDB_TARGET} COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/mkcontrollermappings.sh ${CMAKE_CURRENT_SOURCE_DIR} - COMMENT "Generating controller mappings ..." + WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} + USES_TERMINAL + COMMENT "Fetching controller mappings and rewriting ${GAMECONTROLLERDB_H} ..." ) # Add include directories @@ -226,7 +243,6 @@ add_library(akgl SHARED src/tilemap.c src/util.c src/version.c - ${GAMECONTROLLERDB_H} ) # While the major version is 0 the ABI is not stable across minor releases, so diff --git a/TODO.md b/TODO.md index 3c9f57d..8cfe445 100644 --- a/TODO.md +++ b/TODO.md @@ -740,32 +740,43 @@ Each was found by a test written to assert correct behavior. `tests/controller.c` passes -1 and -4096 to each. 12. **A failed controller-DB fetch silently destroys the tracked fallback.** - `include/akgl/SDL_GameControllerDB.h` is committed deliberately so the - library still builds if upstream disappears. But `mkcontrollermappings.sh` - has no `set -e` and never checks `curl`'s exit status: on a failed fetch it - writes `mappings.txt` empty, then overwrites the good tracked header with - `AKGL_SDL_GAMECONTROLLER_DB_LEN 0` and an empty initializer — and exits 0. - Verified by pointing the script at an unresolvable host. + **Fixed in 0.5.0**, both halves. - Two compounding problems: + `mkcontrollermappings.sh` now runs under `set -euo pipefail`, fetches into a + temporary directory, and moves the result into place only after checking + three things: curl's exit status (with `--fail`, so an HTTP error is a + status rather than an error page in the body), a plausible minimum mapping + count, and that no mapping carries a quote or backslash that would break the + C string literal it becomes. Any of those failing leaves the tracked header + exactly as it was and exits non-zero. `AKGL_CONTROLLERDB_URL` and + `AKGL_CONTROLLERDB_MIN_LINES` are environment-overridable, which is how the + failure paths were exercised. - - `add_custom_command` at `CMakeLists.txt:89-93` declares - `OUTPUT include/akgl/SDL_GameControllerDB.h` as a *relative* path, which - CMake resolves against the binary directory, while the script writes to - the source directory. The declared output never appears, so the command is - permanently out of date and re-runs on every build — making every build - depend on network access and leaving the file dirty in the working tree - each time. - - `const char *SDL_GAMECONTROLLER_DB[] = {\n};` is an empty initializer, - which is a constraint violation in ISO C and compiles only as a GCC - extension. + Verified against all five: an unresolvable host, a 404, a truncated + response, an empty-but-successful response -- the original failure exactly + -- and a response containing a quote. Each refuses, and the header's + checksum is unchanged after every one. + + The build no longer runs it. The `add_custom_command` declared + `OUTPUT include/akgl/SDL_GameControllerDB.h` as a *relative* path, which + CMake resolves against the binary directory while the script writes to the + source directory, so the declared output never appeared, the command was + permanently out of date, and every build re-ran it -- needing network access + and leaving the tree dirty. It is an explicit `controllerdb` target now, and + the header is no longer listed as a library source, which is all it was + there for. + + The empty-initializer problem goes with it: `const char *SDL_GAMECONTROLLER_DB[] = {};` + is a constraint violation in ISO C that compiles only as a GCC extension, + and the minimum-count check is what guarantees at least one entry. + + Regenerating against the real upstream produced byte-identical mappings -- + 2255 entries, no content change -- so the rewrite is faithful. The only diff + is the `$(date)` stamp and the include guard, which is + `_AKGL_SDL_GAMECONTROLLERDB_H_` now to match every other header. That change + was made in the generator rather than by hand, and the tracked copy carries + it so a future regeneration shows no spurious diff. - Fix: have the script fetch to a temporary file, check `curl`'s status and a - plausible minimum line count, and leave the existing header untouched on - failure (exiting non-zero). Separately, make regeneration explicit — a - dedicated `controllerdb` target, or an `OUTPUT` that matches where the - script actually writes — so an ordinary build neither needs the network nor - dirties the tree. 13. **A stale build tree in the source directory breaks the coverage run.** `CMakeLists.txt:208` and `CMakeLists.txt:223` pass `--root "${CMAKE_CURRENT_SOURCE_DIR}"` to gcovr, and gcovr searches for diff --git a/mkcontrollermappings.sh b/mkcontrollermappings.sh index 9325e53..869e1a5 100755 --- a/mkcontrollermappings.sh +++ b/mkcontrollermappings.sh @@ -1,32 +1,108 @@ #!/bin/bash +# +# Regenerate include/akgl/SDL_GameControllerDB.h from the community controller +# database. +# +# That header is tracked on purpose: it is the offline fallback that keeps the +# library buildable when upstream is renamed, rate-limited, taken down, or +# simply unreachable. This script therefore has one rule above all others -- +# **it must never replace a good header with a worse one**. Until 0.5.0 it did +# exactly that: no `set -e`, no check on curl's exit status, so a failed fetch +# wrote an empty mappings file, overwrote the good tracked header with +# `AKGL_SDL_GAMECONTROLLER_DB_LEN 0` and an empty initializer, and exited 0. +# Verified by pointing it at an unresolvable host. +# +# Everything is assembled in a temporary directory and moved into place only +# once it has been checked. A failure leaves the existing header untouched and +# exits non-zero. +# +# This is no longer run by an ordinary build. `cmake --build build --target +# controllerdb` is the deliberate way to update the mappings, so a build needs +# no network access and does not leave the working tree dirty. +# +# Usage: mkcontrollermappings.sh -rootdir=$1 +set -euo pipefail -curl https://raw.githubusercontent.com/mdqinc/SDL_GameControllerDB/refs/heads/master/gamecontrollerdb.txt | grep -v '^#' | grep -v '^$' | sed s/',$'//g > mappings.txt +AKGL_CONTROLLERDB_URL="${AKGL_CONTROLLERDB_URL:-https://raw.githubusercontent.com/mdqinc/SDL_GameControllerDB/refs/heads/master/gamecontrollerdb.txt}" +# A floor, not a target. The database has carried well over two thousand entries +# for years; a few hundred means something truncated the response, which is a +# fetch that "succeeded" and still must not be committed. +AKGL_CONTROLLERDB_MIN_LINES="${AKGL_CONTROLLERDB_MIN_LINES:-1000}" -filelen=$(wc -l mappings.txt | cut -d ' ' -f 1) +function die() +{ + echo "mkcontrollermappings: $*" >&2 + exit 1 +} -cat > ${rootdir}/include/akgl/SDL_GameControllerDB.h <" +fi -// Taken from https://raw.githubusercontent.com/mdqinc/SDL_GameControllerDB/refs/heads/master/gamecontrollerdb.txt on $(date) +rootdir="$1" +target="${rootdir}/include/akgl/SDL_GameControllerDB.h" -#define AKGL_SDL_GAMECONTROLLER_DB_LEN ${filelen} +if [[ ! -d "${rootdir}/include/akgl" ]]; then + die "no such directory: ${rootdir}/include/akgl" +fi +command -v curl >/dev/null 2>&1 || die "curl is not available" -const char *SDL_GAMECONTROLLER_DB[] = { -EOF +workdir="$(mktemp -d)" -counter=0 +function cleanup() +{ + rm -rf "${workdir}" +} +trap cleanup EXIT -cat mappings.txt | while read LINE; -do - if [[ $counter -gt 0 ]]; then - printf ",\n" >> ${rootdir}/include/akgl/SDL_GameControllerDB.h; - fi - printf " \"${LINE}\"" >> ${rootdir}/include/akgl/SDL_GameControllerDB.h; - counter=$((counter + 1)) -done +raw="${workdir}/raw.txt" +mappings="${workdir}/mappings.txt" +staged="${workdir}/SDL_GameControllerDB.h" -printf "\n};\n" >> ${rootdir}/include/akgl/SDL_GameControllerDB.h -printf "#endif // _SDL_GAMECONTROLLERDB_H_\n" >> ${rootdir}/include/akgl/SDL_GameControllerDB.h +# --fail so an HTTP error status is an exit status rather than an error page in +# the body. Not a pipeline: the exit status being checked is curl's, and a +# pipeline reports the last command's. +if ! curl --fail --silent --show-error --location "${AKGL_CONTROLLERDB_URL}" --output "${raw}"; then + die "fetch failed from ${AKGL_CONTROLLERDB_URL}; ${target} left as it was" +fi + +grep -v '^#' "${raw}" | grep -v '^$' | sed s/',$'//g > "${mappings}" || true + +filelen=$(wc -l < "${mappings}" | tr -d ' ') + +if [[ "${filelen}" -lt "${AKGL_CONTROLLERDB_MIN_LINES}" ]]; then + die "fetched only ${filelen} mappings, expected at least ${AKGL_CONTROLLERDB_MIN_LINES}; ${target} left as it was" +fi + +# Every mapping becomes a C string literal, so a quote or a backslash would +# produce a header that does not compile -- or, worse, one that does. +if grep -q '["\\]' "${mappings}"; then + die "fetched mappings contain a quote or a backslash; ${target} left as it was" +fi + +{ + echo "#ifndef _AKGL_SDL_GAMECONTROLLERDB_H_" + echo "#define _AKGL_SDL_GAMECONTROLLERDB_H_" + echo "" + echo "// Taken from ${AKGL_CONTROLLERDB_URL} on $(date)" + echo "" + echo "#define AKGL_SDL_GAMECONTROLLER_DB_LEN ${filelen}" + echo "" + echo "const char *SDL_GAMECONTROLLER_DB[] = {" + # One pass: wrap each mapping as a quoted, indented, comma-terminated entry, + # then drop the trailing comma from the last. An empty initializer is a + # constraint violation in ISO C that compiles only as a GCC extension, and + # the length check above is what guarantees there is at least one entry. + sed -e 's/.*/ "&",/' -e '$ s/,$//' "${mappings}" + echo "};" + echo "#endif // _AKGL_SDL_GAMECONTROLLERDB_H_" +} > "${staged}" + +# Checked before it is allowed anywhere near the tracked copy. +if ! grep -q "^#define AKGL_SDL_GAMECONTROLLER_DB_LEN *${filelen}\$" "${staged}"; then + die "staged header did not come out as expected; ${target} left as it was" +fi + +mv "${staged}" "${target}" +echo "mkcontrollermappings: wrote ${filelen} mappings to ${target}" diff --git a/scripts/mutation_test.py b/scripts/mutation_test.py index ad82b38..e237581 100755 --- a/scripts/mutation_test.py +++ b/scripts/mutation_test.py @@ -24,7 +24,7 @@ Usage: Default: all libakgl-owned C files under src/ --work DIR scratch dir for the mutated copy (default: a temp dir) --timeout SECONDS per-suite ctest timeout (default: 120) - --exclude-test REGEX CTest regex to exclude (default: ^character$) + --exclude-test REGEX CTest regex to exclude (default: none) --threshold PCT exit non-zero if mutation score < PCT (default: 0 = off) --list only list the mutants that would be run, then exit --keep keep the scratch working copy on exit (for debugging) @@ -327,8 +327,10 @@ def main(): ap.add_argument("--target", action="append", default=None) ap.add_argument("--work", default=None) ap.add_argument("--timeout", type=int, default=120) - ap.add_argument("--exclude-test", default="^character$", - help="CTest regex to exclude (default: ^character$)") + # No default exclusion. This was "^character$" while that suite was + # believed to fail deliberately; it does not, and did not. + ap.add_argument("--exclude-test", default="", + help="CTest regex to exclude (default: none)") ap.add_argument("--threshold", type=float, default=0.0) ap.add_argument("--junit", default=None, help="write a JUnit XML report to this path")