From badcb790343791cea60aa82f398e1f55017d6d54 Mon Sep 17 00:00:00 2001 From: Toby Murray Date: Mon, 3 Aug 2026 10:39:24 -0400 Subject: [PATCH 1/7] ci(simulator): ratchet the compiler warnings instead of discarding them The gcc simulator build has always asked for -Wall -Wextra -Wformat=2, and we have always thrown the answers away: una/Makefile appends -Wno-error after requesting -Werror, and the log only ever became an uploaded artifact. #247 hand-fixed a printf("%d", size_t) that gcc had already reported 1,596 times in the CI run for the branch that landed its neighbours. At 75 unrelated warning sites, a number that large is indistinguishable from zero. Keys on (path, flag) with a count rather than on line numbers, so editing above a pre-existing warning doesn't churn the baseline, and aggregates across projects with max() rather than sum() so the baseline describes the code and a PR that builds one app can still be checked against it. The gate needs no write token, so it works from a fork. Only a push to main that built every project may lower the baseline, and it may only ever lower it: an increase there means the gate was bypassed and should stay a failure. --- .github/scripts/warning_baseline.py | 566 ++++++++++++++++++++++++++ .github/warning-baseline.txt | 56 +++ .github/workflows/linux-simulator.yml | 221 +++++++++- 3 files changed, 841 insertions(+), 2 deletions(-) create mode 100644 .github/scripts/warning_baseline.py create mode 100644 .github/warning-baseline.txt diff --git a/.github/scripts/warning_baseline.py b/.github/scripts/warning_baseline.py new file mode 100644 index 000000000..2f4bcd58a --- /dev/null +++ b/.github/scripts/warning_baseline.py @@ -0,0 +1,566 @@ +#!/usr/bin/env python3 +"""Ratcheting compiler-warning gate for the gcc simulator builds. + +The simulator build already asks for the warnings we want: every app's una/Makefile +compiles with -Wall -Wextra -Wformat=2 -Wcast-qual and more. Two things then throw +the answers away -- una/Makefile appends -Wno-error to user_cflags after asking for +-Werror, and linux-simulator.yml only ever tees the log into an uploaded artifact +that nothing reads. + +So the diagnostics are printed and ignored. #247 hand-fixed a printf("%d", size_t) +in Mock/AppMemory.hpp that gcc had reported 1,596 times in the very CI run for the +branch that introduced its neighbours -- twice per build, across 15 projects, in a +log with 75 other warning sites for company. A count that large is indistinguishable +from zero if nobody looks. + +This keeps a checked-in baseline of what is currently tolerated, fails a build that +adds to it, and rewrites it downwards on its own when a full build of main finds +fewer. Nothing here enables a new warning; it only stops the existing ones from +being free. + +Warnings are keyed on (repo-relative path, warning flag) with a count, deliberately +NOT on line numbers: a fingerprint that includes the line churns on every edit above +a pre-existing warning. The cost is that swapping one -Wformat= site for another in +the same file passes; the benefit is a baseline that only moves when the warnings do. + +Counts aggregate across projects with max(), not sum(): the same SDK header is +compiled by every app, so a sum would depend on how many projects a run happened to +build. max() makes the baseline a property of the code, which in turn lets a partial +build (a PR touching one app) be checked against a baseline produced by a full one. + +Subcommands: + extract one build log -> normalized counts (TSV on stdout) + check counts vs baseline -> exit 1 if anything is new or higher + update counts -> baseline -> rewrite, exit 0 if it changed, 2 if unchanged + compare two baselines -> exit 1 if the second tolerates more than the first + --selftest run the tests baked into this file, no checkout needed +""" + +import argparse +import os +import posixpath +import re +import sys +import tempfile +import textwrap +import unittest + +# gcc: "path:line:col: warning: message [-Wflag]". The column is optional -- a few +# diagnostics (and any driver-level ones) omit it. +WARNING_RE = re.compile(r"^(?P\S.*?):(?P\d+)(?::(?P\d+))?: warning: (?P.*)$") +FLAG_RE = re.compile(r"\[(-W[A-Za-z0-9=+-]+)\]\s*$") + +# Not our code; upstream's warnings are not ours to ratchet down. +EXCLUDED_PREFIXES = ("ThirdParty/",) + +NO_FLAG = "(unflagged)" + +BASELINE_HEADER = """\ +# Tolerated compiler warnings in the gcc simulator build -- see +# Utilities/Scripts/warning-baseline/warning_baseline.py. +# +# Generated. Do not hand-edit to make a build pass: the number is the count of +# warning sites of that flag in that file, and raising one is how a warning gets +# in. It is rewritten downwards automatically when a push to main builds every +# project and finds fewer. +# +# pathflagcount +""" + + +def normalize_path(raw, base): + """Return the repo-relative path, or None if it is outside the repo or excluded. + + `base` is the repo-relative directory `raw` is relative to: "" for a path the + caller already made workspace-relative by stripping the workspace prefix, and the + make cwd for one gcc emitted relative (`gui/src/...`, `../../Libs/Sources/...`). + """ + path = raw.replace("\\", "/") + + if posixpath.isabs(path): + return None # absolute and not under the workspace: a system header + + resolved = posixpath.normpath(posixpath.join(base, path)) + if resolved == ".." or resolved.startswith("../"): + return None # escaped the repo root + if resolved.startswith(EXCLUDED_PREFIXES): + return None + return resolved + + +def extract(log_path, app_dir, workspace): + """Parse one build log into {(path, flag): count} of distinct warning sites.""" + workspace = workspace.replace("\\", "/").rstrip("/") + sites = set() + + with open(log_path, "r", encoding="utf-8", errors="replace") as fh: + for raw_line in fh: + match = WARNING_RE.match(raw_line.rstrip("\n")) + if not match: + continue + + raw_path = match.group("path") + # A path under the workspace is repo-relative once the prefix is gone; + # only a genuinely relative one is relative to the make cwd. + if workspace and raw_path.startswith(workspace + "/"): + raw_path = raw_path[len(workspace) + 1 :] + base = "" + else: + base = app_dir + + path = normalize_path(raw_path, base) + if path is None: + continue + + # A warning in a header is re-emitted once per translation unit that + # includes it, so dedupe on the site before counting. + sites.add((path, match.group("line"), match.group("col"), match.group("msg"))) + + counts = {} + for path, _line, _col, msg in sites: + flag_match = FLAG_RE.search(msg) + flag = flag_match.group(1) if flag_match else NO_FLAG + counts[(path, flag)] = counts.get((path, flag), 0) + 1 + return counts + + +def read_counts(path): + """Read a TSV of pathflagcount, ignoring comments and blanks.""" + counts = {} + if not os.path.exists(path): + return counts + with open(path, "r", encoding="utf-8") as fh: + for lineno, line in enumerate(fh, 1): + line = line.strip() + if not line or line.startswith("#"): + continue + fields = line.split("\t") + if len(fields) != 3: + sys.exit(f"{path}:{lineno}: expected 3 tab-separated fields, got {len(fields)}") + try: + count = int(fields[2]) + except ValueError: + sys.exit(f"{path}:{lineno}: count is not an integer: {fields[2]!r}") + key = (fields[0], fields[1]) + # max(), not +=, so concatenated per-project files aggregate correctly. + counts[key] = max(counts.get(key, 0), count) + return counts + + +def write_counts(fh, counts, header=""): + if header: + fh.write(header) + for (path, flag), count in sorted(counts.items()): + fh.write(f"{path}\t{flag}\t{count}\n") + + +def merge(files): + merged = {} + for path in files: + for key, count in read_counts(path).items(): + merged[key] = max(merged.get(key, 0), count) + return merged + + +def format_regressions(regressions): + lines = [] + width = max(len(f"{p} [{f}]") for p, f in regressions) if regressions else 0 + for (path, flag), (observed, allowed) in sorted(regressions.items()): + label = f"{path} [{flag}]" + lines.append(f" {label:<{width}} {allowed} allowed -> {observed} found") + return lines + + +def diff_counts(observed, allowed): + """Keys where observed exceeds allowed, as {key: (observed, allowed)}.""" + return { + key: (count, allowed.get(key, 0)) + for key, count in observed.items() + if count > allowed.get(key, 0) + } + + +def cmd_extract(args): + counts = extract(args.log, args.app_dir.strip("/"), args.workspace) + out = open(args.out, "w", encoding="utf-8") if args.out else sys.stdout + try: + write_counts(out, counts) + finally: + if args.out: + out.close() + total = sum(counts.values()) + print(f"{args.log}: {total} warning site(s) in {len(counts)} file/flag pair(s)", file=sys.stderr) + return 0 + + +def cmd_check(args): + baseline = read_counts(args.baseline) + observed = merge(args.counts) + regressions = diff_counts(observed, baseline) + + if not regressions: + print( + f"OK: {sum(observed.values())} warning site(s), none beyond the " + f"{sum(baseline.values())}-site baseline." + ) + return 0 + + new_sites = sum(o - a for o, a in regressions.values()) + print(f"FAIL: {new_sites} new warning site(s) in {len(regressions)} file/flag pair(s):\n") + print("\n".join(format_regressions(regressions))) + print( + "\nFix the warnings. If a warning is genuinely acceptable, raising its count in\n" + f"{args.baseline} is a reviewable change -- not a silent one." + ) + return 1 + + +def cmd_update(args): + old = read_counts(args.baseline) + new = merge(args.counts) + + # Never ratchet up here: this runs on a full build of main, where an increase + # means a warning slipped past the gate and should stay visible as a failure + # rather than being absorbed into the baseline. + regressions = diff_counts(new, old) + if regressions and not args.allow_increase: + print(f"REFUSING to update: {len(regressions)} file/flag pair(s) increased:\n") + print("\n".join(format_regressions(regressions))) + return 1 + + if new == old: + print(f"Baseline unchanged ({sum(old.values())} warning site(s)).") + return 2 + + with open(args.baseline, "w", encoding="utf-8") as fh: + write_counts(fh, new, BASELINE_HEADER) + removed = sum(old.values()) - sum(new.values()) + verb = f"Ratcheted down ({removed} fewer)" if removed > 0 else "Rewrote baseline" + print( + f"{verb}: {sum(old.values())} -> {sum(new.values())} warning site(s), " + f"{len(old)} -> {len(new)} file/flag pair(s)." + ) + return 0 + + +def cmd_compare(args): + """Flag a baseline that got more permissive -- i.e. hand-raised to pass the gate.""" + before = read_counts(args.before) + after = read_counts(args.after) + raised = diff_counts(after, before) + + if not raised: + print("Baseline is not more permissive than the base revision's.") + return 0 + + print(f"Baseline RAISED for {len(raised)} file/flag pair(s):\n") + print("\n".join(format_regressions(raised))) + return 1 + + +REPO_ROOT = os.path.normpath(os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..")) + +# --------------------------------------------------------------------------- tests + +SELFTEST_WORKSPACE = "/home/runner/work/una-sdk/una-sdk" +SELFTEST_APP_DIR = "Examples/Apps/Hiking/Software/Apps/TouchGFX-GUI" + +# One warning per path shape the real build emits, plus the noise the parser has to +# ignore. Trimmed from an actual linux-simulator.yml build.log. +SELFTEST_LOG = textwrap.dedent( + f"""\ + g++ -c -Wall -Wextra -o build/foo.o foo.cpp + In file included from {SELFTEST_WORKSPACE}/Libs/Header/SDK/Simulator/Kernel/Mock/AppMemory.hpp:12: + {SELFTEST_WORKSPACE}/Libs/Header/SDK/Simulator/Kernel/Mock/AppMemory.hpp:30:28: warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t' [-Wformat=] + {SELFTEST_WORKSPACE}/Libs/Header/SDK/Simulator/Kernel/Mock/AppMemory.hpp:42:30: warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t' [-Wformat=] + {SELFTEST_WORKSPACE}/Libs/Header/SDK/Simulator/OS/OS.hpp:71:9: warning: 'OS::Mutex::mHandle' will be initialized after [-Wreorder] + gui/src/main_screen/MainView.cpp:208:73: warning: format '%lu' expects argument of type 'long unsigned int' [-Wformat=] + ../../Libs/Sources/Service.cpp:286:16: warning: ISO C++ prohibits anonymous structs [-Wpedantic] + ../../../../../../ThirdParty/touchgfx/framework/source/platform/hal/simulator/sdl2/HALSDL2.cpp:1335:38: warning: '%02d' directive output may be truncated [-Wformat-truncation=] + {SELFTEST_WORKSPACE}/ThirdParty/coreJSON/source/core_json.c:88:5: warning: unused variable 'x' [-Wunused-variable] + /usr/include/c++/13/bits/stl_algo.h:120:5: warning: some libstdc++ noise [-Wunused-variable] + ::warning::a GitHub Actions annotation, not a compiler warning + make: *** [Makefile:224: build/foo.o] Error 1 + """ +) + + +class SelftestBase(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + + def write(self, name, text): + path = os.path.join(self.tmp.name, name) + with open(path, "w", encoding="utf-8") as fh: + fh.write(text) + return path + + +class TestExtract(SelftestBase): + def setUp(self): + super().setUp() + self.log = self.write("build.log", SELFTEST_LOG) + self.counts = extract(self.log, SELFTEST_APP_DIR, SELFTEST_WORKSPACE) + + def test_workspace_paths_become_repo_relative(self): + self.assertEqual( + self.counts[("Libs/Header/SDK/Simulator/Kernel/Mock/AppMemory.hpp", "-Wformat=")], 2 + ) + self.assertEqual(self.counts[("Libs/Header/SDK/Simulator/OS/OS.hpp", "-Wreorder")], 1) + + def test_cwd_relative_paths_resolve_against_the_app_dir(self): + self.assertEqual( + self.counts[(f"{SELFTEST_APP_DIR}/gui/src/main_screen/MainView.cpp", "-Wformat=")], 1 + ) + self.assertEqual( + self.counts[("Examples/Apps/Hiking/Software/Libs/Sources/Service.cpp", "-Wpedantic")], 1 + ) + + def test_thirdparty_is_excluded_by_either_path_shape(self): + for path, _flag in self.counts: + self.assertNotIn("ThirdParty", path) + + def test_paths_outside_the_repo_are_dropped(self): + for path, _flag in self.counts: + self.assertFalse(path.startswith("/"), path) + self.assertFalse(path.startswith(".."), path) + + def test_non_compiler_lines_are_ignored(self): + # 5 first-party sites; the annotation, the "In file included from" line, the + # compile command and the make error must not register. + self.assertEqual(sum(self.counts.values()), 5) + + def test_a_header_warning_counts_once_per_site_not_per_tu(self): + # The same sites, re-emitted for a second translation unit. + doubled = self.write("doubled.log", SELFTEST_LOG + SELFTEST_LOG) + self.assertEqual(extract(doubled, SELFTEST_APP_DIR, SELFTEST_WORKSPACE), self.counts) + + def test_a_warning_without_a_column_still_parses(self): + log = self.write( + "nocol.log", + f"{SELFTEST_WORKSPACE}/Libs/Source/Simulator/OS/OS.cpp:12: warning: x [-Wpedantic]\n", + ) + self.assertEqual( + extract(log, SELFTEST_APP_DIR, SELFTEST_WORKSPACE), + {("Libs/Source/Simulator/OS/OS.cpp", "-Wpedantic"): 1}, + ) + + def test_a_warning_without_a_flag_lands_in_the_unflagged_bucket(self): + log = self.write( + "noflag.log", + f"{SELFTEST_WORKSPACE}/Libs/Source/Simulator/OS/OS.cpp:12:1: warning: no flag\n", + ) + self.assertEqual( + extract(log, SELFTEST_APP_DIR, SELFTEST_WORKSPACE), + {("Libs/Source/Simulator/OS/OS.cpp", NO_FLAG): 1}, + ) + + def test_line_numbers_are_not_part_of_the_key(self): + """Editing above a pre-existing warning must not churn the baseline.""" + moved = self.write("moved.log", SELFTEST_LOG.replace(":30:28:", ":130:28:")) + self.assertEqual(extract(moved, SELFTEST_APP_DIR, SELFTEST_WORKSPACE), self.counts) + + +class TestBaselineIO(SelftestBase): + def test_round_trip_through_the_file_format(self): + counts = {("Libs/a.cpp", "-Wformat="): 2, ("Libs/b.hpp", "-Wreorder"): 1} + path = os.path.join(self.tmp.name, "baseline.txt") + with open(path, "w", encoding="utf-8") as fh: + write_counts(fh, counts, BASELINE_HEADER) + self.assertEqual(read_counts(path), counts) + + def test_a_missing_baseline_reads_as_empty(self): + self.assertEqual(read_counts(os.path.join(self.tmp.name, "nope.txt")), {}) + + def test_merge_takes_the_max_across_projects_not_the_sum(self): + """The same SDK header is compiled by every app; summing would make the + baseline depend on how many projects a run happened to build.""" + a = self.write("a.tsv", "Libs/x.hpp\t-Wreorder\t2\n") + b = self.write("b.tsv", "Libs/x.hpp\t-Wreorder\t2\n") + self.assertEqual(merge([a, b]), {("Libs/x.hpp", "-Wreorder"): 2}) + + def test_merge_keeps_the_higher_count_when_projects_differ(self): + a = self.write("a.tsv", "Libs/x.hpp\t-Wreorder\t1\n") + b = self.write("b.tsv", "Libs/x.hpp\t-Wreorder\t3\n") + self.assertEqual(merge([a, b]), {("Libs/x.hpp", "-Wreorder"): 3}) + + def test_a_malformed_line_is_a_hard_error(self): + bad = self.write("bad.tsv", "Libs/x.hpp\t-Wreorder\tnot-a-number\n") + with self.assertRaises(SystemExit): + read_counts(bad) + + +class TestCommands(SelftestBase): + def setUp(self): + super().setUp() + self.baseline = self.write( + "baseline.txt", "Libs/a.cpp\t-Wformat=\t2\nLibs/b.hpp\t-Wreorder\t1\n" + ) + + def test_check_passes_when_observed_matches(self): + obs = self.write("o.tsv", "Libs/a.cpp\t-Wformat=\t2\nLibs/b.hpp\t-Wreorder\t1\n") + self.assertEqual(main(["check", "--baseline", self.baseline, obs]), 0) + + def test_check_passes_on_a_partial_build(self): + """A PR touching one app builds only that app, so it legitimately sees a + subset of the baseline. Fewer warnings is never a failure.""" + obs = self.write("o.tsv", "Libs/b.hpp\t-Wreorder\t1\n") + self.assertEqual(main(["check", "--baseline", self.baseline, obs]), 0) + + def test_check_fails_on_a_brand_new_warning(self): + obs = self.write("o.tsv", "Libs/c.cpp\t-Wformat=\t1\n") + self.assertEqual(main(["check", "--baseline", self.baseline, obs]), 1) + + def test_check_fails_on_one_more_of_an_existing_warning(self): + obs = self.write("o.tsv", "Libs/a.cpp\t-Wformat=\t3\n") + self.assertEqual(main(["check", "--baseline", self.baseline, obs]), 1) + + def test_check_fails_on_a_new_flag_in_an_already_warning_file(self): + obs = self.write("o.tsv", "Libs/a.cpp\t-Wswitch\t1\n") + self.assertEqual(main(["check", "--baseline", self.baseline, obs]), 1) + + def test_update_ratchets_down_and_rewrites(self): + obs = self.write("o.tsv", "Libs/a.cpp\t-Wformat=\t1\nLibs/b.hpp\t-Wreorder\t1\n") + self.assertEqual(main(["update", "--baseline", self.baseline, obs]), 0) + self.assertEqual( + read_counts(self.baseline), + {("Libs/a.cpp", "-Wformat="): 1, ("Libs/b.hpp", "-Wreorder"): 1}, + ) + + def test_update_drops_a_key_that_went_to_zero(self): + obs = self.write("o.tsv", "Libs/b.hpp\t-Wreorder\t1\n") + self.assertEqual(main(["update", "--baseline", self.baseline, obs]), 0) + self.assertEqual(read_counts(self.baseline), {("Libs/b.hpp", "-Wreorder"): 1}) + + def test_update_is_idempotent(self): + """Exit 2 means nothing changed, so the workflow skips an empty commit.""" + obs = self.write("o.tsv", "Libs/a.cpp\t-Wformat=\t2\nLibs/b.hpp\t-Wreorder\t1\n") + self.assertEqual(main(["update", "--baseline", self.baseline, obs]), 2) + + def test_update_refuses_to_absorb_an_increase(self): + """On main an increase means a warning slipped past the gate; it has to stay + visible as a failure rather than becoming the new normal.""" + obs = self.write("o.tsv", "Libs/a.cpp\t-Wformat=\t9\n") + self.assertEqual(main(["update", "--baseline", self.baseline, obs]), 1) + self.assertEqual(read_counts(self.baseline)[("Libs/a.cpp", "-Wformat=")], 2) + + def test_update_allows_an_increase_only_when_asked(self): + obs = self.write("o.tsv", "Libs/a.cpp\t-Wformat=\t9\n") + self.assertEqual(main(["update", "--baseline", self.baseline, "--allow-increase", obs]), 0) + + def test_compare_flags_a_hand_raised_baseline(self): + after = self.write("after.txt", "Libs/a.cpp\t-Wformat=\t5\n") + self.assertEqual(main(["compare", "--before", self.baseline, "--after", after]), 1) + + def test_compare_accepts_a_lowered_baseline(self): + after = self.write("after.txt", "Libs/a.cpp\t-Wformat=\t1\n") + self.assertEqual(main(["compare", "--before", self.baseline, "--after", after]), 0) + + def test_extract_writes_the_requested_file(self): + log = self.write("build.log", SELFTEST_LOG) + out = os.path.join(self.tmp.name, "counts.tsv") + argv = ["extract", "--log", log, "--app-dir", SELFTEST_APP_DIR, + "--workspace", SELFTEST_WORKSPACE, "--out", out] + self.assertEqual(main(argv), 0) + self.assertEqual( + read_counts(out)[("Libs/Header/SDK/Simulator/Kernel/Mock/AppMemory.hpp", "-Wformat=")], 2 + ) + + +class TestCommittedBaseline(unittest.TestCase): + """Catches a baseline that has drifted out of sync with the tree. Skipped when + run outside a checkout.""" + + def setUp(self): + self.baseline = os.path.join(REPO_ROOT, ".github", "warning-baseline.txt") + if not os.path.exists(self.baseline): + self.skipTest("no committed baseline") + + def test_it_parses_and_is_not_empty(self): + self.assertGreater(len(read_counts(self.baseline)), 0) + + def test_every_path_still_exists(self): + missing = [ + path + for path, _flag in read_counts(self.baseline) + if not os.path.exists(os.path.join(REPO_ROOT, path)) + ] + self.assertEqual( + missing, + [], + "the baseline names files that no longer exist -- a full build of main will " + "ratchet them away, or drop the lines by hand", + ) + + def test_no_thirdparty_entries(self): + offenders = [p for p, _f in read_counts(self.baseline) if p.startswith("ThirdParty/")] + self.assertEqual(offenders, []) + + +def run_selftest(): + """--selftest runs the tests baked into this file, so a parsing regression is + catchable without an SDK checkout or a simulator build.""" + loader = unittest.TestLoader() + suite = unittest.TestSuite( + loader.loadTestsFromTestCase(case) + for case in (TestExtract, TestBaselineIO, TestCommands, TestCommittedBaseline) + ) + # The command tests print their own diagnostics; keep that off the test report. + with open(os.devnull, "w", encoding="utf-8") as devnull: + real_stdout, sys.stdout = sys.stdout, devnull + try: + result = unittest.TextTestRunner(stream=real_stdout, verbosity=2).run(suite) + finally: + sys.stdout = real_stdout + return 0 if result.wasSuccessful() else 1 + + +# ---------------------------------------------------------------------------- cli + + +def main(argv=None): + argv = list(sys.argv[1:] if argv is None else argv) + if "--selftest" in argv: + return run_selftest() + + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + sub = parser.add_subparsers(dest="command", required=True) + + p = sub.add_parser("extract", help="parse a build log into normalized warning counts") + p.add_argument("--log", required=True, help="build log to parse") + p.add_argument( + "--app-dir", + required=True, + help="repo-relative make cwd, e.g. Examples/Apps/Hiking/Software/Apps/TouchGFX-GUI", + ) + p.add_argument("--workspace", default="", help="absolute repo root to strip from paths") + p.add_argument("--out", help="write here instead of stdout") + p.set_defaults(func=cmd_extract) + + p = sub.add_parser("check", help="fail if counts exceed the baseline") + p.add_argument("--baseline", required=True) + p.add_argument("counts", nargs="+", help="one or more extract outputs") + p.set_defaults(func=cmd_check) + + p = sub.add_parser("update", help="rewrite the baseline from a full build's counts") + p.add_argument("--baseline", required=True) + p.add_argument( + "--allow-increase", + action="store_true", + help="permit a higher baseline (for the initial seeding only)", + ) + p.add_argument("counts", nargs="+", help="one or more extract outputs") + p.set_defaults(func=cmd_update) + + p = sub.add_parser("compare", help="fail if the second baseline tolerates more") + p.add_argument("--before", required=True) + p.add_argument("--after", required=True) + p.set_defaults(func=cmd_compare) + + args = parser.parse_args(argv) + return args.func(args) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/warning-baseline.txt b/.github/warning-baseline.txt new file mode 100644 index 000000000..4cc3f3f08 --- /dev/null +++ b/.github/warning-baseline.txt @@ -0,0 +1,56 @@ +# Tolerated compiler warnings in the gcc simulator build -- see +# Utilities/Scripts/warning-baseline/warning_baseline.py. +# +# Generated. Do not hand-edit to make a build pass: the number is the count of +# warning sites of that flag in that file, and raising one is how a warning gets +# in. It is rewritten downwards automatically when a push to main builds every +# project and finds fewer. +# +# pathflagcount +Docs/Tutorials/Files/Software/Apps/TouchGFX-GUI/gui/src/main_screen/MainView.cpp -Wswitch 4 +Docs/Tutorials/Files/Software/Libs/Sources/Service.cpp -Wpedantic 1 +Docs/Tutorials/HelloWorld/Software/Libs/Sources/Service.cpp -Wpedantic 1 +Docs/Tutorials/Images/Software/Libs/Sources/Service.cpp -Wpedantic 1 +Docs/Tutorials/ScrollMenu/Software/Libs/Sources/Service.cpp -Wpedantic 1 +Docs/Tutorials/Sensors/Software/Apps/TouchGFX-GUI/gui/src/main_screen/MainView.cpp -Wformat= 6 +Docs/Tutorials/Sensors/Software/Apps/TouchGFX-GUI/gui/src/main_screen/MainView.cpp -Wswitch 4 +Docs/Tutorials/Sensors/Software/Libs/Sources/Service.cpp -Wpedantic 1 +Examples/Apps/Alarm/Software/Apps/TouchGFX-GUI/gui/src/edit_screen/EditPresenter.cpp -Wdeprecated-copy 1 +Examples/Apps/Alarm/Software/Apps/TouchGFX-GUI/gui/src/menu_screen/MenuPresenter.cpp -Wdeprecated-copy 2 +Examples/Apps/Cycling/Software/Libs/Sources/ActivitySummarySerializer.cpp -Wformat= 1 +Examples/Apps/Cycling/Software/Libs/Sources/SettingsSerializer.cpp -Wformat= 1 +Examples/Apps/Hiking/Software/Libs/Sources/ActivitySummarySerializer.cpp -Wformat= 1 +Examples/Apps/Hiking/Software/Libs/Sources/SettingsSerializer.cpp -Wformat= 1 +Examples/Apps/Running/Software/Libs/Sources/ActivitySummarySerializer.cpp -Wformat= 1 +Examples/Apps/Running/Software/Libs/Sources/SettingsSerializer.cpp -Wformat= 1 +Examples/Apps/Stopwatch/Software/Apps/TouchGFX-GUI/gui/src/main_screen/MainView.cpp -Wmissing-field-initializers 1 +Examples/Apps/Treadmill/Software/Libs/Sources/ActivitySummarySerializer.cpp -Wformat= 1 +Examples/Apps/Treadmill/Software/Libs/Sources/SettingsSerializer.cpp -Wformat= 1 +Examples/Apps/Workout/Software/Libs/Sources/ActivitySummarySerializer.cpp -Wformat= 1 +Examples/Apps/Workout/Software/Libs/Sources/SettingsSerializer.cpp -Wformat= 1 +Libs/Header/SDK/Simulator/App/KernelMessageDispatcher.hpp -Wreorder 2 +Libs/Header/SDK/Simulator/Components/InstanceSensorLayer.hpp -Wreorder 2 +Libs/Header/SDK/Simulator/Components/Sensors/Gps/GpsDistance.hpp -Wreorder 2 +Libs/Header/SDK/Simulator/Components/Sensors/Gps/GpsSpeed.hpp -Wreorder 2 +Libs/Header/SDK/Simulator/Components/Sensors/IMU/ImuStepCounter.hpp -Wreorder 2 +Libs/Header/SDK/Simulator/Components/Sensors/SensorBatteryLevel.hpp -Wreorder 2 +Libs/Header/SDK/Simulator/Components/Simulator/GpsStepCounterSimulator.hpp -Wreorder 10 +Libs/Header/SDK/Simulator/OS/OS.hpp -Wreorder 2 +Libs/Header/SDK/Tools/FirmwareVersion.hpp -Wpedantic 1 +Libs/Source/JSON/JsonStreamReader.cpp -Wformat= 1 +Libs/Source/JSON/JsonStreamWriter.cpp -Wformat= 2 +Libs/Source/Simulator/App/KernelMessageDispatcher.cpp -Wignored-qualifiers 1 +Libs/Source/Simulator/App/KernelMessageDispatcher.cpp -Wreorder 1 +Libs/Source/Simulator/App/KernelMessageDispatcher.cpp -Wunused-variable 2 +Libs/Source/Simulator/Components/InstanceSensorLayer.cpp -Wreorder 1 +Libs/Source/Simulator/Components/SensorDataQueue.cpp -Wcast-qual 2 +Libs/Source/Simulator/Components/SensorListener.cpp -Wcast-qual 1 +Libs/Source/Simulator/Components/Sensors/Gps/GpsDistance.cpp -Wreorder 1 +Libs/Source/Simulator/Components/Sensors/Gps/GpsDistance.cpp -Wunused-variable 1 +Libs/Source/Simulator/Components/Sensors/Gps/GpsSpeed.cpp -Wreorder 1 +Libs/Source/Simulator/Components/Sensors/Imu/ImuStepCounter.cpp -Wreorder 1 +Libs/Source/Simulator/Components/Sensors/SensorBatteryLevel.cpp -Wreorder 1 +Libs/Source/Simulator/Components/Simulator/GpsStepCounterSimulator.cpp -Wreorder 1 +Libs/Source/Simulator/Components/Simulator/GpsStepCounterSimulator.cpp -Wunused-but-set-variable 1 +Libs/Source/Simulator/OS/OS.cpp -Wreorder 1 +Libs/Source/Variant/VariantConfig.cpp -Wformat-truncation= 1 diff --git a/.github/workflows/linux-simulator.yml b/.github/workflows/linux-simulator.yml index f80a10612..428fa394d 100644 --- a/.github/workflows/linux-simulator.yml +++ b/.github/workflows/linux-simulator.yml @@ -1,4 +1,13 @@ -name: Linux Simulator (non-gating) +name: Linux Simulator + +# The build and headless-boot stages are informational. The warning ratchet is not: +# it is the one job here meant to be a required check. See warning_baseline.py for +# why -- in short, the simulator build already asks for -Wall -Wextra -Wformat=2 and +# then throws the answers into an artifact nobody reads. +# +# NOTE: the baseline commit this workflow pushes to main carries [skip ci], because +# apps-ci, host-tests and tutorials-ci all trigger on an unfiltered push to main and +# would otherwise run a full cycle for a one-line generated file. on: pull_request: @@ -7,6 +16,7 @@ on: - 'ThirdParty/**' - 'Examples/Apps/**' - 'Docs/Tutorials/**' + - '.github/scripts/warning_baseline.py' - '.github/workflows/linux-simulator.yml' push: branches: [ main, develop ] @@ -15,9 +25,13 @@ on: - 'ThirdParty/**' - 'Examples/Apps/**' - 'Docs/Tutorials/**' + - '.github/scripts/warning_baseline.py' - '.github/workflows/linux-simulator.yml' workflow_dispatch: +# Deliberately NOT in the paths filters above: .github/warning-baseline.txt. The +# ratchet job commits it, and a self-trigger would loop. + permissions: contents: read @@ -31,6 +45,7 @@ jobs: outputs: projects: ${{ steps.pick.outputs.projects }} count: ${{ steps.pick.outputs.count }} + total: ${{ steps.pick.outputs.total }} steps: - name: Checkout uses: actions/checkout@v7 @@ -99,6 +114,7 @@ jobs: echo "projects=$SEL" >> "$GITHUB_OUTPUT" echo "count=$(printf '%s' "$SEL" | jq 'length')" >> "$GITHUB_OUTPUT" + echo "total=$TOTAL" >> "$GITHUB_OUTPUT" { echo "### Linux Simulator — selection" echo "" @@ -107,6 +123,32 @@ jobs: printf '%s' "$SEL" | jq -r '.[].dir | " - `" + . + "`"' } >> "$GITHUB_STEP_SUMMARY" + # Raising the baseline is a legitimate escape hatch, but it should never be a + # quiet one -- it is how a warning gets to stay. Surface it; don't block on it. + - name: Note any hand-raised warning baseline + if: github.event_name == 'pull_request' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -uo pipefail + BASELINE=.github/warning-baseline.txt + # Without the base blob every entry would read as newly raised, so skip + # rather than cry wolf. (Absent on the commit that first adds the baseline.) + if ! git show "${BASE_SHA}:${BASELINE}" > /tmp/baseline.before 2>/dev/null; then + echo "No baseline at the base revision; nothing to compare." + exit 0 + fi + [ -f "$BASELINE" ] || : > "$BASELINE" + if ! out="$(python3 .github/scripts/warning_baseline.py compare \ + --before /tmp/baseline.before --after "$BASELINE")"; then + echo "$out" + echo "::warning title=Warning baseline raised::This PR tolerates more compiler warnings than its base. See the job summary." + { echo "### ⚠️ Warning baseline raised"; echo ""; echo '```'; echo "$out"; echo '```'; } \ + >> "$GITHUB_STEP_SUMMARY" + else + echo "$out" + fi + build: needs: discover if: needs.discover.outputs.projects != '[]' @@ -154,13 +196,36 @@ jobs: grep -q "GUI is now running" "$GITHUB_WORKSPACE/sim.log" \ && echo "BOOT OK" || { echo "BOOT FAILED: marker not found"; exit 1; } + # Runs even on a failed build: a build that dies after emitting a new warning + # should still have that warning reach the gate. A truncated log can only + # under-report, so it never causes a false failure. + - name: Extract compiler warnings + if: always() + run: | + set -euo pipefail + [ -f build.log ] || { echo "no build.log; nothing to extract"; exit 0; } + python3 .github/scripts/warning_baseline.py extract \ + --log build.log \ + --app-dir "$APP_DIR" \ + --workspace "$GITHUB_WORKSPACE" \ + --out "warnings-${{ matrix.project.slug }}.tsv" + + - name: Upload warning counts + if: always() + uses: actions/upload-artifact@v7 + with: + name: warning-counts-${{ matrix.project.slug }} + path: warnings-${{ matrix.project.slug }}.tsv + if-no-files-found: ignore + retention-days: 14 + - name: Write PASS/FAIL summary if: always() run: | b='${{ steps.build.outcome }}'; s='${{ steps.smoke.outcome }}' icon() { [ "$1" = success ] && echo '✅' || { [ "$1" = skipped ] && echo '⏭️' || echo '❌'; }; } { - echo "### ${{ matrix.project.name }} — _(informational, non-gating)_" + echo "### ${{ matrix.project.name }} — _(build/boot informational)_" echo "| Stage | Result |" echo "|-------|--------|" echo "| Build | $(icon "$b") \`$b\` |" @@ -180,3 +245,155 @@ jobs: ${{ matrix.project.dir }}/build/bin/simulator.out if-no-files-found: ignore retention-days: 14 + + # The gate. Needs no write permission and no secrets, so it works unchanged on a + # pull request from a fork. + warnings: + needs: [ discover, build ] + # always(), so a failed build leg still has its warnings checked -- but only once + # discover actually produced a selection. + if: always() && needs.discover.result == 'success' && needs.discover.outputs.projects != '[]' + name: Warning ratchet + runs-on: ubuntu-24.04 + timeout-minutes: 5 + steps: + - name: Checkout + uses: actions/checkout@v7 + with: + persist-credentials: false + + - name: Selftest the ratchet itself + run: python3 .github/scripts/warning_baseline.py --selftest + + - name: Download warning counts + uses: actions/download-artifact@v7 + with: + pattern: warning-counts-* + path: counts + merge-multiple: true + + - name: Compare against the baseline + id: gate + run: | + set -uo pipefail + shopt -s nullglob + files=(counts/*.tsv) + echo "legs=${#files[@]}" >> "$GITHUB_OUTPUT" + if [ ${#files[@]} -eq 0 ]; then + echo "::error title=No warning counts::Every build leg failed before producing a log; the ratchet has nothing to check." + exit 1 + fi + echo "Checking ${#files[@]} of ${{ needs.discover.outputs.count }} selected project(s)." + # No `set -e` here, so the status has to be captured and re-raised at the + # end; without the explicit `exit $rc` a failing gate would exit 0 on the + # trailing echo. + python3 .github/scripts/warning_baseline.py check \ + --baseline .github/warning-baseline.txt "${files[@]}" > /tmp/gate.out 2>&1 + rc=$? + cat /tmp/gate.out + if [ $rc -ne 0 ]; then + echo "::error title=New compiler warning::The build added a warning that is not in .github/warning-baseline.txt. See the job summary." + fi + exit $rc + + - name: Summary + if: always() + run: | + ok='${{ steps.gate.outcome }}' + { + if [ "$ok" = success ]; then echo "### ✅ Warning ratchet"; else echo "### ❌ Warning ratchet"; fi + echo "" + echo '```'; cat /tmp/gate.out 2>/dev/null || echo "(no output)"; echo '```' + echo "" + echo "Coverage: ${{ steps.gate.outputs.legs || 0 }} of ${{ needs.discover.outputs.count }} selected project(s), out of ${{ needs.discover.outputs.total }} in the repo." + } >> "$GITHUB_STEP_SUMMARY" + + # Ratchet down on merge. Only ever lowers the baseline, and only from a build that + # covered every project in the repo -- a partial run would look like a fix and + # quietly discard warnings the missing projects would have reported. + ratchet: + needs: [ discover, build, warnings ] + if: >- + github.event_name == 'push' + && github.ref == 'refs/heads/main' + && needs.build.result == 'success' + && needs.warnings.result == 'success' + && needs.discover.outputs.count == needs.discover.outputs.total + name: Ratchet baseline down + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: + contents: write + steps: + - name: Download warning counts + uses: actions/download-artifact@v7 + with: + pattern: warning-counts-* + path: counts + merge-multiple: true + + # Retried in full: another push can land between the checkout and the push, and + # the baseline is generated, so the correct resolution is always "recompute on + # top of the new tip", never a merge. + - name: Lower the baseline if the warnings went away + env: + GH_TOKEN: ${{ github.token }} + run: | + set -uo pipefail + shopt -s nullglob + files=("$GITHUB_WORKSPACE"/counts/*.tsv) + + # An empty or partial set would look exactly like "the warnings were fixed" + # and wipe the baseline. Refuse rather than guess. + expected='${{ needs.discover.outputs.count }}' + if [ ${#files[@]} -ne "$expected" ]; then + echo "::error title=Incomplete warning coverage::Got ${#files[@]} count file(s) for ${expected} project(s); refusing to rewrite the baseline from a partial build." + exit 1 + fi + + for attempt in 1 2 3; do + # cd is guarded throughout: there is no `set -e` here, and a silent cd + # failure would point the rm/add/commit below at the wrong tree. + cd "$GITHUB_WORKSPACE" || exit 1 + rm -rf repo + git clone --quiet --depth 1 --branch main \ + "https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" repo \ + || { echo "::error::clone failed"; exit 1; } + cd repo || exit 1 + + python3 .github/scripts/warning_baseline.py update \ + --baseline .github/warning-baseline.txt "${files[@]}" + rc=$? + case $rc in + 0) ;; # changed; commit it + 2) echo "Baseline already matches; nothing to commit."; exit 0 ;; + *) echo "::error title=Warning count increased on main::A warning reached main without failing the gate. Refusing to absorb it into the baseline." + exit 1 ;; + esac + + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git add .github/warning-baseline.txt + # [skip ci]: apps-ci, host-tests and tutorials-ci trigger on any push to + # main, and this commit is one generated line. + git commit -m "chore(ci): ratchet the simulator warning baseline down [skip ci]" \ + -m "Generated by .github/workflows/linux-simulator.yml from ${GITHUB_SHA}." + + if git push origin main; then + echo "Baseline lowered." + { + echo "### ⬇️ Warning baseline lowered" + echo "" + echo '```' + git --no-pager show --stat --oneline HEAD + echo '```' + } >> "$GITHUB_STEP_SUMMARY" || true + exit 0 + fi + echo "Push rejected (attempt ${attempt}/3) -- main moved. Recomputing." + done + + # Losing the race three times is not a code problem, and the stale baseline + # is the conservative direction: the gate still holds, just one warning + # higher than it could. + echo "::warning title=Baseline not lowered::Lost the push race 3 times. The next merge will lower it." From 4347807642176fdbbcbfa2bc21d1f6c92bcdd40a Mon Sep 17 00:00:00 2001 From: Toby Murray Date: Sat, 8 Aug 2026 21:10:47 -0400 Subject: [PATCH 2/7] fix(ci): review hardening for the warning ratchet (CodeRabbit) The generated baseline header named a path the script has never lived at; it is regenerated verbatim from BASELINE_HEADER, so both copies drifted together. warning-baseline.txt joins the pull_request paths filter. It stays out of the push filter for the reason the old comment gave -- the ratchet commits it to main, and a self-trigger would loop -- but excluding it from both meant a PR that edited only the baseline never started the workflow, so the hand-raised-baseline compare never ran and the required gate never reported. The coverage guard now reads the project count from env and rejects a non-numeric one before comparing. There is no `set -e` in that step, and `[ x -ne "" ]` exits 2, which an `if` reads as false: the guard meant to refuse a partial build would have fallen through and rewritten the baseline from it. Reading through env also drops the last inline expansion in a run: block that zizmor flagged. Same missing `set -e` made the commit unchecked: a failed `git commit` leaves `git push` with nothing to send, and it exits 0 saying "Everything up-to-date", so the run reported a lowered baseline it never wrote. --- .github/scripts/warning_baseline.py | 2 +- .github/warning-baseline.txt | 2 +- .github/workflows/linux-simulator.yml | 28 +++++++++++++++++++++------ 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/.github/scripts/warning_baseline.py b/.github/scripts/warning_baseline.py index 2f4bcd58a..6dede35ae 100644 --- a/.github/scripts/warning_baseline.py +++ b/.github/scripts/warning_baseline.py @@ -57,7 +57,7 @@ BASELINE_HEADER = """\ # Tolerated compiler warnings in the gcc simulator build -- see -# Utilities/Scripts/warning-baseline/warning_baseline.py. +# .github/scripts/warning_baseline.py. # # Generated. Do not hand-edit to make a build pass: the number is the count of # warning sites of that flag in that file, and raising one is how a warning gets diff --git a/.github/warning-baseline.txt b/.github/warning-baseline.txt index 4cc3f3f08..4b1975f9c 100644 --- a/.github/warning-baseline.txt +++ b/.github/warning-baseline.txt @@ -1,5 +1,5 @@ # Tolerated compiler warnings in the gcc simulator build -- see -# Utilities/Scripts/warning-baseline/warning_baseline.py. +# .github/scripts/warning_baseline.py. # # Generated. Do not hand-edit to make a build pass: the number is the count of # warning sites of that flag in that file, and raising one is how a warning gets diff --git a/.github/workflows/linux-simulator.yml b/.github/workflows/linux-simulator.yml index 428fa394d..101dff1b9 100644 --- a/.github/workflows/linux-simulator.yml +++ b/.github/workflows/linux-simulator.yml @@ -17,6 +17,7 @@ on: - 'Examples/Apps/**' - 'Docs/Tutorials/**' - '.github/scripts/warning_baseline.py' + - '.github/warning-baseline.txt' - '.github/workflows/linux-simulator.yml' push: branches: [ main, develop ] @@ -29,8 +30,10 @@ on: - '.github/workflows/linux-simulator.yml' workflow_dispatch: -# Deliberately NOT in the paths filters above: .github/warning-baseline.txt. The -# ratchet job commits it, and a self-trigger would loop. +# Deliberately NOT in the push paths filter above: .github/warning-baseline.txt. The +# ratchet job commits it to main, and a self-trigger would loop. It IS in the +# pull_request filter: a PR that edits only the baseline is exactly the PR that most +# needs the gate to run, and the ratchet never pushes on that trigger. permissions: contents: read @@ -338,6 +341,7 @@ jobs: - name: Lower the baseline if the warnings went away env: GH_TOKEN: ${{ github.token }} + EXPECTED: ${{ needs.discover.outputs.count }} run: | set -uo pipefail shopt -s nullglob @@ -345,9 +349,17 @@ jobs: # An empty or partial set would look exactly like "the warnings were fixed" # and wipe the baseline. Refuse rather than guess. - expected='${{ needs.discover.outputs.count }}' - if [ ${#files[@]} -ne "$expected" ]; then - echo "::error title=Incomplete warning coverage::Got ${#files[@]} count file(s) for ${expected} project(s); refusing to rewrite the baseline from a partial build." + # + # The count is checked for digits before it is compared: `[` on a + # non-numeric operand exits 2, which reads as "false" to the `if` below and + # would wave the partial set through into the baseline. + case "${EXPECTED:-}" in + ''|*[!0-9]*) + echo "::error title=Unusable project count::discover reported '${EXPECTED:-}' as the project count; refusing to rewrite the baseline." + exit 1 ;; + esac + if [ "${#files[@]}" -ne "$EXPECTED" ]; then + echo "::error title=Incomplete warning coverage::Got ${#files[@]} count file(s) for ${EXPECTED} project(s); refusing to rewrite the baseline from a partial build." exit 1 fi @@ -376,8 +388,12 @@ jobs: git add .github/warning-baseline.txt # [skip ci]: apps-ci, host-tests and tutorials-ci trigger on any push to # main, and this commit is one generated line. + # Checked: a failed commit leaves the push below with nothing to send, and + # `git push` reports "Everything up-to-date" and exits 0, so the run would + # claim it lowered a baseline it never wrote. git commit -m "chore(ci): ratchet the simulator warning baseline down [skip ci]" \ - -m "Generated by .github/workflows/linux-simulator.yml from ${GITHUB_SHA}." + -m "Generated by .github/workflows/linux-simulator.yml from ${GITHUB_SHA}." \ + || { echo "::error title=Commit failed::update rewrote the baseline but git commit failed."; exit 1; } if git push origin main; then echo "Baseline lowered." From afa5fd29506d228b2e6a21e78e7622aea7f845fb Mon Sep 17 00:00:00 2001 From: Toby Murray Date: Sat, 8 Aug 2026 21:41:18 -0400 Subject: [PATCH 3/7] fix(ci): close the ways the warning ratchet could go up, or stop counting The gate could be satisfied without the warnings being checked, and the half that lowers the baseline could not run at all. Seven fixes, in rough order of how likely each was to bite. The gate never ran on changes to the gate. `.github/scripts/warning_baseline.py` and `.github/warning-baseline.txt` trigger the workflow but were absent from discover's build-all list, so a PR touching only them selected no project -- which skipped the build, the comparison, and the script's own selftest. A skipped job satisfies a required check, so the one change that can disable the ratchet outright was the one change it never inspected. Both are now in the build-all list, and the comment claiming otherwise is corrected. `bash -e` is GitHub's default shell and `set -uo pipefail` does not clear it, so the careful `rc=$?` handling in the gate and ratchet steps was dead code. `update` exits 2 for "baseline unchanged" -- the usual outcome of a merge -- and errexit turned that into a red ratchet job on almost every push to main. Both steps now `set +e` explicitly, which also restores the gate's error annotation. A push rejected by branch protection was reported as a lost race, retried three times and swallowed as a warning with exit 0. Making `warnings` required makes it apply to the bot's own push, so this was going to be the permanent state: the baseline could go up by review and never come back down, silently and green. Rejections are now classified, and protection failures are fatal. Nothing asserted the flags were still on the command line -- and nothing could, since una/Makefile compiles with a leading `@` and the flags never reach the log. Dropping -Wcast-qual from one app read as a fix to `check` and as a reason to delete those keys to `update`, after which restoring the flag failed the gate on every PR: the ratchet blocked its own repair. `flags` reads WARN/CXXWARN out of all fifteen Makefiles and fails on a missing warning, a new -Wno-, or a bare -w. Retiring a warning now means editing REQUIRED_WARNINGS, in one visible place. `update` had no floor. An empty observation from all fifteen legs -- a stripped flag, a parser that stopped matching -- passed every guard and wiped the baseline for good. Drops past half the total now need --allow-collapse. The gate passed on partial coverage: it only failed when *every* leg produced nothing, so one flaky apt-get was enough to leave a project unchecked and green. It now requires a count file per selected project, and fails if any build leg did not succeed -- removing -Wno-error, for instance, turns every warning into an `error:` line the parser cannot see, which used to read as zero warnings on a broken build. A dead discover job skipped the gate entirely; it is now asserted in a step, and a discover that finds no projects fails rather than vouching for an empty build. Two smaller ones: GNU make's own `Makefile:224: warning:` diagnostics have gcc's exact shape and were counted as code warnings, and the failure output named the file and flag but not the line, so reacting to it started with downloading an artifact. `extract --details` now carries line, column and message through to the failure text. Selftests grow 30 -> 47, including a check that reads the real Makefiles. --- .github/scripts/warning_baseline.py | 416 +++++++++++++++++++++++++- .github/workflows/linux-simulator.yml | 151 ++++++++-- 2 files changed, 532 insertions(+), 35 deletions(-) diff --git a/.github/scripts/warning_baseline.py b/.github/scripts/warning_baseline.py index 6dede35ae..bfb7d47f7 100644 --- a/.github/scripts/warning_baseline.py +++ b/.github/scripts/warning_baseline.py @@ -28,15 +28,25 @@ build. max() makes the baseline a property of the code, which in turn lets a partial build (a PR touching one app) be checked against a baseline produced by a full one. +A count is only meaningful if the flags that produce it are still on the command +line, and nothing in a build log proves that -- una/Makefile compiles with a leading +`@`, so the flags never reach the log at all. Dropping -Wcast-qual from one app makes +its warnings vanish, which reads to `check` as a fix and to `update` as a reason to +delete those keys for good. `flags` closes that: it reads the WARN/CXXWARN lists out +of every una/Makefile and fails if a required warning is gone, a new suppression +appeared, or -w turned up in user_cflags. + Subcommands: extract one build log -> normalized counts (TSV on stdout) check counts vs baseline -> exit 1 if anything is new or higher - update counts -> baseline -> rewrite, exit 0 if it changed, 2 if unchanged + update counts -> baseline -> rewrite; 0 changed, 2 unchanged, 1 increase, 3 collapse compare two baselines -> exit 1 if the second tolerates more than the first + flags every una/Makefile -> exit 1 if the warning flags were weakened --selftest run the tests baked into this file, no checkout needed """ import argparse +import io import os import posixpath import re @@ -53,8 +63,21 @@ # Not our code; upstream's warnings are not ours to ratchet down. EXCLUDED_PREFIXES = ("ThirdParty/",) +# GNU make reports its own diagnostics as "Makefile:224: warning: overriding recipe", +# which is indistinguishable from gcc's shape. Counting those would let a Makefile +# edit fail the gate, and would bake a non-compiler warning into the baseline. +IGNORED_BASENAME_RE = re.compile(r"^([Mm]akefile(\..*)?|.*\.mk)$") + NO_FLAG = "(unflagged)" +# A full build that suddenly reports almost nothing is far more likely to be a broken +# parser or a stripped flag than 40 hand-fixed warnings, and `update` is the one place +# that can make the mistake permanent. Anything below this fraction of the previous +# total needs --allow-collapse. Below COLLAPSE_MIN_TOTAL there is nothing left worth +# guarding, and every honest fix would trip it. +COLLAPSE_FLOOR = 0.5 +COLLAPSE_MIN_TOTAL = 10 + BASELINE_HEADER = """\ # Tolerated compiler warnings in the gcc simulator build -- see # .github/scripts/warning_baseline.py. @@ -88,8 +111,8 @@ def normalize_path(raw, base): return resolved -def extract(log_path, app_dir, workspace): - """Parse one build log into {(path, flag): count} of distinct warning sites.""" +def extract_sites(log_path, app_dir, workspace): + """Parse one build log into a set of distinct (path, flag, line, col, msg) sites.""" workspace = workspace.replace("\\", "/").rstrip("/") sites = set() @@ -111,19 +134,51 @@ def extract(log_path, app_dir, workspace): path = normalize_path(raw_path, base) if path is None: continue + if IGNORED_BASENAME_RE.match(posixpath.basename(path)): + continue # A warning in a header is re-emitted once per translation unit that # includes it, so dedupe on the site before counting. - sites.add((path, match.group("line"), match.group("col"), match.group("msg"))) + msg = match.group("msg") + flag_match = FLAG_RE.search(msg) + flag = flag_match.group(1) if flag_match else NO_FLAG + sites.add((path, flag, match.group("line"), match.group("col") or "", msg)) + return sites + + +def counts_from_sites(sites): counts = {} - for path, _line, _col, msg in sites: - flag_match = FLAG_RE.search(msg) - flag = flag_match.group(1) if flag_match else NO_FLAG + for path, flag, _line, _col, _msg in sites: counts[(path, flag)] = counts.get((path, flag), 0) + 1 return counts +def extract(log_path, app_dir, workspace): + """Parse one build log into {(path, flag): count} of distinct warning sites.""" + return counts_from_sites(extract_sites(log_path, app_dir, workspace)) + + +def write_sites(fh, sites): + for path, flag, line, col, msg in sorted(sites): + fh.write(f"{path}\t{flag}\t{line}\t{col}\t{msg}\n") + + +def read_sites(paths): + """Merge extract --details sidecars into {(path, flag): [(line, col, msg), ...]}.""" + merged = {} + for path in paths: + if not os.path.exists(path): + continue + with open(path, "r", encoding="utf-8") as fh: + for line in fh: + fields = line.rstrip("\n").split("\t", 4) + if len(fields) != 5: + continue + merged.setdefault((fields[0], fields[1]), set()).add(tuple(fields[2:])) + return merged + + def read_counts(path): """Read a TSV of pathflagcount, ignoring comments and blanks.""" counts = {} @@ -162,15 +217,35 @@ def merge(files): return merged -def format_regressions(regressions): +MAX_SITES_SHOWN = 6 + + +def format_regressions(regressions, details=None): + """One line per regressed key, plus the concrete sites when a sidecar has them. + + Without the sites a contributor knows the file and the flag but has to download + the build.log artifact to find out which line -- which is most of the cost of + reacting to this gate at all. + """ lines = [] width = max(len(f"{p} [{f}]") for p, f in regressions) if regressions else 0 - for (path, flag), (observed, allowed) in sorted(regressions.items()): + for key, (observed, allowed) in sorted(regressions.items()): + path, flag = key label = f"{path} [{flag}]" lines.append(f" {label:<{width}} {allowed} allowed -> {observed} found") + for line, col, msg in sorted((details or {}).get(key, []), key=_site_order)[ + :MAX_SITES_SHOWN + ]: + where = f"{path}:{line}:{col}" if col else f"{path}:{line}" + lines.append(f" {where}: {msg}") return lines +def _site_order(site): + line, col, _msg = site + return (int(line) if line.isdigit() else 0, int(col) if col.isdigit() else 0) + + def diff_counts(observed, allowed): """Keys where observed exceeds allowed, as {key: (observed, allowed)}.""" return { @@ -180,14 +255,155 @@ def diff_counts(observed, allowed): } +# ------------------------------------------------------------------- flag policy + +# Every una/Makefile must still ASK for these. Deleting one from the list below is +# how you legitimately retire a warning -- and it is a diff a reviewer can see, which +# editing a Makefile in one app out of fifteen is not. +REQUIRED_WARNINGS = frozenset( + { + "all", + "extra", + "format=2", + "cast-qual", + "write-strings", + "init-self", + "pointer-arith", + "strict-aliasing", + "uninitialized", + "missing-declarations", + } +) +REQUIRED_CXX_WARNINGS = frozenset({"non-virtual-dtor", "ctor-dtor-privacy"}) + +# The suppressions the tree already carries. A new -Wno-* silences warnings just as +# effectively as deleting the flag that finds them, so the set is closed. +ALLOWED_SUPPRESSIONS = frozenset( + { + "no-long-long", + "no-unused-parameter", + "no-variadic-macros", + "no-format-extra-args", + "no-conversion", + "no-overloaded-virtual", + } +) + +# -Wno-error is the documented status quo: the Makefile asks for -Werror and then +# takes it back, which is why these are warnings and not build failures. +ALLOWED_CFLAG_SUPPRESSIONS = frozenset({"-Wno-error"}) + +MAKEFILE_SEARCH_ROOTS = ("Examples/Apps", "Docs/Tutorials") + +ASSIGN_RE = re.compile(r"^(?PWARN|CXXWARN)\s*[:+?]?=\s*(?P.*)$") + + +def _logical_lines(text): + """Makefile lines with backslash continuations joined.""" + return re.sub(r"\\\n\s*", " ", text).splitlines() + + +def check_makefile_flags(text): + """Return a list of human-readable problems with one una/Makefile's warning flags.""" + problems = [] + lists = {} + cflag_tokens = [] + uses_warn = set() + + for line in _logical_lines(text): + stripped = line.strip() + assign = ASSIGN_RE.match(stripped) + if assign: + lists[assign.group("name")] = assign.group("value").split() + continue + if "user_cflags" in stripped: + cflag_tokens += stripped.split() + for var in ("c_compiler_options_local", "cpp_compiler_options_local"): + if stripped.startswith(var) and "$(WARN)" in stripped: + uses_warn.add(var) + + for name, required in (("WARN", REQUIRED_WARNINGS), ("CXXWARN", REQUIRED_CXX_WARNINGS)): + if name not in lists: + problems.append(f"{name} is not defined") + continue + tokens = set(lists[name]) + for missing in sorted(required - tokens): + problems.append(f"{name} no longer asks for -W{missing}") + for token in sorted(tokens): + if token.startswith("no-") and token not in ALLOWED_SUPPRESSIONS: + problems.append(f"{name} adds the suppression -W{token}") + + for var in ("c_compiler_options_local", "cpp_compiler_options_local"): + if var not in uses_warn: + problems.append(f"{var} no longer expands $(WARN)") + if "-pedantic" not in text: + problems.append("-pedantic is gone") + + for token in cflag_tokens: + if token == "-w": + problems.append("user_cflags disables all warnings with -w") + elif token.startswith("-Wno-") and token not in ALLOWED_CFLAG_SUPPRESSIONS: + problems.append(f"user_cflags adds the suppression {token}") + + return problems + + +def find_una_makefiles(root): + """Every /una/Makefile under the app and tutorial trees, repo-relative.""" + found = [] + for base in MAKEFILE_SEARCH_ROOTS: + for dirpath, _dirnames, filenames in os.walk(os.path.join(root, base)): + if os.path.basename(dirpath) == "una" and "Makefile" in filenames: + full = os.path.join(dirpath, "Makefile") + found.append(os.path.relpath(full, root).replace(os.sep, "/")) + return sorted(found) + + +def cmd_flags(args): + root = args.root or REPO_ROOT + makefiles = find_una_makefiles(root) + + # Finding none would otherwise pass silently and vouch for nothing. + if not makefiles: + print(f"FAIL: no una/Makefile found under {'/, '.join(MAKEFILE_SEARCH_ROOTS)}/ in {root}") + return 1 + + failures = 0 + for rel in makefiles: + with open(os.path.join(root, rel), "r", encoding="utf-8", errors="replace") as fh: + problems = check_makefile_flags(fh.read()) + if problems: + failures += 1 + print(f"FAIL {rel}") + for problem in problems: + print(f" - {problem}") + + if failures: + print( + f"\n{failures} of {len(makefiles)} simulator project(s) weakened their warning " + "flags.\nThe baseline counts warnings the build reports; a flag that is no longer\n" + "requested reports nothing, which reads as a fix and is then ratcheted away for\n" + "good. Retire a warning by removing it from REQUIRED_WARNINGS in this script --\n" + "in one reviewable place -- not from one app's Makefile." + ) + return 1 + + print(f"OK: {len(makefiles)} simulator project(s) still request the required warning flags.") + return 0 + + def cmd_extract(args): - counts = extract(args.log, args.app_dir.strip("/"), args.workspace) + sites = extract_sites(args.log, args.app_dir.strip("/"), args.workspace) + counts = counts_from_sites(sites) out = open(args.out, "w", encoding="utf-8") if args.out else sys.stdout try: write_counts(out, counts) finally: if args.out: out.close() + if args.details: + with open(args.details, "w", encoding="utf-8") as fh: + write_sites(fh, sites) total = sum(counts.values()) print(f"{args.log}: {total} warning site(s) in {len(counts)} file/flag pair(s)", file=sys.stderr) return 0 @@ -207,7 +423,7 @@ def cmd_check(args): new_sites = sum(o - a for o, a in regressions.values()) print(f"FAIL: {new_sites} new warning site(s) in {len(regressions)} file/flag pair(s):\n") - print("\n".join(format_regressions(regressions))) + print("\n".join(format_regressions(regressions, read_sites(args.details)))) print( "\nFix the warnings. If a warning is genuinely acceptable, raising its count in\n" f"{args.baseline} is a reviewable change -- not a silent one." @@ -228,6 +444,24 @@ def cmd_update(args): print("\n".join(format_regressions(regressions))) return 1 + # A collapse is the one direction the ratchet cannot walk back: once the keys are + # gone, restoring the flags that found them fails `check` on every PR, so the gate + # ends up blocking its own repair. Make a big drop an explicit decision. + old_total, new_total = sum(old.values()), sum(new.values()) + if ( + old_total >= COLLAPSE_MIN_TOTAL + and new_total < old_total * COLLAPSE_FLOOR + and not args.allow_collapse + ): + print( + f"REFUSING to update: {old_total} -> {new_total} warning site(s) is a " + f"{100 * (old_total - new_total) // old_total}% drop.\n\n" + "A drop that size is more often a build that stopped asking for the flags, or\n" + "a parser that stopped recognising the output, than a real fix. Confirm the\n" + "warnings are genuinely gone, then re-run with --allow-collapse." + ) + return 3 + if new == old: print(f"Baseline unchanged ({sum(old.values())} warning site(s)).") return 2 @@ -361,6 +595,22 @@ def test_line_numbers_are_not_part_of_the_key(self): moved = self.write("moved.log", SELFTEST_LOG.replace(":30:28:", ":130:28:")) self.assertEqual(extract(moved, SELFTEST_APP_DIR, SELFTEST_WORKSPACE), self.counts) + def test_make_diagnostics_are_not_counted_as_code_warnings(self): + """`Makefile:224: warning: overriding recipe` has gcc's exact shape.""" + log = self.write( + "make.log", + "Makefile:224: warning: overriding recipe for target 'x'\n" + "una/Makefile:12: warning: ignoring old recipe\n" + "config/gcc/app.mk:3:1: warning: something [-Wpedantic]\n", + ) + self.assertEqual(extract(log, SELFTEST_APP_DIR, SELFTEST_WORKSPACE), {}) + + def test_details_carry_the_line_and_message(self): + sites = extract_sites(self.log, SELFTEST_APP_DIR, SELFTEST_WORKSPACE) + key = ("Libs/Header/SDK/Simulator/Kernel/Mock/AppMemory.hpp", "-Wformat=") + lines = {line for path, flag, line, _col, _msg in sites if (path, flag) == key} + self.assertEqual(lines, {"30", "42"}) + class TestBaselineIO(SelftestBase): def test_round_trip_through_the_file_format(self): @@ -449,6 +699,44 @@ def test_update_allows_an_increase_only_when_asked(self): obs = self.write("o.tsv", "Libs/a.cpp\t-Wformat=\t9\n") self.assertEqual(main(["update", "--baseline", self.baseline, "--allow-increase", obs]), 0) + def test_update_refuses_a_collapse(self): + """A near-empty observation is what a stripped flag or a broken parser looks + like, and deleting the keys is the one direction the ratchet cannot walk + back: restoring the flags afterwards fails `check` on every PR.""" + big = self.write("big.txt", "Libs/a.cpp\t-Wformat=\t20\n") + obs = self.write("o.tsv", "") + self.assertEqual(main(["update", "--baseline", big, obs]), 3) + self.assertEqual(sum(read_counts(big).values()), 20) + + def test_update_allows_a_collapse_only_when_asked(self): + big = self.write("big.txt", "Libs/a.cpp\t-Wformat=\t20\n") + obs = self.write("o.tsv", "") + self.assertEqual(main(["update", "--baseline", big, "--allow-collapse", obs]), 0) + self.assertEqual(read_counts(big), {}) + + def test_update_allows_a_drop_that_stays_above_the_floor(self): + big = self.write("big.txt", "Libs/a.cpp\t-Wformat=\t20\n") + obs = self.write("o.tsv", "Libs/a.cpp\t-Wformat=\t15\n") + self.assertEqual(main(["update", "--baseline", big, obs]), 0) + + def test_a_tiny_baseline_is_not_subject_to_the_collapse_floor(self): + """Fixing the last two warnings must not need a flag.""" + obs = self.write("o.tsv", "") + self.assertEqual(main(["update", "--baseline", self.baseline, obs]), 0) + + def test_check_names_the_offending_lines_when_details_are_supplied(self): + obs = self.write("o.tsv", "Libs/c.cpp\t-Wformat=\t1\n") + det = self.write("o.details.tsv", "Libs/c.cpp\t-Wformat=\t7\t3\tformat '%d' expects int\n") + out = io.StringIO() + real, sys.stdout = sys.stdout, out + try: + self.assertEqual( + main(["check", "--baseline", self.baseline, "--details", det, obs]), 1 + ) + finally: + sys.stdout = real + self.assertIn("Libs/c.cpp:7:3: format '%d' expects int", out.getvalue()) + def test_compare_flags_a_hand_raised_baseline(self): after = self.write("after.txt", "Libs/a.cpp\t-Wformat=\t5\n") self.assertEqual(main(["compare", "--before", self.baseline, "--after", after]), 1) @@ -468,6 +756,85 @@ def test_extract_writes_the_requested_file(self): ) +GOOD_MAKEFILE = textwrap.dedent( + """\ + WARN = error all extra write-strings init-self cast-qual \\ + pointer-arith strict-aliasing format=2 uninitialized \\ + missing-declarations no-long-long no-unused-parameter \\ + no-variadic-macros no-format-extra-args \\ + no-conversion no-overloaded-virtual + CXXWARN = non-virtual-dtor ctor-dtor-privacy + + c_compiler_options_local += -pedantic $(addprefix -W,$(WARN)) + cpp_compiler_options_local += -pedantic $(addprefix -W,$(WARN) $(CXXWARN)) + override user_cflags += -Wno-error + override user_cflags += -DBUILD_VERSION=\\"1.2.3\\" + """ +) + + +class TestFlagPolicy(unittest.TestCase): + """A count only means something while the flags that produce it are requested.""" + + def test_the_tree_as_it_stands_is_accepted(self): + self.assertEqual(check_makefile_flags(GOOD_MAKEFILE), []) + + def test_a_dropped_warning_is_caught(self): + weakened = GOOD_MAKEFILE.replace(" cast-qual", "") + self.assertIn("WARN no longer asks for -Wcast-qual", check_makefile_flags(weakened)) + + def test_a_new_suppression_is_caught(self): + weakened = GOOD_MAKEFILE.replace("no-long-long", "no-long-long no-cast-qual") + self.assertIn("WARN adds the suppression -Wno-cast-qual", check_makefile_flags(weakened)) + + def test_a_dropped_cxx_warning_is_caught(self): + weakened = GOOD_MAKEFILE.replace("non-virtual-dtor ", "") + self.assertIn("CXXWARN no longer asks for -Wnon-virtual-dtor", check_makefile_flags(weakened)) + + def test_blanket_w_in_user_cflags_is_caught(self): + weakened = GOOD_MAKEFILE.replace("-Wno-error", "-Wno-error -w") + self.assertIn("user_cflags disables all warnings with -w", check_makefile_flags(weakened)) + + def test_a_suppression_smuggled_into_user_cflags_is_caught(self): + weakened = GOOD_MAKEFILE.replace("-Wno-error", "-Wno-error -Wno-reorder") + self.assertIn( + "user_cflags adds the suppression -Wno-reorder", check_makefile_flags(weakened) + ) + + def test_detaching_the_list_from_the_command_line_is_caught(self): + """Leaving WARN defined but unused would satisfy a naive grep.""" + weakened = GOOD_MAKEFILE.replace("$(addprefix -W,$(WARN))", "") + self.assertIn( + "c_compiler_options_local no longer expands $(WARN)", check_makefile_flags(weakened) + ) + + def test_dropping_pedantic_is_caught(self): + self.assertIn("-pedantic is gone", check_makefile_flags(GOOD_MAKEFILE.replace("-pedantic", ""))) + + +class TestCommittedMakefiles(unittest.TestCase): + """Reads the real tree. Weakening the gate now means editing this file, in a diff + that says so, rather than one app's Makefile out of fifteen.""" + + def setUp(self): + self.makefiles = find_una_makefiles(REPO_ROOT) + if not self.makefiles: + self.skipTest("no una/Makefile in this checkout") + + def test_the_headline_flags_are_required_by_name(self): + for flag in ("all", "extra", "format=2", "cast-qual"): + self.assertIn(flag, REQUIRED_WARNINGS, f"-W{flag} was dropped from the policy") + + def test_every_project_still_requests_them(self): + offenders = {} + for rel in self.makefiles: + with open(os.path.join(REPO_ROOT, rel), "r", encoding="utf-8") as fh: + problems = check_makefile_flags(fh.read()) + if problems: + offenders[rel] = problems + self.assertEqual(offenders, {}) + + class TestCommittedBaseline(unittest.TestCase): """Catches a baseline that has drifted out of sync with the tree. Skipped when run outside a checkout.""" @@ -504,7 +871,14 @@ def run_selftest(): loader = unittest.TestLoader() suite = unittest.TestSuite( loader.loadTestsFromTestCase(case) - for case in (TestExtract, TestBaselineIO, TestCommands, TestCommittedBaseline) + for case in ( + TestExtract, + TestBaselineIO, + TestCommands, + TestFlagPolicy, + TestCommittedMakefiles, + TestCommittedBaseline, + ) ) # The command tests print their own diagnostics; keep that off the test report. with open(os.devnull, "w", encoding="utf-8") as devnull: @@ -536,10 +910,19 @@ def main(argv=None): ) p.add_argument("--workspace", default="", help="absolute repo root to strip from paths") p.add_argument("--out", help="write here instead of stdout") + p.add_argument("--details", help="also write path/flag/line/col/message sites here") p.set_defaults(func=cmd_extract) p = sub.add_parser("check", help="fail if counts exceed the baseline") p.add_argument("--baseline", required=True) + # Repeatable rather than nargs="*", which would swallow the positional counts. + p.add_argument( + "--details", + action="append", + default=[], + metavar="FILE", + help="an extract --details sidecar, to name the offending lines in the failure", + ) p.add_argument("counts", nargs="+", help="one or more extract outputs") p.set_defaults(func=cmd_check) @@ -550,6 +933,11 @@ def main(argv=None): action="store_true", help="permit a higher baseline (for the initial seeding only)", ) + p.add_argument( + "--allow-collapse", + action="store_true", + help=f"permit a drop below {int(COLLAPSE_FLOOR * 100)}%% of the current total", + ) p.add_argument("counts", nargs="+", help="one or more extract outputs") p.set_defaults(func=cmd_update) @@ -558,6 +946,10 @@ def main(argv=None): p.add_argument("--after", required=True) p.set_defaults(func=cmd_compare) + p = sub.add_parser("flags", help="fail if any una/Makefile weakened its warning flags") + p.add_argument("--root", help="repo root to scan (default: this script's checkout)") + p.set_defaults(func=cmd_flags) + args = parser.parse_args(argv) return args.func(args) diff --git a/.github/workflows/linux-simulator.yml b/.github/workflows/linux-simulator.yml index 101dff1b9..42d380d6f 100644 --- a/.github/workflows/linux-simulator.yml +++ b/.github/workflows/linux-simulator.yml @@ -8,6 +8,12 @@ name: Linux Simulator # NOTE: the baseline commit this workflow pushes to main carries [skip ci], because # apps-ci, host-tests and tutorials-ci all trigger on an unfiltered push to main and # would otherwise run a full cycle for a one-line generated file. +# +# SETUP: making `Warning ratchet` a required check also makes it apply to direct +# pushes, including the ratchet job's own. github-actions[bot] therefore needs a +# bypass for main in the repository ruleset; without one the baseline can go up by +# review but never come back down. The job now fails loudly on that rejection rather +# than mistaking it for a lost push race. on: pull_request: @@ -34,6 +40,13 @@ on: # ratchet job commits it to main, and a self-trigger would loop. It IS in the # pull_request filter: a PR that edits only the baseline is exactly the PR that most # needs the gate to run, and the ratchet never pushes on that trigger. +# +# Triggering is only half of that, though. Landing in the filter gets the workflow +# started; it takes a matching entry in the build-all list in `discover` to get any +# project selected, and with nothing selected the gate has nothing to check and +# reports success. So the gate's own two files are listed in BOTH places, and the +# `warnings` job below runs unconditionally rather than being skipped along with the +# build -- a skipped job satisfies a required check just as a passing one does. permissions: contents: read @@ -64,13 +77,20 @@ jobs: run: | set -uo pipefail - # A project is any dir containing simulator/gcc/Makefile. + # A project is any dir containing simulator/gcc/Makefile. `|| true` because + # pipefail is on and find exits non-zero for an absent search root -- which + # would kill this step, and a dead discover used to leave the gate skipped + # (i.e. green). It is caught by the TOTAL check below instead. find_projects() { - find Examples/Apps Docs/Tutorials -path '*/simulator/gcc/Makefile' 2>/dev/null \ - | sed 's#/simulator/gcc/Makefile##' | sort + { find Examples/Apps Docs/Tutorials -path '*/simulator/gcc/Makefile' 2>/dev/null \ + || true; } | sed 's#/simulator/gcc/Makefile##' | sort } TOTAL="$(find_projects | wc -l | tr -d ' ')" echo "Discovered $TOTAL simulator project(s)." + if [ "$TOTAL" -eq 0 ]; then + echo "::error title=No simulator projects::Found no */simulator/gcc/Makefile. Refusing to report a green gate over an empty build." + exit 1 + fi # push / dispatch build everything; a PR builds only the projects it # touches, or everything when it touches shared inputs. @@ -93,7 +113,11 @@ jobs: # "nothing to build" -- fail safe to a full build rather than skip. echo "::warning::Could not determine changed files for base ${BASE_SHA}; building all projects." BUILD_ALL=true - elif printf '%s\n' "$CHANGED" | grep -qE '^(Libs/|ThirdParty/|\.github/workflows/linux-simulator\.yml)'; then + # Shared inputs, plus the gate's own machinery. A PR that edits only the + # script or only the baseline used to select nothing, which skipped the + # build, the gate AND the script's selftest -- so the one change that can + # disable the ratchet outright was the one change it never inspected. + elif printf '%s\n' "$CHANGED" | grep -qE '^(Libs/|ThirdParty/|\.github/(workflows/linux-simulator\.yml|scripts/warning_baseline\.py|warning-baseline\.txt))'; then BUILD_ALL=true fi fi @@ -211,14 +235,19 @@ jobs: --log build.log \ --app-dir "$APP_DIR" \ --workspace "$GITHUB_WORKSPACE" \ - --out "warnings-${{ matrix.project.slug }}.tsv" + --out "warnings-${{ matrix.project.slug }}.tsv" \ + --details "warnings-${{ matrix.project.slug }}.details" + # The counts drive the gate; the details exist so its failure can name the line + # and the message instead of sending the contributor to download build.log. - name: Upload warning counts if: always() uses: actions/upload-artifact@v7 with: name: warning-counts-${{ matrix.project.slug }} - path: warnings-${{ matrix.project.slug }}.tsv + path: | + warnings-${{ matrix.project.slug }}.tsv + warnings-${{ matrix.project.slug }}.details if-no-files-found: ignore retention-days: 14 @@ -253,13 +282,21 @@ jobs: # pull request from a fork. warnings: needs: [ discover, build ] - # always(), so a failed build leg still has its warnings checked -- but only once - # discover actually produced a selection. - if: always() && needs.discover.result == 'success' && needs.discover.outputs.projects != '[]' + # Bare always(): this job must reach a conclusion on every run of the workflow. + # Branch protection treats a skipped required check as a satisfied one, so every + # `if:` that could skip this job is a way to merge without being checked -- which + # is why the discover and build results are asserted in steps below instead. + if: always() name: Warning ratchet runs-on: ubuntu-24.04 timeout-minutes: 5 steps: + - name: Require a usable project selection + if: needs.discover.result != 'success' + run: | + echo "::error title=Selection failed::discover finished '${{ needs.discover.result }}', so the ratchet cannot know which projects should have been built." + exit 1 + - name: Checkout uses: actions/checkout@v7 with: @@ -268,7 +305,16 @@ jobs: - name: Selftest the ratchet itself run: python3 .github/scripts/warning_baseline.py --selftest + # The counts are only evidence while the compiler is still asked for the + # warnings. una/Makefile compiles with a leading `@`, so the flags never appear + # in build.log and no amount of parsing can confirm them -- read them from the + # Makefiles instead. Deleting -Wcast-qual from one app would otherwise pass the + # gate as a fix and be ratcheted away on merge. + - name: Verify the warning flags are still requested + run: python3 .github/scripts/warning_baseline.py flags + - name: Download warning counts + if: needs.discover.outputs.projects != '[]' uses: actions/download-artifact@v7 with: pattern: warning-counts-* @@ -277,21 +323,38 @@ jobs: - name: Compare against the baseline id: gate + if: needs.discover.outputs.projects != '[]' + env: + EXPECTED: ${{ needs.discover.outputs.count }} run: | + set +e # see below; the gate's exit status is explicit set -uo pipefail shopt -s nullglob files=(counts/*.tsv) + details=() + for d in counts/*.details; do details+=(--details "$d"); done echo "legs=${#files[@]}" >> "$GITHUB_OUTPUT" - if [ ${#files[@]} -eq 0 ]; then - echo "::error title=No warning counts::Every build leg failed before producing a log; the ratchet has nothing to check." + + # A missing leg is a project whose warnings nobody looked at. That used to + # pass as long as one leg survived, so an unrelated apt-get flake was enough + # to wave a new warning through. + case "${EXPECTED:-}" in + ''|*[!0-9]*) + echo "::error title=Unusable project count::discover reported '${EXPECTED:-}' as the project count." + exit 1 ;; + esac + if [ "${#files[@]}" -ne "$EXPECTED" ]; then + echo "::error title=Incomplete warning coverage::Got ${#files[@]} count file(s) for ${EXPECTED} selected project(s); the missing ones were never checked. Re-run the failed leg." exit 1 fi - echo "Checking ${#files[@]} of ${{ needs.discover.outputs.count }} selected project(s)." - # No `set -e` here, so the status has to be captured and re-raised at the - # end; without the explicit `exit $rc` a failing gate would exit 0 on the - # trailing echo. + echo "Checking ${#files[@]} of ${EXPECTED} selected project(s)." + + # GitHub runs `run:` under `bash -e`, which a `set -uo pipefail` line does + # not undo: without the `set +e` above, a failing check would abort the step + # right here and the annotation and output below would never be emitted. python3 .github/scripts/warning_baseline.py check \ - --baseline .github/warning-baseline.txt "${files[@]}" > /tmp/gate.out 2>&1 + --baseline .github/warning-baseline.txt \ + "${details[@]}" "${files[@]}" > /tmp/gate.out 2>&1 rc=$? cat /tmp/gate.out if [ $rc -ne 0 ]; then @@ -304,13 +367,29 @@ jobs: run: | ok='${{ steps.gate.outcome }}' { - if [ "$ok" = success ]; then echo "### ✅ Warning ratchet"; else echo "### ❌ Warning ratchet"; fi + if [ "$ok" = failure ]; then echo "### ❌ Warning ratchet"; else echo "### ✅ Warning ratchet"; fi echo "" - echo '```'; cat /tmp/gate.out 2>/dev/null || echo "(no output)"; echo '```' + if [ "$ok" = skipped ]; then + echo "No simulator project was selected, so there were no warnings to check." + echo "The flag policy and the ratchet's own selftest still ran." + else + echo '```'; cat /tmp/gate.out 2>/dev/null || echo "(no output)"; echo '```' + fi echo "" echo "Coverage: ${{ steps.gate.outputs.legs || 0 }} of ${{ needs.discover.outputs.count }} selected project(s), out of ${{ needs.discover.outputs.total }} in the repo." } >> "$GITHUB_STEP_SUMMARY" + # Last, so the warning comparison above is always reported first: a build that + # died has its warnings checked (that is the point of always()), but it must not + # leave a green required check behind. Removing -Wno-error, for instance, turns + # every warning into an `error:` line the parser cannot see -- zero warnings + # found, build broken, and this is what catches it. + - name: Require every build leg to have succeeded + if: always() && needs.build.result != 'success' && needs.build.result != 'skipped' + run: | + echo "::error title=Build incomplete::The simulator build finished '${{ needs.build.result }}'. Warnings from a build that did not complete are not evidence that there are none." + exit 1 + # Ratchet down on merge. Only ever lowers the baseline, and only from a build that # covered every project in the repo -- a partial run would look like a fix and # quietly discard warnings the missing projects would have reported. @@ -343,6 +422,11 @@ jobs: GH_TOKEN: ${{ github.token }} EXPECTED: ${{ needs.discover.outputs.count }} run: | + # `bash -e` is GitHub's default and `set -uo pipefail` does not clear it. + # With errexit left on, `update`'s exit 2 ("baseline unchanged" -- the usual + # outcome of a merge) would kill this step before the `case` below could + # read it, and the ratchet would report failure on almost every push. + set +e set -uo pipefail shopt -s nullglob files=("$GITHUB_WORKSPACE"/counts/*.tsv) @@ -358,14 +442,14 @@ jobs: echo "::error title=Unusable project count::discover reported '${EXPECTED:-}' as the project count; refusing to rewrite the baseline." exit 1 ;; esac - if [ "${#files[@]}" -ne "$EXPECTED" ]; then + if [ "${#files[@]}" -eq 0 ] || [ "${#files[@]}" -ne "$EXPECTED" ]; then echo "::error title=Incomplete warning coverage::Got ${#files[@]} count file(s) for ${EXPECTED} project(s); refusing to rewrite the baseline from a partial build." exit 1 fi for attempt in 1 2 3; do - # cd is guarded throughout: there is no `set -e` here, and a silent cd - # failure would point the rm/add/commit below at the wrong tree. + # cd is guarded throughout: errexit is off above, and a silent cd failure + # would point the rm/add/commit below at the wrong tree. cd "$GITHUB_WORKSPACE" || exit 1 rm -rf repo git clone --quiet --depth 1 --branch main \ @@ -379,7 +463,11 @@ jobs: case $rc in 0) ;; # changed; commit it 2) echo "Baseline already matches; nothing to commit."; exit 0 ;; - *) echo "::error title=Warning count increased on main::A warning reached main without failing the gate. Refusing to absorb it into the baseline." + 1) echo "::error title=Warning count increased on main::A warning reached main without failing the gate. Refusing to absorb it into the baseline." + exit 1 ;; + 3) echo "::error title=Warning count collapsed::The build reported far fewer warnings than the baseline. That is usually a stripped flag or a broken parser, not 40 fixes; deleting the keys cannot be undone without hand-editing the baseline. Confirm, then re-run with --allow-collapse." + exit 1 ;; + *) echo "::error title=Baseline update failed::warning_baseline.py update exited ${rc}." exit 1 ;; esac @@ -395,7 +483,10 @@ jobs: -m "Generated by .github/workflows/linux-simulator.yml from ${GITHUB_SHA}." \ || { echo "::error title=Commit failed::update rewrote the baseline but git commit failed."; exit 1; } - if git push origin main; then + push_out="$(git push origin main 2>&1)" + push_rc=$? + echo "$push_out" + if [ $push_rc -eq 0 ]; then echo "Baseline lowered." { echo "### ⬇️ Warning baseline lowered" @@ -406,6 +497,20 @@ jobs: } >> "$GITHUB_STEP_SUMMARY" || true exit 0 fi + + # Only a lost race is worth retrying. Branch protection is the rejection + # this job is most likely to meet -- making `warnings` a required check is + # the point of the PR, and required checks apply to direct pushes too, so + # the bot needs a ruleset bypass. Retrying that three times and exiting 0 + # would leave the downward half of the ratchet permanently dead and green. + case "$push_out" in + *GH006*|*"protected branch"*|*"Protected branch"*|*"required status check"*|*"Required status check"*|*"refusing to allow"*) + echo "::error title=Baseline push blocked by branch protection::main rejected the bot's push, so the baseline can never be lowered. Grant github-actions[bot] a bypass for main in the repository ruleset, or drop the ratchet job." + exit 1 ;; + *"Permission to"*|*"403"*|*"denied"*) + echo "::error title=Baseline push denied::The workflow token cannot push to main. Check the job's contents: write permission and any org-level restriction." + exit 1 ;; + esac echo "Push rejected (attempt ${attempt}/3) -- main moved. Recomputing." done From 61dfd81492f726781e970029e781e0f7d97e8706 Mon Sep 17 00:00:00 2001 From: Toby Murray Date: Sat, 8 Aug 2026 21:49:19 -0400 Subject: [PATCH 4/7] fix(ci): scan app.mk and simulator/gcc/Makefile for smuggled suppressions too The flag policy only read una/Makefile, which is where the warning lists live but not the only file that reaches the command line. config/gcc/app.mk is included before them and already sets user_cflags in one project, and simulator/gcc/Makefile exports into the same sub-make; `override user_cflags +=` appends to whatever either left behind. So `-w` was still one line away, in a file the check was not looking at. Both siblings are now scanned for suppressions -- 45 make fragments across the 15 projects rather than 15 -- and the scan keys off the assigned variable name rather than a substring, so it covers CXXFLAGS and compiler_options as well and does not mistake `$(addprefix -W,$(WARN))` for one. --- .github/scripts/warning_baseline.py | 107 ++++++++++++++++++++++------ 1 file changed, 85 insertions(+), 22 deletions(-) diff --git a/.github/scripts/warning_baseline.py b/.github/scripts/warning_baseline.py index bfb7d47f7..f68b715a1 100644 --- a/.github/scripts/warning_baseline.py +++ b/.github/scripts/warning_baseline.py @@ -295,7 +295,18 @@ def diff_counts(observed, allowed): MAKEFILE_SEARCH_ROOTS = ("Examples/Apps", "Docs/Tutorials") +# una/Makefile owns the warning lists, but it is not the only file that reaches the +# command line. config/gcc/app.mk is included before them and already sets +# user_cflags in one project; simulator/gcc/Makefile exports into the same sub-make. +# Both are scanned for suppressions -- checking only una/Makefile would leave `-w` +# one line away in a file nobody was looking at. +SUPPRESSION_SCAN_SIBLINGS = ("config/gcc/app.mk", "simulator/gcc/Makefile") + ASSIGN_RE = re.compile(r"^(?PWARN|CXXWARN)\s*[:+?]?=\s*(?P.*)$") +ANY_ASSIGN_RE = re.compile( + r"^(?:override\s+|export\s+)*(?P[A-Za-z_][A-Za-z0-9_]*)\s*[:+?]?=\s*(?P.*)$" +) +FLAG_VAR_RE = re.compile(r"cflags|cxxflags|compiler_options", re.IGNORECASE) def _logical_lines(text): @@ -303,11 +314,26 @@ def _logical_lines(text): return re.sub(r"\\\n\s*", " ", text).splitlines() +def check_suppressions(text): + """Problems with any make fragment that assigns to a compiler-flag variable.""" + problems = [] + for line in _logical_lines(text): + assign = ANY_ASSIGN_RE.match(line.strip()) + if not assign or not FLAG_VAR_RE.search(assign.group("name")): + continue + name = assign.group("name") + for token in assign.group("value").split(): + if token == "-w": + problems.append(f"{name} disables all warnings with -w") + elif token.startswith("-Wno-") and token not in ALLOWED_CFLAG_SUPPRESSIONS: + problems.append(f"{name} adds the suppression {token}") + return problems + + def check_makefile_flags(text): """Return a list of human-readable problems with one una/Makefile's warning flags.""" problems = [] lists = {} - cflag_tokens = [] uses_warn = set() for line in _logical_lines(text): @@ -316,8 +342,6 @@ def check_makefile_flags(text): if assign: lists[assign.group("name")] = assign.group("value").split() continue - if "user_cflags" in stripped: - cflag_tokens += stripped.split() for var in ("c_compiler_options_local", "cpp_compiler_options_local"): if stripped.startswith(var) and "$(WARN)" in stripped: uses_warn.add(var) @@ -339,13 +363,7 @@ def check_makefile_flags(text): if "-pedantic" not in text: problems.append("-pedantic is gone") - for token in cflag_tokens: - if token == "-w": - problems.append("user_cflags disables all warnings with -w") - elif token.startswith("-Wno-") and token not in ALLOWED_CFLAG_SUPPRESSIONS: - problems.append(f"user_cflags adds the suppression {token}") - - return problems + return problems + check_suppressions(text) def find_una_makefiles(root): @@ -368,27 +386,45 @@ def cmd_flags(args): print(f"FAIL: no una/Makefile found under {'/, '.join(MAKEFILE_SEARCH_ROOTS)}/ in {root}") return 1 + def report(rel, problems): + if not problems: + return 0 + print(f"FAIL {rel}") + for problem in problems: + print(f" - {problem}") + return 1 + failures = 0 + scanned = 0 for rel in makefiles: with open(os.path.join(root, rel), "r", encoding="utf-8", errors="replace") as fh: - problems = check_makefile_flags(fh.read()) - if problems: - failures += 1 - print(f"FAIL {rel}") - for problem in problems: - print(f" - {problem}") + failures += report(rel, check_makefile_flags(fh.read())) + scanned += 1 + + project = posixpath.dirname(posixpath.dirname(rel)) + for sibling in SUPPRESSION_SCAN_SIBLINGS: + sib_rel = posixpath.join(project, sibling) + sib_abs = os.path.join(root, sib_rel) + if not os.path.exists(sib_abs): + continue + with open(sib_abs, "r", encoding="utf-8", errors="replace") as fh: + failures += report(sib_rel, check_suppressions(fh.read())) + scanned += 1 if failures: print( - f"\n{failures} of {len(makefiles)} simulator project(s) weakened their warning " - "flags.\nThe baseline counts warnings the build reports; a flag that is no longer\n" - "requested reports nothing, which reads as a fix and is then ratcheted away for\n" - "good. Retire a warning by removing it from REQUIRED_WARNINGS in this script --\n" - "in one reviewable place -- not from one app's Makefile." + f"\n{failures} file(s) across {len(makefiles)} simulator project(s) weakened the " + "warning flags.\nThe baseline counts warnings the build reports; a flag that is no " + "longer requested\nreports nothing, which reads as a fix and is then ratcheted away " + "for good. Retire a\nwarning by removing it from REQUIRED_WARNINGS in this script -- " + "in one reviewable\nplace -- not from one app's Makefile." ) return 1 - print(f"OK: {len(makefiles)} simulator project(s) still request the required warning flags.") + print( + f"OK: {len(makefiles)} simulator project(s) still request the required warning flags " + f"({scanned} make fragment(s) scanned)." + ) return 0 @@ -811,6 +847,33 @@ def test_detaching_the_list_from_the_command_line_is_caught(self): def test_dropping_pedantic_is_caught(self): self.assertIn("-pedantic is gone", check_makefile_flags(GOOD_MAKEFILE.replace("-pedantic", ""))) + def test_the_real_app_mk_is_accepted(self): + """config/gcc/app.mk legitimately sets user_cflags in one project.""" + self.assertEqual( + check_suppressions("touchgfx_path := ../../ThirdParty/touchgfx\nuser_cflags := -DUSE_BPP=8\n"), + [], + ) + + def test_a_sibling_fragment_smuggling_w_is_caught(self): + """app.mk is included before una/Makefile's own flags and survives the += it + does; scanning only una/Makefile would leave -w one line away.""" + self.assertIn( + "user_cflags disables all warnings with -w", + check_suppressions("user_cflags := -DUSE_BPP=8 -w\n"), + ) + + def test_a_sibling_fragment_smuggling_a_suppression_is_caught(self): + self.assertIn( + "CXXFLAGS adds the suppression -Wno-cast-qual", + check_suppressions("export CXXFLAGS += -Wno-cast-qual\n"), + ) + + def test_the_addprefix_line_is_not_mistaken_for_a_suppression(self): + self.assertEqual( + check_suppressions("cpp_compiler_options_local += -pedantic $(addprefix -W,$(WARN))\n"), + [], + ) + class TestCommittedMakefiles(unittest.TestCase): """Reads the real tree. Weakening the gate now means editing this file, in a diff From 22f608e0d0d3961f8ed718b5e7a57dbf3dfedfaa Mon Sep 17 00:00:00 2001 From: Toby Murray Date: Sat, 8 Aug 2026 21:50:34 -0400 Subject: [PATCH 5/7] fix(ci): don't post a failed gate for a run that was cancelled always() fires on cancellation too, so a push superseded by cancel-in-progress reached the warnings job, found discover 'cancelled', and posted a failing required check for a run nobody was waiting on. A gate that goes red on someone else's second push is the kind that gets turned off. --- .github/workflows/linux-simulator.yml | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/.github/workflows/linux-simulator.yml b/.github/workflows/linux-simulator.yml index 42d380d6f..0ab204d78 100644 --- a/.github/workflows/linux-simulator.yml +++ b/.github/workflows/linux-simulator.yml @@ -282,11 +282,15 @@ jobs: # pull request from a fork. warnings: needs: [ discover, build ] - # Bare always(): this job must reach a conclusion on every run of the workflow. - # Branch protection treats a skipped required check as a satisfied one, so every - # `if:` that could skip this job is a way to merge without being checked -- which - # is why the discover and build results are asserted in steps below instead. - if: always() + # This job must reach a conclusion whatever discover and build did: branch + # protection treats a skipped required check as a satisfied one, so every `if:` + # that could skip it is a way to merge unchecked. The results are asserted in + # steps below instead of in this condition. + # + # !cancelled() rather than always(), which also fires on cancellation: a run + # superseded by cancel-in-progress would otherwise reach this job, find discover + # 'cancelled', and post a failed required check for a push nobody is waiting on. + if: ${{ !cancelled() }} name: Warning ratchet runs-on: ubuntu-24.04 timeout-minutes: 5 @@ -385,7 +389,7 @@ jobs: # every warning into an `error:` line the parser cannot see -- zero warnings # found, build broken, and this is what catches it. - name: Require every build leg to have succeeded - if: always() && needs.build.result != 'success' && needs.build.result != 'skipped' + if: ${{ !cancelled() && needs.build.result != 'success' && needs.build.result != 'skipped' }} run: | echo "::error title=Build incomplete::The simulator build finished '${{ needs.build.result }}'. Warnings from a build that did not complete are not evidence that there are none." exit 1 From c18fb287bc1971bf8482a3270a1e66c610b4bab8 Mon Sep 17 00:00:00 2001 From: Toby Murray Date: Sat, 8 Aug 2026 22:22:45 -0400 Subject: [PATCH 6/7] fix(ci): accumulate WARN appends, and key the summary on the job result Both from CodeRabbit review, both reproduced first. The flag policy read every assignment operator as a replacement, so only the last WARN/CXXWARN line survived. An ordinary `WARN = ...` / `WARN += ...` split reported the first line's flags as missing -- a red required check with a wrong reason -- and, the other way round, a suppression on the first line hid behind a clean += on the last. That second one is a working bypass, not a theoretical one: make emits -Wno-pedantic ahead of the required flags, nothing later re-enables it, and every -Wpedantic entry in the baseline disappears while the check reports OK. Confirmed against gcc. The operators now follow make's own semantics, including ?= not overriding a variable that is already set. The job summary keyed its heading on the gate step's outcome, which is 'skipped' when the selection check, the selftest or the flag policy failed first -- so a red job printed a tick and "no simulator project was selected". It now keys on job.status, distinguishes "nothing to check" from "the comparison never ran", and the build-leg assertion moved above it so the summary can see that failure too. --- .github/scripts/warning_baseline.py | 36 +++++++++++++++++++++++++-- .github/workflows/linux-simulator.yml | 36 ++++++++++++++++----------- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/.github/scripts/warning_baseline.py b/.github/scripts/warning_baseline.py index f68b715a1..550a14af0 100644 --- a/.github/scripts/warning_baseline.py +++ b/.github/scripts/warning_baseline.py @@ -302,7 +302,7 @@ def diff_counts(observed, allowed): # one line away in a file nobody was looking at. SUPPRESSION_SCAN_SIBLINGS = ("config/gcc/app.mk", "simulator/gcc/Makefile") -ASSIGN_RE = re.compile(r"^(?PWARN|CXXWARN)\s*[:+?]?=\s*(?P.*)$") +ASSIGN_RE = re.compile(r"^(?PWARN|CXXWARN)\s*(?P[:+?]?=)\s*(?P.*)$") ANY_ASSIGN_RE = re.compile( r"^(?:override\s+|export\s+)*(?P[A-Za-z_][A-Za-z0-9_]*)\s*[:+?]?=\s*(?P.*)$" ) @@ -340,7 +340,17 @@ def check_makefile_flags(text): stripped = line.strip() assign = ASSIGN_RE.match(stripped) if assign: - lists[assign.group("name")] = assign.group("value").split() + # Follow make's own semantics for the operator. Reading every form as a + # replacement would report the first line's flags as missing on an + # ordinary `WARN = ...` / `WARN += ...` split, and -- worse -- would let + # a suppression on the first line hide behind a clean += on the last. + name, tokens = assign.group("name"), assign.group("value").split() + if assign.group("op") == "+=": + lists.setdefault(name, []).extend(tokens) + elif assign.group("op") == "?=": + lists.setdefault(name, tokens) # make ignores ?= once the var is set + else: + lists[name] = tokens continue for var in ("c_compiler_options_local", "cpp_compiler_options_local"): if stripped.startswith(var) and "$(WARN)" in stripped: @@ -847,6 +857,28 @@ def test_detaching_the_list_from_the_command_line_is_caught(self): def test_dropping_pedantic_is_caught(self): self.assertIn("-pedantic is gone", check_makefile_flags(GOOD_MAKEFILE.replace("-pedantic", ""))) + def test_flags_split_across_an_append_are_accumulated(self): + """`WARN = ...` then `WARN += ...` is ordinary make. Reading only the last + assignment reported everything on the first line as missing.""" + split = GOOD_MAKEFILE.replace( + "WARN = error all extra write-strings", + "WARN = error all extra\nWARN += write-strings", + ) + self.assertEqual(check_makefile_flags(split), []) + + def test_a_suppression_hidden_behind_a_later_append_is_caught(self): + """The bypass the accumulation fixes: put -Wno-pedantic in the first + assignment and every required flag in a clean += on the last. make puts the + suppression on the command line ahead of the flags, where nothing re-enables + it, and reading only the last assignment saw nothing wrong.""" + hidden = GOOD_MAKEFILE.replace("WARN = error", "WARN = no-pedantic\nWARN += error") + self.assertIn("WARN adds the suppression -Wno-pedantic", check_makefile_flags(hidden)) + + def test_a_conditional_assignment_does_not_override_an_earlier_one(self): + """make ignores ?= once the variable is set, so the check must too.""" + conditional = GOOD_MAKEFILE + "WARN ?= all\n" + self.assertEqual(check_makefile_flags(conditional), []) + def test_the_real_app_mk_is_accepted(self): """config/gcc/app.mk legitimately sets user_cflags in one project.""" self.assertEqual( diff --git a/.github/workflows/linux-simulator.yml b/.github/workflows/linux-simulator.yml index 0ab204d78..4719e7c87 100644 --- a/.github/workflows/linux-simulator.yml +++ b/.github/workflows/linux-simulator.yml @@ -366,34 +366,40 @@ jobs: fi exit $rc + # Before the summary, so the summary can see the job go red: a build that died + # has its warnings checked anyway (the gate above does not need needs.build to + # have succeeded), but it must not leave a green required check behind. + # Removing -Wno-error, for instance, turns every warning into an `error:` line + # the parser cannot see -- zero warnings found, build broken, caught here. + - name: Require every build leg to have succeeded + if: ${{ !cancelled() && needs.build.result != 'success' && needs.build.result != 'skipped' }} + run: | + echo "::error title=Build incomplete::The simulator build finished '${{ needs.build.result }}'. Warnings from a build that did not complete are not evidence that there are none." + exit 1 + + # Keyed on job.status, not on the gate step: a failure in the selection check, + # the selftest or the flag policy leaves steps.gate skipped, and reading that + # alone reported a tick and "no project was selected" over a red job. - name: Summary if: always() run: | - ok='${{ steps.gate.outcome }}' + job='${{ job.status }}' + gate='${{ steps.gate.outcome }}' { - if [ "$ok" = failure ]; then echo "### ❌ Warning ratchet"; else echo "### ✅ Warning ratchet"; fi + if [ "$job" = success ]; then echo "### ✅ Warning ratchet"; else echo "### ❌ Warning ratchet"; fi echo "" - if [ "$ok" = skipped ]; then + if [ "$gate" != skipped ]; then + echo '```'; cat /tmp/gate.out 2>/dev/null || echo "(no output)"; echo '```' + elif [ "$job" = success ]; then echo "No simulator project was selected, so there were no warnings to check." echo "The flag policy and the ratchet's own selftest still ran." else - echo '```'; cat /tmp/gate.out 2>/dev/null || echo "(no output)"; echo '```' + echo "The comparison never ran: an earlier step failed. See the job log." fi echo "" echo "Coverage: ${{ steps.gate.outputs.legs || 0 }} of ${{ needs.discover.outputs.count }} selected project(s), out of ${{ needs.discover.outputs.total }} in the repo." } >> "$GITHUB_STEP_SUMMARY" - # Last, so the warning comparison above is always reported first: a build that - # died has its warnings checked (that is the point of always()), but it must not - # leave a green required check behind. Removing -Wno-error, for instance, turns - # every warning into an `error:` line the parser cannot see -- zero warnings - # found, build broken, and this is what catches it. - - name: Require every build leg to have succeeded - if: ${{ !cancelled() && needs.build.result != 'success' && needs.build.result != 'skipped' }} - run: | - echo "::error title=Build incomplete::The simulator build finished '${{ needs.build.result }}'. Warnings from a build that did not complete are not evidence that there are none." - exit 1 - # Ratchet down on merge. Only ever lowers the baseline, and only from a build that # covered every project in the repo -- a partial run would look like a fix and # quietly discard warnings the missing projects would have reported. From 41e5a3bd0f20747ea9b06b77172857b1aa0d2d4b Mon Sep 17 00:00:00 2001 From: Toby Murray Date: Sat, 8 Aug 2026 22:38:19 -0400 Subject: [PATCH 7/7] fix(ci): don't let the site list imply which warning is the new one The key has no line number by design, so for a 2 -> 3 regression the tool cannot know which of the three sites is the new one -- but listing them bare read as if it did, and would send someone to fix a warning the baseline already tolerated. The list now says so, and says how many sites it truncated. Also records, where the flag policy is defined, that it reads assignments rather than the command line: `EXTRA := -w` expanded into a flags variable later still gets through. It is a tripwire against casual weakening, not a sandbox, and closing it properly means observing gcc's actual invocation -- a Makefile change, not a CI one. --- .github/scripts/warning_baseline.py | 35 ++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/.github/scripts/warning_baseline.py b/.github/scripts/warning_baseline.py index 550a14af0..c3d22955a 100644 --- a/.github/scripts/warning_baseline.py +++ b/.github/scripts/warning_baseline.py @@ -233,11 +233,18 @@ def format_regressions(regressions, details=None): path, flag = key label = f"{path} [{flag}]" lines.append(f" {label:<{width}} {allowed} allowed -> {observed} found") - for line, col, msg in sorted((details or {}).get(key, []), key=_site_order)[ - :MAX_SITES_SHOWN - ]: + + sites = sorted((details or {}).get(key, []), key=_site_order) + # The key deliberately has no line number, so which of these is the new one + # is not knowable -- say so rather than let the list imply it. When the + # baseline already tolerates some, every site here is a candidate. + if sites and allowed: + lines.append(f" every site for this key -- {allowed} of them predate this change:") + for line, col, msg in sites[:MAX_SITES_SHOWN]: where = f"{path}:{line}:{col}" if col else f"{path}:{line}" lines.append(f" {where}: {msg}") + if len(sites) > MAX_SITES_SHOWN: + lines.append(f" ... and {len(sites) - MAX_SITES_SHOWN} more") return lines @@ -300,6 +307,12 @@ def diff_counts(observed, allowed): # user_cflags in one project; simulator/gcc/Makefile exports into the same sub-make. # Both are scanned for suppressions -- checking only una/Makefile would leave `-w` # one line away in a file nobody was looking at. +# +# The scan reads assignments, not the command line, so it is a tripwire against +# casual weakening rather than a sandbox: `EXTRA := -w` expanded into a flags +# variable later still gets through. Closing that properly means observing what gcc +# is actually invoked with -- `make -n`, or dropping the `@` from the compile rules +# so the flags reach build.log -- which is a change to the Makefiles, not to CI. SUPPRESSION_SCAN_SIBLINGS = ("config/gcc/app.mk", "simulator/gcc/Makefile") ASSIGN_RE = re.compile(r"^(?PWARN|CXXWARN)\s*(?P[:+?]?=)\s*(?P.*)$") @@ -783,6 +796,22 @@ def test_check_names_the_offending_lines_when_details_are_supplied(self): sys.stdout = real self.assertIn("Libs/c.cpp:7:3: format '%d' expects int", out.getvalue()) + def test_details_do_not_imply_which_site_is_the_new_one(self): + """The key has no line number, so a 2 -> 3 regression cannot say which of the + three is new. Listing them bare would send someone to fix the wrong one.""" + regressions = {("Libs/a.cpp", "-Wformat="): (3, 2)} + details = { + ("Libs/a.cpp", "-Wformat="): {("10", "5", "old"), ("20", "5", "old"), ("30", "5", "new")} + } + out = "\n".join(format_regressions(regressions, details)) + self.assertIn("2 of them predate this change", out) + + def test_a_long_site_list_says_how_many_were_hidden(self): + key = ("Libs/a.cpp", "-Wformat=") + details = {key: {(str(n), "1", "msg") for n in range(MAX_SITES_SHOWN + 3)}} + out = "\n".join(format_regressions({key: (MAX_SITES_SHOWN + 3, 0)}, details)) + self.assertIn("... and 3 more", out) + def test_compare_flags_a_hand_raised_baseline(self): after = self.write("after.txt", "Libs/a.cpp\t-Wformat=\t5\n") self.assertEqual(main(["compare", "--before", self.baseline, "--after", after]), 1)