Validate toolchain extra_cflags/extra_ldflags as string lists

[swayam-2003]

Summary

• Remove duplicate else in BackendDispatcher.configure() that caused a SyntaxError and blocked importing the package.
• Unknown backends now raise RuntimeError consistently in configure()/build().
• Validate toolchain.extra_cflags and toolchain.extra_ldflags as string lists so scalar YAML values cannot silently split into per-character flags.

Approach

Followed existing target-field validation patterns in config.py and regression tests in tests/unit/test_dispatch.py.

Testing

python -m pip install -e ".[dev]"
python -m pytest tests/unit/test_dispatch.py tests/ebuild/test_config_validation.py -v

21 tests passed locally.

DCO

Signed-off-by: Swayam Nayak 154440440+swayam-2003@users.noreply.github.com

[Copilot]

🟡 Changes recommended

Existing repo tests still assert ValueError for unknown backends and the build() error message currently presents a contradictory supported-backend list for ninja, so the test suite and messaging need alignment.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes a Python syntax error in BackendDispatcher.configure() that prevented importing the package, and tightens config parsing so toolchain flag fields are validated as lists of strings (avoiding accidental per-character flag splitting from scalar YAML values).

Changes:

• Remove the duplicate else path in BackendDispatcher.configure() and standardize unknown/unhandled backend failures to raise RuntimeError in configure()/build().
• Add toolchain.extra_cflags / toolchain.extra_ldflags validation in load_config to require list[str].
• Add regression tests for toolchain flag type validation and document the behavior in the changelog.

File summaries

File	Description
tests/ebuild/test_config_validation.py	Adds regression tests ensuring toolchain flag fields are lists of strings.
ebuild/core/config.py	Validates extra_cflags/extra_ldflags types before constructing ToolchainConfig.
ebuild/build/dispatch.py	Fixes configure() syntax error and changes unknown/unhandled backend failures to RuntimeError.
CHANGELOG.md	Documents the dispatcher import fix and new toolchain flag validation rules.

Review details

Suppressed comments (1)

ebuild/build/dispatch.py:131

• configure() now raises RuntimeError for unhandled/unknown backends, but the existing tests/ebuild/test_dispatch.py::TestUnknownBackend suite still asserts ValueError for unknown backend names (e.g. bazel, gradle, scons). Unless those tests are intentionally deprecated, this change will break the default pytest run (pytest.ini discovers tests/**/test_*.py). Please update that test suite (and any docs/docstrings that still mention ValueError) to match the new exception type/contract.

        else:
            raise RuntimeError(
                f"BackendDispatcher cannot configure backend '{backend}'. "
                "This dispatcher only handles cmake, meson, and cargo "
                "(make/kbuild need no configure step). ebuild's own ninja "

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[srpatcha]

The flags validation is the valuable half, and I confirmed it

extra_cflags: -O2 -Wall is the natural thing to write, and YAML gives you a string. ToolchainConfig annotates the field List[str] and nothing enforces it, so the string is passed straight through to code that iterates it:

YAML gives:        '-O2 -Wall'  (str)
iterating it:      ['-', 'O', '2', ' ', '-', 'W', 'a', 'l', ...]

Every character becomes a separate compiler flag. The build then fails with something about an unrecognised option -, which points at nothing the developer wrote. A type annotation is documentation, not a check, and this is the case that shows the difference.

Rejecting it at load with a message naming the field is right. It is the same treatment #73 gives a malformed packages: section, and the same class as several others in this repo: a value of the wrong shape silently accepted, and the consequence surfacing somewhere unrelated.

The dispatcher half is superseded

Three PRs fix the same SyntaxError: #66, #70 and this one. I have approved #66 — it came first and is the wider change. My own #70 I have marked superseded for the same reason.

Standardising on RuntimeError for unknown backends is correct and is what #66 does too, so that part lands either way.

What I would like

Rebase onto #66 and reduce this to the config.py validation plus its tests. That should be a small branch with no conflicts, and I will merge it — the flags bug is not fixed by #66 or by anything else open, and it is the piece that would otherwise be lost.

Worth extending while you are there, if you want: extra_ldflags and extra_cflags are not the only List[str] fields that a scalar would break. sources, includes, defines and depends on a target have the same shape, and sources: src/main.c is at least as easy to write as the list form.

Verification

Confirmed the scalar-splitting behaviour on current master with a direct YAML load. Not verified this branch merged — it conflicts with #66 in dispatch.py, which is the overlap described above.

[swayam-2003]

Thanks for the review @srpatcha — rebased onto latest master and removed the dispatch.py changes (superseded by #66). This PR now contains only the toolchain flag validation in config.py + tests + CHANGELOG. Ready for another look.

[Copilot]

🟡 Changes recommended

The PR title/description includes BackendDispatcher fixes, but the provided diff only updates config validation/tests/changelog, creating a scope mismatch that must be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[srpatcha]

The toolchain-flag validation here is a genuine find and I want it kept. The
syntax repair in the title is not in the diff, though — I checked, because two
other PRs claim the same fix and I wanted to know which actually deliver it.

The syntax error is still present on this branch

$ git show pr75:ebuild/build/dispatch.py > /tmp/d75.py
$ python3 -c "import ast; ast.parse(open('/tmp/d75.py').read())"
  line 133: invalid syntax
   129                 f"Unknown build backend '{backend}'. "
   130                 f"Supported backends: {', '.join(sorted(ALL_BACKENDS))}"
   131             )
   132
   133         else:
   134             raise RuntimeError(

Byte-identical to master. For comparison, #66 and #78 both parse after their
changes.

Tracked as #87. #66 is the one I would take for it — it removes the duplicated
error path that caused the splice rather than deleting the stray else:.

The flags validation is the part worth saving

This is a real bug and I verified it independently. extra_cflags given as a
string is iterated character by character:

>>> list('-O2 -Wall')
['-', 'O', '2', ' ', '-', 'W', 'a', 'l', 'l']

Every character becomes its own compiler argument. The build then fails with
something unrelated-looking, or worse, silently drops the flags the developer
asked for — a -O2 that never reached the compiler is not a visible failure, it
is a slow binary nobody can explain.

Rejecting a scalar where a list is expected, with a message naming the field, is
exactly right, and I have not seen this covered anywhere else in the open PRs.

Suggestion

Rebase onto #66 and drop the dispatch.py hunks that overlap it, leaving this
PR as the flags validation alone. It then reviews on its own merits, has no
conflict to settle, and the part that is uniquely yours is not blocked behind a
syntax fix that two other PRs are also trying to land.

Retitling would help too — as it stands the title promises a repair the diff does
not contain, which is how a reviewer ends up merging it expecting master to be
fixed.

Happy to approve once it is scoped to the validation.

[Copilot]

🟡 Changes recommended

Added validation for extra_ldflags item types is not covered by the new tests, leaving a gap in regression protection.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[swayam-2003]

Rebased onto latest upstream/master (d3958f7). Previous CI failures were caused by a stale base that still had the dispatch.py SyntaxError — that blocked pytest collection entirely. This PR now contains only toolchain flag validation + tests + CHANGELOG. All 42 related tests pass locally after rebase.