From 949ed1f037182b42d5792f5c95ce941c591f3862 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 11 Sep 2026 15:30:17 -0400 Subject: [PATCH] [Test, CI] (tools/trace_replay, workflows): give retrace-split the negative control it was cloned without, match ConfigLoader's whole INFO sentence rather than a KEY=VALUE an env dump also prints, read the three-arm names file the identity step was writing and discarding, and refuse an unknown manifest key that silently empties a CI matrix --- .github/workflows/test.yml | 149 +++++++++++++++++++++--- tools/trace_replay/run_trace_case.cmake | 32 ++++- tools/trace_replay/trace_cases.py | 44 +++++++ 3 files changed, 207 insertions(+), 18 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1bd30ca7..d1cc23d9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1000,15 +1000,45 @@ jobs: run: | ulimit -c unlimited sudo sysctl -w kernel.core_pattern='/tmp/core.%e.%p' - ctest -N -L integration-gpu | grep -E '^ *Test *#[0-9]+:' | sed -E 's/^ *Test *#[0-9]+: //' | sort > /tmp/names-split.txt - count=$(wc -l < /tmp/names-split.txt) + count=$(ctest -N -L integration-gpu | grep -cE '^ *Test *#[0-9]+:') if [ "${count}" -lt 1 ]; then echo "::error::the split runtime registers ${count} integration-gpu entries" exit 1 fi echo "integration-gpu entries in the split build: ${count}" - MOBILEGL_TRANSPORT=monolith ctest --output-on-failure -L integration-gpu --no-tests=error -j 4 - MOBILEGL_TRANSPORT=inproc ctest --output-on-failure -L integration-gpu --no-tests=error -j 4 + MOBILEGL_TRANSPORT=monolith ctest --output-on-failure -L integration-gpu --no-tests=error -j 4 --output-junit "${RUNNER_TEMP}/arm-monolith.xml" + MOBILEGL_TRANSPORT=inproc ctest --output-on-failure -L integration-gpu --no-tests=error -j 4 --output-junit "${RUNNER_TEMP}/arm-inproc.xml" + # NAME **AND STATUS**, and it is the comparison this step claimed to make and did not + # (review finding N-3): the first version wrote a names file and never read it, and + # `--output-on-failure` treats a SKIPPED test as not-a-failure - so the very failure + # ARCHITECTURE.md:521 is about, "an inproc arm that skipped forty entries the monolith arm + # ran", was invisible here and caught only by the local gate. `ctest -N` cannot see it + # either: this is one build directory, so the two arms have identical name lists by + # construction and the difference is entirely in what each entry DID. + python3 - "${RUNNER_TEMP}/arm-monolith.xml" "${RUNNER_TEMP}/arm-inproc.xml" <<'PY' + import sys, xml.etree.ElementTree as ET + def rows(path): + out = {} + for case in ET.parse(path).getroot().iter('testcase'): + status = 'passed' + if case.find('failure') is not None or case.find('error') is not None: + status = 'failed' + elif case.find('skipped') is not None or case.get('status') in ('notrun', 'disabled'): + status = 'skipped' + out[case.get('name')] = status + return out + a, b = rows(sys.argv[1]), rows(sys.argv[2]) + diff = sorted(set(a) ^ set(b)) + sorted(n for n in set(a) & set(b) if a[n] != b[n]) + if diff: + for name in diff[:40]: + print(f"::error::{name}: monolith={a.get(name, '')} inproc={b.get(name, '')}") + print(f"::error::the monolith and inproc arms of ctest -L integration-gpu differ on " + f"{len(diff)} entries. ARCHITECTURE.md:521 requires them identical name for name " + f"AND status; an entry that SKIPPED on one arm and ran on the other is the " + f"failure this compares for, and it is not a failure to --output-on-failure.") + raise SystemExit(1) + print(f"the two arms agree on all {len(a)} entries, name and status") + PY # THE RUNTIME HALF OF "THIS IS REALLY A SPLIT BUILD". The build-level nm check in # build-linux-split proves the library CARRIES MG_Remote; this proves the transport @@ -1025,8 +1055,11 @@ jobs: echo "::error::${log} does not exist: the DirectGLES.Split.PersistentMapArm. entry never ran, so nothing in this job establishes that MOBILEGL_TRANSPORT ever resolved to inproc in a live process" exit 1 fi - if ! grep -q "MOBILEGL_TRANSPORT=inproc" "${log}"; then - echo "::error::${log} carries no transport-resolution line. ConfigLoader logs one at INFO when it selects InProcess, and that code exists only in a MOBILEGL_BUILD_DISAGGREGATED build - so this lane ran a monolith library while claiming to be the split lane." + # THE DISTINCTIVE PART OF THE INFO LINE, not the bare KEY=VALUE (review finding M-5): + # ConfigLoader logs `Config: Accepted env variable: MOBILEGL_TRANSPORT=inproc` for the + # env dump too, unconditionally and in a PULL build, and at DEBUG that line is live. + if ! grep -q "MOBILEGL_TRANSPORT=inproc - the MGPipe record stream" "${log}"; then + echo "::error::${log} carries no transport-resolution line. ConfigLoader::InitTransport logs it at INFO when it selects InProcess, and that code exists only in a MOBILEGL_BUILD_DISAGGREGATED build - so this lane ran a monolith library while claiming to be the split lane. (A bare MOBILEGL_TRANSPORT=inproc substring is NOT accepted: the env dump prints one in every build.)" exit 1 fi echo "the split lane resolved MOBILEGL_TRANSPORT=inproc" @@ -1051,12 +1084,24 @@ jobs: env: MOBILEGL_ITEST_REQUIRE_GPU: "1" run: | - armed=0 - if grep -qs "MGITEST_REMOTE_CLIENT_PRESENT" MobileGL/MG_IntegrationTest/*_tests.cmake; then - armed=1 - fi - if [ "${armed}" -eq 0 ]; then - echo "::warning::the DirectGLES.Split. lanes are DISARMED on this tree - MG_Remote still carries package c0's signature stubs, so every Split entry skips and neither negative control can fire. This step becomes a gate automatically on the commit that lands the last of c1/s1/v1; it is not a green that asserted anything today." + # THE ARMED STATE IS DERIVED FROM BEHAVIOUR, not from a marker string in the generated + # ctest files. The first version read MGITEST_REMOTE_CLIENT_PRESENT out of + # *_tests.cmake, which was a restatement of the CMake source probe review finding M-1 + # falsified; the arming condition is now a runtime fact inside each test process, so the + # only honest way to ask it from a shell is to look at what the entries DID. + ctest -L integration-split -j 4 --no-tests=error --output-junit "${RUNNER_TEMP}/isplit.xml" || true + armed=$(python3 - "${RUNNER_TEMP}/isplit.xml" <<'PY' + import sys, xml.etree.ElementTree as ET + ran = 0 + for case in ET.parse(sys.argv[1]).getroot().iter('testcase'): + if case.find('skipped') is None and case.get('status') not in ('notrun', 'disabled'): + ran += 1 + print(ran) + PY + ) + echo "split entries that actually ran: ${armed}" + if [ "${armed}" -lt 1 ]; then + echo "::warning::every DirectGLES.Split. entry SKIPPED, so neither negative control can fire. The arming condition is a runtime fact - MG_Config::Transport, ClientSession::Active() and ImplementedVerbCount(), read by Harness/SplitRuntimePeek - and it becomes true on the commit that lands the last of c1/s1/v1. This step becomes a gate then, with no edit; it is not a green that asserted anything today." exit 0 fi run_control() { @@ -1391,7 +1436,16 @@ jobs: # so retrace-split needs no fixtures of its own either. It is one case today - OpenRA, # which is what the phase gate names - and trace_cases.py refuses a `split` case that is # not in CI or does not run DirectGLES, so the subset cannot silently become empty. - echo "split-matrix=$(python3 tools/trace_replay/trace_cases.py --ci --format github-split-matrix)" >> "$GITHUB_OUTPUT" + SPLIT_MATRIX=$(python3 tools/trace_replay/trace_cases.py --ci --format github-split-matrix) + # AND IT MUST NOT BE EMPTY. An empty `include` is not an error to GitHub - it skips the + # whole retrace-split job with no red anywhere - so the one way this subset can vanish + # silently is guarded here. trace_cases.py now also rejects an unknown manifest key, which + # was the hole: `"splitt": true` loaded clean and emptied the subset (review N-4). + if [ "$(printf '%s' "${SPLIT_MATRIX}" | python3 -c 'import json,sys; print(len(json.load(sys.stdin)["include"]))')" -lt 1 ]; then + echo "::error::the split retrace subset is EMPTY. No case in trace_cases.json carries \"split\": true, so retrace-split would be skipped with no red. Exit gate E2 names OpenRA." + exit 1 + fi + echo "split-matrix=${SPLIT_MATRIX}" >> "$GITHUB_OUTPUT" trace-fixtures: name: trace fixture (${{ matrix.case }}) @@ -1787,11 +1841,14 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 240 needs: + - build-linux - build-linux-split - build-retrace - trace-cases - trace-fixtures - if: ${{ always() && needs.build-linux-split.result == 'success' && needs.build-retrace.result == 'success' && needs.trace-cases.result == 'success' }} + # build-linux is needed for the negative control ONLY: its pull library is what the control + # swaps in to prove the transport-resolution assertion is load-bearing. + if: ${{ always() && needs.build-linux.result == 'success' && needs.build-linux-split.result == 'success' && needs.build-retrace.result == 'success' && needs.trace-cases.result == 'success' }} strategy: fail-fast: false max-parallel: 4 @@ -1839,6 +1896,18 @@ jobs: name: mobilegl-trace-replay path: . + # The PULL runtime, for the negative control at the end of this job and for nothing else. + - name: Download Linux pull runtime (negative control) + uses: actions/download-artifact@v8 + with: + name: mobilegl-linux-runtime + path: pull-runtime + + - name: Unpack the pull runtime (negative control) + run: | + tar -xzf pull-runtime/mobilegl-linux-runtime.tgz -C pull-runtime + test -f pull-runtime/build-linux/libMobileGL.so + - name: Unpack the SPLIT runtime as the library under test # build-retrace's CTestTestfile.cmake has the absolute path # /build-linux/libMobileGL.so frozen into every case, so the swap happens here @@ -1870,6 +1939,58 @@ jobs: ctest -V --no-tests=error --timeout 10800 \ -R '^MobileGLTraceReplay\.${{ matrix.case }}\.${{ matrix.backend }}$' + # THE RETRACE LANE'S OWN ALWAYS-ON NEGATIVE CONTROL, and its absence was review finding M-6: + # this job was a clone of retrace-verify with the one step removed that makes the lane mean + # anything. retrace-verify carries "a corrupted snapshot field must red this retrace" so that + # "79 traces, zero divergences" is not a statement about a comparator nobody watched; the + # same sentence applies here word for word. + # + # The control swaps the PULL library into the frozen path and requires the SAME replay to + # fail. It is the sharpest one available, because OpenRA scores ssim 1.000000 either way - + # measured - so this step fails if and only if run_trace_case.cmake's transport-resolution + # assertion has stopped working, which is the single thing standing between this job and a + # green that ran monolith end to end. + # + # NOT the control BRIEF 7 E2 names ("patch the Clear emitter to drop one emission; SSIM must + # fall below threshold"). That one needs an emitter, i.e. package c1, and it is carried as an + # explicit debt in t1-v1.md rather than silently substituted - which is what the first + # version of this package did. + # + # The rerun replays into the same case directory, so the good run's images are put aside and + # restored whichever way the control goes; "Upload actual image" below runs `if: always()` + # and would otherwise ship the deliberately-wrong run's output under the good run's name. + - name: Negative control - the PULL library must red this split retrace + working-directory: build-retrace/tools/trace_replay + run: | + set +e + GOOD_OUTPUT="${RUNNER_TEMP}/split-verified-output" + rm -rf "${GOOD_OUTPUT}" + if [ -d "${{ matrix.case }}" ]; then cp -a "${{ matrix.case }}" "${GOOD_OUTPUT}"; fi + # The pull library, unpacked from build-linux's artifact, over the frozen path every + # case has baked in. It defines no MG_Remote symbol, so ConfigLoader has no transport + # parser and MOBILEGL_TRANSPORT=inproc is accepted and ignored - the exact shape of "the + # split lane ran monolith". + cp "${GITHUB_WORKSPACE}/pull-runtime/build-linux/libMobileGL.so" \ + "${GITHUB_WORKSPACE}/build-linux/libMobileGL.so" + if nm --defined-only "${GITHUB_WORKSPACE}/build-linux/libMobileGL.so" | grep -q -i MG_Remote; then + echo "::error::the control's own library defines MG_Remote symbols, so it is not a pull build and this control would prove nothing" + exit 1 + fi + export MOBILEGL_TRANSPORT=inproc + ctest -V --no-tests=error --timeout 10800 \ + -R "^MobileGLTraceReplay\.${{ matrix.case }}\.${{ matrix.backend }}$" + control_rc=$? + set -e + if [ -d "${GOOD_OUTPUT}" ]; then + rm -rf "${{ matrix.case }}"; mv "${GOOD_OUTPUT}" "${{ matrix.case }}" + echo "restored the verified run's output over the control's" + fi + if [ "${control_rc}" -eq 0 ]; then + echo "::error::a PULL library passed the split retrace. OpenRA scores ssim 1.000000 under a monolith library too (measured), so the picture is not and cannot be this lane's gate - run_trace_case.cmake's transport-resolution assertion is, and it has stopped working. Every green in this job is then a monolith run under a name that says split." + exit 1 + fi + echo "the pull library turned the split retrace red, as it must (ctest exit ${control_rc})" + # The refusal census, recorded rather than gated. run_trace_case.cmake already REDS the case # on any Fatal{, so reaching here means the count is zero - but the number and the distinct # slot names are what MEASUREMENTS wants from every split run, and reading them out of the diff --git a/tools/trace_replay/run_trace_case.cmake b/tools/trace_replay/run_trace_case.cmake index 394867a8..f684c8e3 100644 --- a/tools/trace_replay/run_trace_case.cmake +++ b/tools/trace_replay/run_trace_case.cmake @@ -252,17 +252,41 @@ if(DEFINED ENV{MOBILEGL_TRANSPORT} AND NOT "$ENV{MOBILEGL_TRANSPORT}" STREQUAL " "retrace with no library log cannot be counted as a split retrace.") else() file(READ "${mobilegl_log}" split_log) - string(FIND "${split_log}" "MOBILEGL_TRANSPORT=inproc" split_armed_at) + # THE DISTINCTIVE PART OF ConfigLoader's INFO LINE, not the bare KEY=VALUE - review finding + # M-5. ConfigLoader.cpp:71 logs `Config: Accepted env variable: %s=%s` for EVERY MOBILEGL_* + # variable, unconditionally, in every build including the pull one. That line is MGLOG_D, + # so at the INFO level CI and the gate use it is compiled out - but at + # MOBILEGL_LOG_ACTIVE_LEVEL=..._DEBUG it reads `Config: Accepted env variable: + # MOBILEGL_TRANSPORT=inproc` and satisfied the old substring search. Observed GREEN on a + # crafted pull-build log. The person most likely to hit that is the one who rebuilds at + # DEBUG to debug a split failure. The sentence below exists only in + # ConfigLoader::InitTransport's InProcess arm, which exists only under + # MOBILEGL_BUILD_DISAGGREGATED. + set(split_expected_marker "MOBILEGL_TRANSPORT=inproc - the MGPipe record stream") + string(FIND "${split_log}" "${split_expected_marker}" split_armed_at) if(split_armed_at EQUAL -1) + # Say which transport was actually asked for. ConfigLoader REFUSES spawn / unix: / + # pipe: BY NAME and stays on monolith (they are P6's), so a run that set one of those + # has a different diagnosis from one that set inproc against a monolith library, and + # the old message named `inproc` either way. + set(split_refusal "") + if(NOT "$ENV{MOBILEGL_TRANSPORT}" STREQUAL "inproc") + set(split_refusal + " NOTE: this run asked for '$ENV{MOBILEGL_TRANSPORT}', which P5 does not " + "implement - ConfigLoader recognises spawn / unix: / pipe: and REFUSES them by " + "name, staying on monolith. Only 'inproc' can resolve in P5.") + endif() message(FATAL_ERROR "MOBILEGL_TRANSPORT=$ENV{MOBILEGL_TRANSPORT} is set for ${split_case} and the library " - "never reported resolving it. ConfigLoader::InitTransport logs one line at INFO when " - "it selects InProcess, and that line exists only in a build configured with " + "never reported resolving it: ${mobilegl_log} carries no " + "\"${split_expected_marker}\". ConfigLoader::InitTransport logs that line at INFO " + "when it selects InProcess, and it exists only in a build configured with " "-DMOBILEGL_BUILD_DISAGGREGATED=ON - in a build without it the whole parser is " "compiled out and the variable is accepted and ignored, which is exactly the 'the " "split lane ran monolith and went green' failure. Check that the SPLIT runtime " "artifact is the one at ${MOBILEGL_LIBRARY}, and that MOBILEGL_LOG_ACTIVE_LEVEL " - "admits INFO.") + "admits INFO (at WARN or above the line is compiled out and this reds for no " + "defect).${split_refusal}") endif() # The refusal census. Recorded on every split run, pass or fail. file(STRINGS "${mobilegl_log}" split_fatals REGEX "Fatal\\{") diff --git a/tools/trace_replay/trace_cases.py b/tools/trace_replay/trace_cases.py index 75f944e1..204fa5b4 100644 --- a/tools/trace_replay/trace_cases.py +++ b/tools/trace_replay/trace_cases.py @@ -8,11 +8,48 @@ from pathlib import Path TRACE_CASES_JSON = Path(__file__).with_name("trace_cases.json") CI_BACKENDS = ("DirectGLES", "DirectVulkan") +# Every key a case or the defaults block may carry. An UNKNOWN key is a hard error rather than a +# silent no-op, which is review finding N-4: `"split": true` mistyped as `"splitt": true` loaded +# clean, the split subset became [], the GitHub matrix became {"include":[]}, and `retrace-split` +# was skipped with no red anywhere. Every other way of getting `split` wrong already raised +# (`"ci": false`, a backend list without DirectGLES, a non-bool value) - the typo was the one hole, +# and it is the shape that makes a whole CI job quietly stop existing. +# +# Adding a key means adding it here, deliberately, in the same commit. That is the point. +KNOWN_CASE_KEYS = frozenset({ + "name", + "trace_archive", + "trace_file", + "golden", + "alternate_golden", + "target_call", + "width", + "height", + "ssim_threshold", + "crop_x", + "crop_y", + "crop_width", + "crop_height", + "coherent_as_flush", + "timeout_seconds", + "ci", + "ci_backends", + "verify", + "split", + "avoid_angle_llvmpipe_explicit_lod_bias", +}) + def load_trace_case_manifest(path=TRACE_CASES_JSON): with Path(path).open("r", encoding="utf-8") as file: manifest = json.load(file) defaults = manifest.get("defaults", {}) + unknown_defaults = sorted(set(defaults) - KNOWN_CASE_KEYS) + if unknown_defaults: + raise ValueError( + f"unknown key(s) in the defaults block: {', '.join(unknown_defaults)}. " + f"Known keys are {', '.join(sorted(KNOWN_CASE_KEYS))}" + ) cases = [] seen = set() for case in manifest.get("cases", []): @@ -20,6 +57,13 @@ def load_trace_case_manifest(path=TRACE_CASES_JSON): name = merged.get("name") if not name: raise ValueError("trace case is missing name") + unknown = sorted(set(case) - KNOWN_CASE_KEYS) + if unknown: + raise ValueError( + f"unknown key(s) for {name}: {', '.join(unknown)}. A mistyped flag loads clean and " + f"turns its whole CI subset into an empty matrix, which GitHub skips with no red. " + f"Known keys are {', '.join(sorted(KNOWN_CASE_KEYS))}" + ) if name in seen: raise ValueError(f"duplicate trace case: {name}") seen.add(name)