From afce5aa108fcfa1cb33b1f2a0a2ca2205ede2ebe Mon Sep 17 00:00:00 2001 From: Christopher Williams Date: Thu, 24 Sep 2026 16:50:50 -0400 Subject: [PATCH] =?UTF-8?q?phase12:=20sf3=5Fcc=20--help=20was=20broken=20?= =?UTF-8?q?=E2=80=94=20three=20of=20four=20workers=20tripped=20on=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Worker B, worker C and worker D all probed `tools/sf3_cc` with `--help` during the Phase 12 capability probe, and all three got a raw coreutils `basename` error instead of help: ./tools/sf3_cc: line 29: .run/sf3_cc/Usage: basename NAME [SUFFIX] ... : No such file ANY leading-`-` argument was taken as a source path. `basename --help .c` prints coreutils' own help text, and that entire text then became the output filename, so the failure surfaced as a confusing redirection/cpp error. Every worker lost time on it, on the first command they tried, and worker D spent probe effort diagnosing it as a "genuinely broken path". This is cookbook 179 from the other side. That finding says a rule every worker must follow belongs in a TRACKED tool rather than a file each worker copies. Here the tool WAS tracked -- and it was still unusable the way every single worker reaches for it first. Tracking a tool is necessary but not sufficient; it also has to survive first contact. Fixed: `--help`/`-h` print real help and exit 0, an unknown option is rejected BY NAME, and a missing source file says so rather than failing inside cpp. The compile path is unchanged. Also documented its LIMIT, which was previously unwritten and is easy to misread as a match: the maspsx stage always runs with DEFAULT options and no region option is passed, so this shows a SPELLING's shape and NOT a region's final bytes. A region needing `maspsx=epilogue`, `nopmarker`, `moves`, `regread`, `off` or `gp=`/`cc1=`/`as=` looks different here than under `sf3_match range`. Sweep spellings with this; decide matches with `sf3_match range` -- and decide an `epilogue` token from the CANDIDATE's tail, never the original's (finding 165/180). 9 new tests (tools/tests/test_sf3_cc.py): the argument-handling cases run without a toolchain, including one that pins the exact symptom (no coreutils help text in the output, and no file created named after it); the compile-path test skips cleanly when the ignored toolchain or any src/func_*.c is absent, and one test pins the documented LIMIT so it cannot be dropped. make test 290 tests, OK (from 281) --- tools/sf3_cc | 40 +++++++++++ tools/tests/test_sf3_cc.py | 133 +++++++++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+) create mode 100644 tools/tests/test_sf3_cc.py diff --git a/tools/sf3_cc b/tools/sf3_cc index 65c5467..64c6211 100755 --- a/tools/sf3_cc +++ b/tools/sf3_cc @@ -12,6 +12,23 @@ # # Pipeline: cpp -> cc1 (the pinned 2.7.2-psx build) -> maspsx (the project's vendored patched # version), which is exactly what sf3_match does internally. +# +# PHASE 12 ADDITION -- argument handling, and why it is here rather than in a charter. +# Three of the four Phase 12 workers probed this tool with `--help` and every one got a raw +# coreutils `basename` error, because ANY leading-`-` argument was taken as a source path. +# That is cookbook 179 from the other side: the rule lives in a tracked tool, but the tool +# was not usable the way every worker reaches for it first. A tool that fails confusingly on +# its own help flag costs a worker time on the one path it will always try. So: `--help` +# works, an unknown option is rejected BY NAME, and a missing source file says so instead of +# surfacing a cpp error. The compile path is unchanged. +# +# LIMIT -- READ BEFORE TRUSTING THE OUTPUT. The maspsx stage always runs with DEFAULT options, +# and no region option is passed. This tool shows you a SPELLING's shape, NOT a region's final +# bytes: a region needing a maspsx token (`epilogue`, `nopmarker`, `moves`, `regread`, `off`) +# or `gp=`/`cc1=`/`as=` will look DIFFERENT here than under `sf3_match range`. Sweep SPELLINGS +# with this; decide MATCHES with `sf3_match range`. An `epilogue` row in particular shows the +# UNFILLED tail here, and that token must be decided from the CANDIDATE's tail, never the +# original's (cookbook 165/180). set -e REPO=$(cd "$(dirname "$0")/.." && pwd) cd "$REPO" @@ -21,7 +38,30 @@ if [ $# -lt 1 ]; then exit 2 fi +case "$1" in + -h|--help) + echo "usage: sf3_cc -> prints cc1 assembly (maspsx applied) for ONE file" + echo + echo 'The spelling-sweep tool: one spelling costs ~0.1 s, which is what turns' + echo '"try a few variants" into "grid the whole space". Pair it with tools/sf3_diff,' + echo "which names WHICH DIMENSION a residual lives in -- compile+diff together remove" + echo "both the cost of a spelling and the guesswork about what is wrong." + echo + echo "Env: SF3_CC_OUT (default .run/sf3_cc) selects the scratch directory." + exit 0 + ;; + -*) + echo "sf3_cc: unknown option: $1" >&2 + echo "usage: sf3_cc " >&2 + exit 2 + ;; +esac + SRC=$1 +if [ ! -f "$SRC" ]; then + echo "sf3_cc: no such source file: $SRC" >&2 + exit 2 +fi BASE=$(basename "$SRC" .c) OUT=${SF3_CC_OUT:-.run/sf3_cc} mkdir -p "$OUT" diff --git a/tools/tests/test_sf3_cc.py b/tools/tests/test_sf3_cc.py new file mode 100644 index 0000000..aaefd47 --- /dev/null +++ b/tools/tests/test_sf3_cc.py @@ -0,0 +1,133 @@ +"""Synthetic-only tests for the spelling-sweep compile tool. + +`tools/sf3_cc` is a POSIX shell wrapper: cpp -> cc1 (the pinned 2.7.2-psx build) -> maspsx. +Only its ARGUMENT HANDLING is tested here without a toolchain, because that is the part that +broke: in Phase 12 **three of the four workers** probed it with `--help` and every one got a +raw coreutils `basename` error, since any leading-`-` argument was taken as a source path. +Every one of them lost time on it, on the first command they tried. + +Cookbook 179 says a rule every worker must follow belongs in a TRACKED tool rather than in a +file each worker copies. This is the same failure from the other side: the tool WAS tracked, +but it was not usable the way every worker reaches for it first. + +The compile-path test skips cleanly when the ignored local toolchain or any ``src/func_*.c`` +is absent, in the style of the existing local-toolchain skips. It never reads the disc or the +game executable -- ``sf3_cc`` reads only the C source it is handed. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import tempfile +import unittest +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] +TOOL = REPO / "tools" / "sf3_cc" +CC1 = REPO / "tools" / "old-gcc" / "gcc-2.7.2-psx" / "cc1" +CPP = REPO / "tools" / "old-gcc" / "gcc-2.7.2-psx" / "cpp" +MASPSX = REPO / "tools" / "maspsx" / "maspsx.py" + + +def _toolchain_available() -> bool: + return TOOL.is_file() and CC1.is_file() and CPP.is_file() and MASPSX.is_file() + + +def _any_source() -> Path | None: + return next(iter(sorted((REPO / "src").glob("func_*.c"))), None) + + +def _run(args: list[str], out_dir: Path | None = None) -> subprocess.CompletedProcess: + env = dict(os.environ) + if out_dir is not None: + env["SF3_CC_OUT"] = str(out_dir) + return subprocess.run( + [str(TOOL), *args], cwd=REPO, capture_output=True, text=True, env=env + ) + + +class ArgumentHandlingTests(unittest.TestCase): + """The defect: a `-`-leading argument was treated as a source path. + + `basename --help .c` prints coreutils' own help, and that whole text then became the + output filename, so the failure surfaced as a confusing redirection/cpp error rather + than as "unknown option". + """ + + def test_help_exits_zero_and_names_the_usage(self) -> None: + result = _run(["--help"]) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("usage: sf3_cc", result.stdout) + + def test_short_help_alias_also_works(self) -> None: + result = _run(["-h"]) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("usage: sf3_cc", result.stdout) + + def test_help_output_does_not_leak_coreutils_basename_help(self) -> None: + """The exact confusing symptom three workers saw.""" + result = _run(["--help"]) + combined = result.stdout + result.stderr + self.assertNotIn("Report bugs to", combined) + self.assertNotIn("basename NAME", combined) + self.assertNotIn("GNU coreutils", combined) + + def test_no_arguments_exits_two_with_usage_on_stderr(self) -> None: + result = _run([]) + self.assertEqual(result.returncode, 2) + self.assertIn("usage: sf3_cc", result.stderr) + + def test_unknown_option_exits_two_and_names_the_option(self) -> None: + result = _run(["-x"]) + self.assertEqual(result.returncode, 2) + self.assertIn("-x", result.stderr) + self.assertIn("usage: sf3_cc", result.stderr) + + def test_a_missing_source_file_says_so_rather_than_failing_in_cpp(self) -> None: + result = _run(["definitely-not-here.c"]) + self.assertEqual(result.returncode, 2) + self.assertIn("definitely-not-here.c", result.stderr) + self.assertIn("no such source file", result.stderr) + + def test_an_option_like_argument_is_not_accepted_as_a_path(self) -> None: + """`--help` must not create a file named after coreutils' help text.""" + with tempfile.TemporaryDirectory() as tmp: + result = _run(["--help"], out_dir=Path(tmp)) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(list(Path(tmp).iterdir()), []) + + +@unittest.skipUnless(_toolchain_available(), "ignored local toolchain is absent") +class CompilePathTests(unittest.TestCase): + """The path workers actually use: one spelling costs ~0.1 s.""" + + def setUp(self) -> None: + source = _any_source() + if source is None: + self.skipTest("no tracked src/func_*.c to compile") + self.source = source + self._tmp = tempfile.TemporaryDirectory() + self.addCleanup(self._tmp.cleanup) + self.out = Path(self._tmp.name) + + def test_compiles_a_real_source_to_assembly(self) -> None: + result = _run([str(self.source.relative_to(REPO))], out_dir=self.out) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn(".end", result.stdout) + self.assertTrue(any(self.out.iterdir())) + + def test_the_documented_limit_is_stated_in_the_source(self) -> None: + """A tool's LIMITS belong in the tool: sf3_cc passes no region option. + + A worker who trusts this output as a region's final bytes will mis-read every + `epilogue`/`nopmarker`/`gp=` row, so the limit must be written down where it is read. + """ + text = TOOL.read_text() + self.assertIn("LIMIT", text) + self.assertIn("DEFAULT options", text) + + +if __name__ == "__main__": + unittest.main()