fix(cad): parse whole s-expression blocks, and ship a board to run them on - #94
Merged
srpatcha merged 5 commits intoSep 1, 2026
Merged
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Rebuilt on current master. Master has since fixed the dispatch.py SyntaxError and restored NinjaBackend._object_path, so embeddedos-org#66's versions of those are dropped in favour of what landed — including its choice of RuntimeError for an unknown backend, which is now consistent across both dispatch test suites. What follows is what master still does not have. **build.ninja is invalid on Windows.** Ninja splits build statements on unescaped spaces and colons, so a Windows absolute path puts a drive-letter colon where Ninja expects the separator between outputs and the rule name: ninja: error: build.ninja:20: expected build command name build C:\...\main.o: cc main.c ^ near here Every generated file was rejected before a command ran; a POSIX path containing a space fails identically. _ninja_path() escapes `$`, `:` and ` `, applied to build-statement paths only — variable values (cflags, ldflags) are read to end of line and are left alone, since escaping them hands the compiler mangled flags. Four regression tests, one asserting each build statement contains exactly one unescaped colon. **test_shared_library_uses_shared_link_rule fails on macOS.** It asserts the literal "-shared", but _shared_flag() correctly returns "-dynamiclib" on darwin. The implementation is right and the test was not; made it platform-aware. This is the one test failing on master today. **The Windows leg of the test matrix has never run.** `Run test suite` uses backslash line continuations, which PowerShell rejects: ParserError: Missing expression after unary operator '--'. Marked `shell: bash`, which GitHub provides on Windows runners. **macos-13 is a retired runner image**, so those jobs are never assigned a runner and sit queued until they time out. Every other workflow here already uses macos-latest. **`mypy .` checks nothing.** It aborts with `Duplicate module named "tests"` because layers/eosuite/ vendors its own tests/ package. The step is continue-on-error, so this went unnoticed. Excluding layers/ makes it check 84 files; it stays continue-on-error, so the 12 pre-existing findings are visible without gating the build. **ci.yml has no concurrency group**, alone among this repo's workflows, so pushes pile up queued runs competing for the same scarce runners. Also gitignored _build/, which the suite leaves in the repo root. Verified: pytest 292 passed (287 + the fixed shared-flag test + 4 new). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With `shell: bash` the Windows leg of the matrix runs pytest for the first
time, and it fails four tests. Neither cause is new; both were simply never
executed.
`test_test_target_links_like_an_executable` picks the link edge out of
build.ninja with `l.split(":")[0]`, meaning "the text before the rule
separator". On Windows the first colon is the drive letter, so that expression
returns "build C" for every line, the `.o` filter never matches, and the test
selects the compile edge and asserts `": link "` against it.
Splitting on the first *unescaped* colon is what was meant, and is now
unambiguous: `_ninja_path()` writes the drive colon as `$:` and leaves exactly
one bare colon per statement, the separator.
build C$:\...\obj\t_smoke\t.c.o: cc t.c -> outputs end in .o (compile)
build C$:\...\t_smoke.exe: link ... -> outputs do not (link)
The three `test_integration_initramfs_security.py` cases fail with WinError 2:
`_create_initramfs()` drives find(1) and cpio(1) directly and neither exists on
a stock Windows runner, so they die before reaching the command-injection
behaviour they exist to check. Building a Linux initramfs is not a Windows
operation, so they skip when the tools are absent — the same shape as the
existing skip in test_ninja_backend.py when no host C compiler is present.
Verified: pytest 292 passed locally; the Windows selection logic checked
against an escaped drive-letter path directly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…em on
tools/cad_pipeline.py extracts its blocks with non-greedy regexes:
re.search(r'\(eos_cpu(.*?)\)', content, re.DOTALL)
re.finditer(r'\(eos_peripheral(.*?)\)', content, re.DOTALL)
`(.*?)\)` stops at the FIRST `)`. In
(eos_cpu
(arch "aarch64")
(core "cortex-a57")
that is the paren closing (arch ...), so the match ends after the block's first
attribute and every later one falls out of it and takes its dataclass default.
The result is silent and plausible. A board declaring cortex-a72 at 1800 MHz
parses as cortex-a57 at 1000 -- CpuDef's defaults -- and every eos_peripheral
comes back as unknown/unknown/0x0/irq -1 regardless of what the file says. The
region and peripheral *counts* are right, so nothing looks wrong:
file: core=cortex-a72 freq=1800 fpu=crypto-neon entry=my_entry
parsed: {"arch": "aarch64", "core": "cortex-a57", "freq_mhz": 1000}
Those values flow straight into the generated artifacts. The DTS emitted
compatible = "unknown" and reg = <0x0 0x00000000> for every device; the CMake
toolchain emitted -mcpu for the wrong core. A linker script and device tree
describing hardware that is not the board is worse than a failure, because it
builds.
_sexpr_body() replaces both regexes with a depth counter that skips quoted
strings, so a block ends where it actually ends.
Why this was never caught: the pipeline had no test and no input in this
repository. The only board it is ever pointed at is
eFab/samples/eos_reference_board.kicad_pcb, and embeddedos-org/eFab does not
exist -- `gh api` returns 404 -- so both eos CI jobs that invoke this tool die
at the checkout and the tool itself had never run anywhere.
samples/eos_reference_board.kicad_pcb gives it one: the five memory regions the
eos simulation workflow asserts on (FLASH_NOR, RAM_LPDDR4, UART0_PL011,
HEAP_REGION, OTA_SCRATCH), a full CPU block, and two peripherals.
tests/unit/test_cad_pipeline.py covers the parse and the generated artifacts.
Against the unfixed parser three of its seven fail, including the decisive one
-- a board whose values differ from every default must not read back as the
defaults.
Verified: pytest 302 passed. Stacked on embeddedos-org#66; on master the macOS leg pins the
retired macos-13 image, so a branch off master cannot get a green run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Caught by the new test on the Windows leg:
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x97 in position 152
cad_pipeline.py reads the board with encoding="utf-8" but writes all five
artifacts with a bare open(..., "w"), which uses the platform default -- cp1252
on Windows. The generated banners carry em dashes and box-drawing characters,
so on Windows the linker script, device tree, recipe, toolchain and manifest
were written in a different encoding than the one the tool itself reads.
Asymmetric read/write encoding in a code generator is a bug in the generator,
not in the caller that reads its output as UTF-8.
The artifact test now also decodes each file as UTF-8 rather than only checking
it is non-empty, so this cannot regress silently.
Verified: pytest 302 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kartikey1306
force-pushed
the
fix/cad-pipeline-parser
branch
from
September 1, 2026 09:38
13ccbb5 to
4543fa6
Compare
srpatcha
previously approved these changes
Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
tools/cad_pipeline.pyextracts its blocks with non-greedy regexes:(.*?)\)stops at the first). Inthat is the paren closing
(arch ...)— so the match ends after the block's first attribute, and every later one falls outside it and silently takes its dataclass default.What that looks like
Every
eos_peripheralcomes back asunknown / unknown / 0x0 / irq -1regardless of what the file says. And the region and peripheral counts are correct, so nothing looks wrong.Those values flow straight into the generated artifacts:
compatible = "unknown"andreg = <0x0 0x00000000>for every device-mcpu=for the wrong coreA linker script and device tree describing hardware that is not the board is worse than a failure, because it builds.
_sexpr_body()replaces both regexes with a depth counter that skips quoted strings, so a block ends where it actually ends.Why this was never caught
The pipeline has no test and no input in this repository. The only board it is ever pointed at is
eFab/samples/eos_reference_board.kicad_pcb, andembeddedos-org/eFabdoes not exist:Both eos CI jobs that invoke this tool die at that checkout, so the tool had never run anywhere — not in CI, not locally.
samples/eos_reference_board.kicad_pcbgives it one: the five memory regions the eos simulation workflow asserts on (FLASH_NOR,RAM_LPDDR4,UART0_PL011,HEAP_REGION,OTA_SCRATCH), a full CPU block, and two peripherals. That also unblocks eos's CAD job, which can point at this instead of a repository that isn't there.Tests
tests/unit/test_cad_pipeline.py— 7 tests covering the parse and the generated artifacts. Against the unfixed parser, three fail:The decisive one is the second CPU test: a board whose every value differs from the defaults must not read back as the defaults. A test using only the sample's own values would pass against the broken parser, since the sample happens to match
CpuDef's defaults.Verification
pytest tests/Stacked on #66 — on
masterthe macOS leg still pins the retiredmacos-13image, so a branch off master cannot get a green run at all.🤖 Generated with Claude Code