validate-board-config: follow source ${SRC}/config/boards inheritance#9748
validate-board-config: follow source ${SRC}/config/boards inheritance#9748igorpecovnik merged 1 commit intomainfrom
Conversation
…eritance `Validate changed board configs` (PR #9747 CI run 25258999953) failed on the three Ayn-Odin2 derivative boards with: ERROR: config/boards/ayn-odin2mini.csc: BOARDFAMILY: required, ... ERROR: config/boards/ayn-odin2mini.csc: KERNEL_TARGET: required, ... (same pair on ayn-odin2portal.csc and ayn-thor.csc) The validator's header explicitly says it does NOT source the file ("boards have side-effecty function bodies"), and the regex `parse_assignments` only sees top-level `KEY=value` lines. Boards that consist of one `source "${SRC}/config/boards/<parent>.csc"` line plus a handful of overrides (BOARD_NAME, BOARD_VENDOR, BOARD_MAINTAINER, ARCH) inherit BOARDFAMILY / KERNEL_TARGET / KERNEL_TEST_TARGET from the parent, so the validator saw them as missing and erred out. The errors are pre-existing — they would fire on any future change to those .csc files. PR #9747 just made them visible by being the first PR to touch an inheriting board since the validator landed. Add `collect_inherited_assignments`: when a top-level `source` line points at another file under config/boards/ (matched by a regex anchored to start-of-line so `source` calls inside function bodies — which are always indented — aren't followed), parse that file's top-level assignments and lay them behind the child's own. Child's explicit values still win via dict.setdefault. Recursion is guarded by a visited set keyed on resolved paths, so a self-source or a parent-source-child cycle terminates instead of looping. Missing source targets are silently skipped — the main required-field checks will still flag any field that ends up unset after the merge, so a typo'd source path doesn't mask a real gap. Verified against the five PR-changed files: 6 errors → 0 errors, 4 warnings → 1 warning (the unmaintained-board warning on aml-t95z-plus.tvb, which is unrelated to inheritance and was already there). Self-contained boards (musepipro.conf etc.) produce identical output to before this change.
📝 WalkthroughWalkthroughThis pull request adds support for resolving inherited board configuration fields referenced via ChangesBoard Configuration Inheritance
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 6/8 reviews remaining, refill in 11 minutes and 53 seconds.Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/validate-board-config.py (1)
134-135: 💤 Low valueMinor: sourced file is read twice.
The file at
sourcedis read inside the recursivecollect_inherited_assignments()call (line 134, at its line 124), and then again at line 135 forparse_assignments(). For deeply inherited chains or large files, this doubles I/O.You could refactor to return both the parsed assignments and the source matches from a single read, or have
collect_inherited_assignmentsreturn the text as well. Low priority given typical file sizes.♻️ Possible optimization
def collect_inherited_assignments(path: Path, _visited: set[Path] | None = None) -> dict[str, str]: ... text = path.read_text(errors="replace") + own_fields = parse_assignments(text) inherited: dict[str, str] = {} for m in _SOURCE_RE.finditer(text): sourced = path.parent / m.group(1) if not sourced.is_file(): continue ancestors = collect_inherited_assignments(sourced, _visited) - parent_fields = parse_assignments(sourced.read_text(errors="replace")) - for k, v in parent_fields.items(): - inherited.setdefault(k, v) for k, v in ancestors.items(): inherited.setdefault(k, v) + # Merge own fields last so they take precedence + for k, v in own_fields.items(): + inherited.setdefault(k, v) return inheritedThis reads each file once and lets the recursion handle parent parsing. Caller would then just use the returned dict directly without calling
parse_assignmentsseparately invalidate().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/validate-board-config.py` around lines 134 - 135, The code reads the same file twice by calling collect_inherited_assignments(sourced, _visited) (which itself reads sourced) and then calling parse_assignments(sourced.read_text(...)) again; refactor collect_inherited_assignments to return the parsed assignments (or the file text) along with the current ancestors so callers can reuse that result, then replace the second call to parse_assignments(...) in validate() with the parsed data returned from collect_inherited_assignments; update recursive calls inside collect_inherited_assignments to propagate the parsed/text return value so each file is read only once (refer to functions collect_inherited_assignments and parse_assignments and the variable parent_fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/validate-board-config.py`:
- Around line 134-135: The code reads the same file twice by calling
collect_inherited_assignments(sourced, _visited) (which itself reads sourced)
and then calling parse_assignments(sourced.read_text(...)) again; refactor
collect_inherited_assignments to return the parsed assignments (or the file
text) along with the current ancestors so callers can reuse that result, then
replace the second call to parse_assignments(...) in validate() with the parsed
data returned from collect_inherited_assignments; update recursive calls inside
collect_inherited_assignments to propagate the parsed/text return value so each
file is read only once (refer to functions collect_inherited_assignments and
parse_assignments and the variable parent_fields).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e56c5fb1-bf3c-492f-acb5-2f23e17eda22
📒 Files selected for processing (1)
tools/validate-board-config.py
Summary
Extend
tools/validate-board-config.pyto recognise thesource "${SRC}/config/boards/<parent>.csc"pattern that derivative boards use, and treat fields the parent declares as inherited by the child. Today three Ayn-Odin2 derivative boards (ayn-odin2mini.csc,ayn-odin2portal.csc,ayn-thor.csc) use this pattern: each is onesourceline plus a handful of overrides, withBOARDFAMILY/KERNEL_TARGET/KERNEL_TEST_TARGETdefined exclusively in the parentayn-odin2.csc. The validator's regex-only parser couldn't see them and would emit:This is a pre-existing latent bug — it would fire on any future change to those files. PR #9747 (which adds explicit
ARCH=arm64to those boards among others) is what surfaced it.Why not just source the bash file
The validator's own header explains:
So the fix has to stay regex-only. Adding a parallel regex for the
sourcepattern keeps that constraint.Implementation
New
collect_inherited_assignments(path):^source\s+["']?\$\{?SRC\}?/config/boards/(<file>)["']?(anchored to start-of-line sosourcecalls inside function bodies — which are always indented — aren't followed).dict.setdefault. Child's explicit values still win.Verified
Pre-fix: 6 errors, 3
KERNEL_TEST_TARGETwarnings (KERNEL_TEST_TARGET="current"is set on the parent — now correctly inherited).Self-contained boards (
musepipro.conf,bananapif3.conf,musebook.conf,orangepi5.conf, …) produce byte-identical validator output to before this change — they have nosourcedirective so the inheritance walk is a no-op.Cycle protection verified with a synthetic fixture (a file sourcing itself → terminates cleanly, no infinite loop).
Companion PR
#9747 declares
ARCH=arm64explicitly on five inheriting boards. ItsValidate changed board configsjob is currently failing on the same three.cscderivatives this PR fixes; #9747 needs this PR (or its content) onmainbefore the boards PR can pass CI.Test plan
Summary by CodeRabbit