diff --git a/scripts/p4a_descriptor_negative_control.sh b/scripts/p4a_descriptor_negative_control.sh old mode 100755 new mode 100644 index 51abd3d4..c0b621e3 --- a/scripts/p4a_descriptor_negative_control.sh +++ b/scripts/p4a_descriptor_negative_control.sh @@ -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 # # 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:
@@@. 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)" "$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 "$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)" "$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 "$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)" "$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 "$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 <