mirror of
https://github.com/MobileGL-Dev/MobileGL
synced 2026-09-09 20:58:31 +09:00
[Fix] (Test): make G7's descriptor negative control able to report success - a patch that compiles into a scoped enum, a verdict its own patcher cannot pollute, and repair state the signal traps can see
This commit is contained in:
rereview
Executable → Regular
+123
-33
@@ -25,6 +25,12 @@
|
||||
# plausible rather than an error. It is also the field with the least other coverage: the
|
||||
# surface's Res, Level and Layer are all still right, so nothing about the ATTACHMENT changes
|
||||
# except the one bit that says how much of the texture it is.
|
||||
# TWO MECHANISMS, NOT ONE, on a tree that carries the framebuffer emitter (review F-m8): the
|
||||
# regex also matches the field's copy inside the ContentHash staging helper, so the run drops
|
||||
# Layered from the emitted record AND from the framebuffer content hash. The suite still goes
|
||||
# red naming the field, which is what the control asks; the sentence above is narrowed here
|
||||
# rather than in the code because excluding the hash copy would mean hard-coding another
|
||||
# package's helper name into a regex that deliberately does not know one.
|
||||
# 2. SamplerParameters::borderColorForm stops being copied in MG_Impl/Pipe/SamplerEmit.h. All
|
||||
# four border-colour VALUES still cross; what is lost is which of the three forms
|
||||
# (float / int / uint) they are to be read as, and D-F4 is explicit that the form crosses and
|
||||
@@ -39,6 +45,21 @@
|
||||
# assign its field at all, because the emitter has not landed yet or because the conversion is
|
||||
# spelled some other way, that is exit 2, "could not run", never a pass.
|
||||
#
|
||||
# THE CONSTANT IS `{}` AND NOT `0`, and the one character is the difference between a control that
|
||||
# can answer and one that cannot (review F-M1). SamplerParameters::borderColorForm is a SCOPED enum
|
||||
# (MG_Pipe/MGPipeValueTypes.h, `enum class BorderColorForm : Uint8`), and `x = 0` on one is
|
||||
# `cannot convert 'int' to 'BorderColorForm' in assignment` - the patched header does not compile,
|
||||
# the script takes its "the patched header did not compile" path, and the control reports
|
||||
# could-not-run FOREVER, on every tree, with a summary line saying that exit 2 is the expected
|
||||
# answer here. `x = {}` is valid for the scoped enum AND for MGPSurface::Layered (a Uint8), so ONE
|
||||
# replacement covers both controls and neither of them needs to know its field's type.
|
||||
#
|
||||
# A CONTROL'S FIELD MUST MATTER IN AT LEAST ONE CASE. `{}` is the type's zero, so a suite whose
|
||||
# every case happens to expect the zero value of the field would stay green with the copy dropped -
|
||||
# which this script would then report, correctly, as "the negative control did not trip". Both
|
||||
# suites' headers say they must fail BY FIELD NAME, and both drive a non-default value; that is the
|
||||
# owning package's contract, and this script is what checks it rather than assuming it.
|
||||
#
|
||||
# ON THE P4a CONTRACT TREE THIS SCRIPT EXITS 2 AND SAYS SO. Both headers EXIST there - the contract
|
||||
# commit creates all five emit headers with STUB emitters that return 0 payload bytes (contract-v1
|
||||
# D1) - but neither assigns anything, so there is no field copy to drop. That is the honest report:
|
||||
@@ -52,7 +73,11 @@
|
||||
# scripts/p4a_descriptor_negative_control.sh <build-dir>
|
||||
#
|
||||
# <build-dir> a configured build directory carrying the push-only unit suites (the emission
|
||||
# cases are compiled only under MOBILEGL_PIPE_PUSH)
|
||||
# cases are compiled only under MOBILEGL_PIPE_PUSH). It is CHECKED, not assumed:
|
||||
# in a pull build every emission case is a visible skip, ctest is green before and
|
||||
# after the patch, and the run would record "the negative control did not trip" -
|
||||
# a finding about the suite that is really a finding about the build directory
|
||||
# (review F-m10). MOBILEGL_PIPE_PUSH is read out of the directory's CMakeCache.txt.
|
||||
#
|
||||
# RESTORE IS NOT ENOUGH; THE REBUILD IS PART OF THE CONTRACT. Once a header has been patched, EVERY
|
||||
# way out of this script goes through repair(): restore the header, rebuild the library from it, and
|
||||
@@ -96,6 +121,35 @@ REPO_ROOT=$(cd "$(dirname "$0")/.." && pwd) || exit 2
|
||||
cd "$REPO_ROOT" || exit 2
|
||||
[ -f "$BUILD_DIR/CMakeCache.txt" ] || { echo "$BUILD_DIR is not a configured build directory" >&2; exit 2; }
|
||||
|
||||
# F-m10: a PULL build directory would take both controls through "did-not-trip", which reads as a
|
||||
# finding about the suites and is a finding about the argument. Every emission case is
|
||||
# `#if MOBILEGL_PIPE_PUSH` and a visible skip otherwise, so ctest is green before the patch and
|
||||
# green after it, and the script would report a defect in somebody else's test suite.
|
||||
#
|
||||
# BOTH CACHE ENTRIES ARE READ, because MOBILEGL_PIPE_VERIFY=ON forces MOBILEGL_PIPE_PUSH on for
|
||||
# the configure WITHOUT writing it back to the cache (CMakeLists.txt: the `set(... ON)` shadows the
|
||||
# cached OFF). A verify build directory therefore compiles the emission cases while its cache still
|
||||
# says MOBILEGL_PIPE_PUSH:BOOL=OFF, and refusing it would be exactly the wrong answer.
|
||||
mgl_cache_is_on() {
|
||||
local line
|
||||
line=$(grep -m1 "^$1:" "$BUILD_DIR/CMakeCache.txt" 2>/dev/null) || return 1
|
||||
case "${line#*=}" in
|
||||
ON|On|on|1|TRUE|True|true|YES|Yes|yes|Y|y) return 0 ;;
|
||||
*) return 1 ;;
|
||||
esac
|
||||
}
|
||||
if ! mgl_cache_is_on MOBILEGL_PIPE_PUSH && ! mgl_cache_is_on MOBILEGL_PIPE_VERIFY; then
|
||||
echo "$BUILD_DIR does not carry the push-only emission cases:" >&2
|
||||
grep -E '^MOBILEGL_PIPE_(PUSH|VERIFY):' "$BUILD_DIR/CMakeCache.txt" >&2 || \
|
||||
echo " (its CMakeCache.txt names neither MOBILEGL_PIPE_PUSH nor MOBILEGL_PIPE_VERIFY)" >&2
|
||||
echo "Every case in MG_Test/Pipe/{Framebuffer,Sampler}EmitTest.cpp is #if MOBILEGL_PIPE_PUSH and" >&2
|
||||
echo "a visible skip otherwise, so here ctest is green with the field dropped as well as without" >&2
|
||||
echo "it and this script would report 'the negative control did not trip' about a suite that" >&2
|
||||
echo "never ran - a finding about the build directory dressed up as a finding about the test." >&2
|
||||
echo "Point it at the push (or verify) build directory." >&2
|
||||
exit 2
|
||||
fi
|
||||
|
||||
# One row per control: <header>@<field>@<ctest regex>@<owning package>. The ctest regex is the
|
||||
# SHORTEST string that selects only that suite, the way the CI filters are written.
|
||||
CONTROLS="\
|
||||
@@ -128,13 +182,13 @@ repair() {
|
||||
# dirty tree must get their own tree back, not HEAD.
|
||||
if [ -f "$backup" ]; then cp -f "$backup" "$header" || { REPAIR_RC=2; return 2; }; fi
|
||||
say "restored $header; rebuilding $BUILD_DIR from it"
|
||||
if ! cmake --build "$BUILD_DIR" -j "$(nproc)" > "$LOG_DIR/build-restored.log" 2>&1; then
|
||||
if ! cmake --build "$BUILD_DIR" -j "$(nproc)" </dev/null > "$LOG_DIR/build-restored.log" 2>&1; then
|
||||
say "the tree did NOT rebuild after the restore - see $LOG_DIR/build-restored.log"
|
||||
say "THE BUILD DIRECTORY IS NOT TRUSTWORTHY: repair it before reading anything out of it."
|
||||
REPAIR_RC=2
|
||||
return 2
|
||||
fi
|
||||
if ! ctest --test-dir "$BUILD_DIR" -R "$test" --no-tests=error \
|
||||
if ! ctest --test-dir "$BUILD_DIR" -R "$test" --no-tests=error </dev/null \
|
||||
> "$LOG_DIR/ctest-restored.log" 2>&1; then
|
||||
say "the tree did NOT go back to green after the restore - see $LOG_DIR/ctest-restored.log"
|
||||
REPAIR_RC=2
|
||||
@@ -154,12 +208,32 @@ trap 'repair; trap - INT; kill -INT $$' INT
|
||||
trap 'repair; trap - TERM; kill -TERM $$' TERM
|
||||
|
||||
# --- one control -----------------------------------------------------------------------------
|
||||
# Echoes "tripped" / "did-not-trip" / "wrong-reason" / "could-not-run" on stdout; everything else
|
||||
# goes to stderr. The tree is repaired before it returns, whatever the answer.
|
||||
# Sets the GLOBAL CONTROL_VERDICT to "tripped" / "did-not-trip" / "wrong-reason" /
|
||||
# "could-not-run"; everything it has to say goes to stderr. The tree is repaired before it
|
||||
# returns, whatever the answer.
|
||||
#
|
||||
# A GLOBAL AND NOT AN ECHO, AND IT IS CALLED PLAINLY AND NOT IN `$(...)` (review F-M2 and F-M3,
|
||||
# two separate defects that the same change closes). The earlier form was
|
||||
# `verdict=$(run_control ...)`, and a command substitution is a SUBSHELL:
|
||||
#
|
||||
# * F-M2 - anything the function or anything it called wrote to stdout became part of
|
||||
# `$verdict`. The patcher's own success line did, so on a tree where a control really tripped
|
||||
# the verdict was a TWO-LINE string, the `case` fell through to `*)`, and the run scored a
|
||||
# control that had answered perfectly as "a control did not answer". Exit 0 was unreachable.
|
||||
# The patcher now writes to stderr as well (belt and braces), but the verdict no longer
|
||||
# travels through a stream that anything else can write to, which is the actual fix.
|
||||
# * F-M3 - PATCHED_HEADER was assigned INSIDE that subshell. Bash resets a script's traps in a
|
||||
# command substitution, so the subshell had no traps to fire, and its assignments never
|
||||
# reached the parent, so the parent's EXIT/INT/TERM traps ran with PATCHED_HEADER empty and
|
||||
# repair() returned 0 immediately. A Ctrl-C during a rebuild left the patched header in the
|
||||
# tree - the exact thing the header's own paragraph promises cannot happen, and the p3a-g7 m3
|
||||
# lesson it cites. Called plainly, the state is the parent's and the traps repair.
|
||||
CONTROL_VERDICT=""
|
||||
run_control() {
|
||||
local header=$1 field=$2 test=$3 owner=$4
|
||||
local tag matched
|
||||
tag=$(basename "$header" .h)-$field
|
||||
CONTROL_VERDICT=could-not-run
|
||||
|
||||
# 0. the control has to have something to break.
|
||||
if [ ! -f "$header" ]; then
|
||||
@@ -167,7 +241,7 @@ run_control() {
|
||||
say "It is P4a package $owner's file (BRIEF-P4A.md C.5/C.7): the client-side emitter that"
|
||||
say "carries the ${field} copy. Until it lands there is no field copy to drop, so this control"
|
||||
say "cannot run and MUST NOT report a pass. Re-run on a tree that carries that package."
|
||||
echo could-not-run
|
||||
CONTROL_VERDICT=could-not-run
|
||||
return
|
||||
fi
|
||||
if ! grep -qE "\.${field}[[:space:]]*=" "$header"; then
|
||||
@@ -179,7 +253,7 @@ run_control() {
|
||||
say "at all, and the second one would mean G6 is already broken in exactly the way this control"
|
||||
say "is supposed to create. Neither is something this script may report as a pass; look at the"
|
||||
say "header."
|
||||
echo could-not-run
|
||||
CONTROL_VERDICT=could-not-run
|
||||
return
|
||||
fi
|
||||
|
||||
@@ -191,23 +265,23 @@ run_control() {
|
||||
say "no test matches $test in $BUILD_DIR."
|
||||
say "The suite is registered by the P4a contract commit; until it carries the emission cases"
|
||||
say "this control has nothing to trip and cannot report a pass."
|
||||
echo could-not-run
|
||||
CONTROL_VERDICT=could-not-run
|
||||
return
|
||||
fi
|
||||
say "[$tag] $matched matching test(s) before the patch"
|
||||
|
||||
say "[$tag] building $BUILD_DIR as it is"
|
||||
if ! cmake --build "$BUILD_DIR" -j "$(nproc)" > "$LOG_DIR/build-before-$tag.log" 2>&1; then
|
||||
if ! cmake --build "$BUILD_DIR" -j "$(nproc)" </dev/null > "$LOG_DIR/build-before-$tag.log" 2>&1; then
|
||||
say "[$tag] the build is already broken before any patch - see $LOG_DIR/build-before-$tag.log"
|
||||
tail -20 "$LOG_DIR/build-before-$tag.log" >&2
|
||||
echo could-not-run
|
||||
CONTROL_VERDICT=could-not-run
|
||||
return
|
||||
fi
|
||||
if ! ctest --test-dir "$BUILD_DIR" -R "$test" --no-tests=error --output-on-failure \
|
||||
if ! ctest --test-dir "$BUILD_DIR" -R "$test" --no-tests=error --output-on-failure </dev/null \
|
||||
> "$LOG_DIR/ctest-before-$tag.log" 2>&1; then
|
||||
say "[$tag] $test is already red before the patch - fix that first, the control proves nothing"
|
||||
tail -30 "$LOG_DIR/ctest-before-$tag.log" >&2
|
||||
echo could-not-run
|
||||
CONTROL_VERDICT=could-not-run
|
||||
return
|
||||
fi
|
||||
say "[$tag] $test is green before the patch"
|
||||
@@ -215,7 +289,7 @@ run_control() {
|
||||
# 2. stop copying the field.
|
||||
PATCHED_BACKUP=$LOG_DIR/$(basename "$header").orig
|
||||
PATCHED_TEST=$test
|
||||
cp -f "$header" "$PATCHED_BACKUP" || { echo could-not-run; return; }
|
||||
cp -f "$header" "$PATCHED_BACKUP" || { CONTROL_VERDICT=could-not-run; return; }
|
||||
# Set BEFORE the patcher runs, not after: a python that died half-way through the write must
|
||||
# still be repaired. The cost is one unnecessary rebuild in the case where the patcher matched
|
||||
# nothing and the file is byte-identical (cp refreshes its mtime).
|
||||
@@ -231,32 +305,38 @@ text = open(path, encoding='utf-8').read()
|
||||
# side is REPLACED rather than the line deleted, so the record still HAS the field and the break
|
||||
# stays one the compiler cannot see.
|
||||
pattern = re.compile(r'(\.' + re.escape(field) + r'\s*=\s*)([^;,\n]+)([;,])')
|
||||
patched, count = pattern.subn(r'\g<1>0 /* G7 NEGATIVE CONTROL: was \g<2> */\g<3>', text)
|
||||
# `{}` and not `0`: borderColorForm is a SCOPED enum (enum class BorderColorForm : Uint8) and
|
||||
# `= 0` does not convert, so the patched header would never compile and this control could
|
||||
# never answer (review F-M1). `= {}` is valid for the scoped enum and for MGPSurface::Layered's
|
||||
# Uint8 alike, in an assignment and in a designated initializer, so one string covers both.
|
||||
patched, count = pattern.subn(r'\g<1>{} /* G7 NEGATIVE CONTROL: was \g<2> */\g<3>', text)
|
||||
if count == 0:
|
||||
sys.stderr.write('[p4a-g7] no assignment to .%s to patch - the header changed shape since this '
|
||||
'control was written; update the control, do not delete it.\n' % field)
|
||||
sys.exit(1)
|
||||
open(path, 'w', encoding='utf-8', newline='\n').write(patched)
|
||||
print('[p4a-g7] neutralised %d assignment(s) to .%s' % (count, field))
|
||||
# STDERR, like every other diagnostic here: this used to be the one line in the file that
|
||||
# went to stdout, and stdout was the channel the caller captured the verdict through.
|
||||
sys.stderr.write('[p4a-g7] neutralised %d assignment(s) to .%s\n' % (count, field))
|
||||
PY
|
||||
then
|
||||
say "[$tag] the patcher did not apply; the tree is repaired on the way out"
|
||||
repair || true
|
||||
echo could-not-run
|
||||
CONTROL_VERDICT=could-not-run
|
||||
return
|
||||
fi
|
||||
|
||||
# 3. it must still COMPILE. A build break here would prove the static_asserts work, not that the
|
||||
# suite still checks.
|
||||
say "[$tag] rebuilding with the dropped field"
|
||||
if ! cmake --build "$BUILD_DIR" -j "$(nproc)" > "$LOG_DIR/build-after-$tag.log" 2>&1; then
|
||||
if ! cmake --build "$BUILD_DIR" -j "$(nproc)" </dev/null > "$LOG_DIR/build-after-$tag.log" 2>&1; then
|
||||
say "[$tag] the patched header did not compile, so the control cannot tell 'the test failed'"
|
||||
say "from 'nothing was built'. The break is supposed to be invisible to the compiler - if the"
|
||||
say "field is read somewhere that needs its value, say so in the control rather than working"
|
||||
say "around it."
|
||||
grep -m10 -E 'error:' "$LOG_DIR/build-after-$tag.log" >&2
|
||||
repair || true
|
||||
echo could-not-run
|
||||
CONTROL_VERDICT=could-not-run
|
||||
return
|
||||
fi
|
||||
say "[$tag] the patched header still compiles, so the record still has the field and its size"
|
||||
@@ -264,11 +344,10 @@ PY
|
||||
# 4. the suite must now be RED, and NAME the field. The verdict is only RECORDED here; nothing is
|
||||
# reported and nothing returns until step 5 has put the tree back, because all three outcomes
|
||||
# leave the same corrupted build directory behind.
|
||||
local verdict
|
||||
say "[$tag] running $test against the dropped field"
|
||||
if ctest --test-dir "$BUILD_DIR" -R "$test" --no-tests=error --output-on-failure \
|
||||
if ctest --test-dir "$BUILD_DIR" -R "$test" --no-tests=error --output-on-failure </dev/null \
|
||||
> "$LOG_DIR/ctest-after-$tag.log" 2>&1; then
|
||||
verdict=did-not-trip
|
||||
CONTROL_VERDICT=did-not-trip
|
||||
elif awk '/: Failure$/ || /: error:/ { block = 1 } block { print } /^[[:space:]]*$/ { block = 0 }' \
|
||||
"$LOG_DIR/ctest-after-$tag.log" | grep -q "$field"; then
|
||||
# A red is not yet a pass: a suite that had started failing for an unrelated reason satisfies
|
||||
@@ -279,21 +358,22 @@ PY
|
||||
# line, is the right unit and the whole file is the wrong one. A case NAMED after the field, a
|
||||
# skip reason quoting it or a compiler note echoed into the log would all have made "tripped"
|
||||
# mean "the string exists somewhere in the output".
|
||||
verdict=tripped
|
||||
CONTROL_VERDICT=tripped
|
||||
else
|
||||
verdict=wrong-reason
|
||||
CONTROL_VERDICT=wrong-reason
|
||||
fi
|
||||
|
||||
# 5. put it back, and prove it went back. SHARED BY ALL THREE OUTCOMES, and that is the whole
|
||||
# point of doing it before the verdict is reported.
|
||||
if ! repair; then
|
||||
say "[$tag] the control's own verdict was '$verdict', but the repair failed, so that verdict is"
|
||||
say "not what this run reports: a build directory that could not be put back is 'could not run'."
|
||||
echo could-not-run
|
||||
say "[$tag] the control's own verdict was '$CONTROL_VERDICT', but the repair failed, so that"
|
||||
say "verdict is not what this run reports: a build directory that could not be put back is"
|
||||
say "'could not run'."
|
||||
CONTROL_VERDICT=could-not-run
|
||||
return
|
||||
fi
|
||||
|
||||
case "$verdict" in
|
||||
case "$CONTROL_VERDICT" in
|
||||
did-not-trip)
|
||||
say "[$tag] NEGATIVE CONTROL DID NOT TRIP: $test was still green with ${field} no longer"
|
||||
say "copied into the record. The emission comparison did not notice a field it claims to"
|
||||
@@ -310,7 +390,6 @@ PY
|
||||
say "[$tag] negative control tripped, naming $field, and the tree is green again"
|
||||
;;
|
||||
esac
|
||||
echo "$verdict"
|
||||
}
|
||||
|
||||
# --- both controls, then one verdict ----------------------------------------------------------
|
||||
@@ -320,9 +399,22 @@ PY
|
||||
# 1 (did not answer).
|
||||
WORST=0
|
||||
SUMMARY=""
|
||||
while IFS='@' read -r header field test owner; do
|
||||
# The rows are collected FIRST and the loop over them carries no redirection, so that
|
||||
# run_control - which is called plainly now, in this shell, holding this shell's repair state -
|
||||
# cannot have the remaining rows eaten out from under it by a child that reads stdin. (cmake and
|
||||
# ctest are also given /dev/null explicitly below; this is the belt to that pair of braces.)
|
||||
CONTROL_ROWS=()
|
||||
while IFS= read -r mglRow; do
|
||||
[ -n "$mglRow" ] && CONTROL_ROWS+=("$mglRow")
|
||||
done <<EOF
|
||||
$CONTROLS
|
||||
EOF
|
||||
|
||||
for mglRow in "${CONTROL_ROWS[@]}"; do
|
||||
IFS='@' read -r header field test owner <<< "$mglRow"
|
||||
[ -n "$header" ] || continue
|
||||
verdict=$(run_control "$header" "$field" "$test" "$owner")
|
||||
run_control "$header" "$field" "$test" "$owner"
|
||||
verdict=$CONTROL_VERDICT
|
||||
SUMMARY="$SUMMARY
|
||||
$field ($header): $verdict"
|
||||
case "$verdict" in
|
||||
@@ -330,9 +422,7 @@ while IFS='@' read -r header field test owner; do
|
||||
could-not-run) WORST=2 ;;
|
||||
*) [ "$WORST" -eq 2 ] || WORST=1 ;;
|
||||
esac
|
||||
done <<EOF
|
||||
$CONTROLS
|
||||
EOF
|
||||
done
|
||||
|
||||
trap - EXIT
|
||||
say "---- G7 (P4a descriptor emission) ----$SUMMARY"
|
||||
|
||||
Reference in New Issue
Block a user