andrew/libakgl
1
0
Fork 0
Code Issues Pull Requests 1 Actions 1 Packages Projects Releases Wiki Activity

Stop a failed controller-DB fetch from destroying the tracked fallback

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Andrew Kesterson
2026-08-01 06:54:31 -04:00
parent 913834a3af
commit d9f0187ecf
6 changed files with 184 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
.gitea/workflows/ci.yaml
View File

@@ -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

41
AGENTS.md
View File

@@ -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

24
CMakeLists.txt
View File

@@ -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

57
TODO.md
View File

@@ -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

118
mkcontrollermappings.sh
View File

@@ -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 <repository-root>
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 <<EOF
#ifndef _SDL_GAMECONTROLLERDB_H_
#define _SDL_GAMECONTROLLERDB_H_
if [[ $# -lt 1 ]]; then
die "usage: $0 <repository-root>"
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}"

8
scripts/mutation_test.py
View File

@@ -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")