fix(ebuild): repair SyntaxError in dispatch.py and unify backend errors - #78
Conversation
## Problem
`ebuild/build/dispatch.py` does not parse. `BackendDispatcher.configure()`
carries two `else:` clauses attached to the same `if` chain:
File "ebuild/build/dispatch.py", line 133
else:
^^^^
SyntaxError: invalid syntax
Every `ebuild` command that reaches the dispatcher imports this module, so
on master `ebuild build`, `ebuild configure` and `ebuild clean` all fail at
import, and pytest aborts collection with 2 errors before running anything.
The cause is a merge, not a typo. Two branches independently added an
unhandled-backend guard to `configure()` — one raising `ValueError` listing
`ALL_BACKENDS`, one raising `RuntimeError` with a ninja-specific hint. The
merge kept both bodies. Their two test suites survived as well and disagree:
`tests/ebuild/test_dispatch.py` expects `ValueError`, `tests/unit/test_dispatch.py`
expects `RuntimeError`.
Two further defects were preserved in the same chain:
- `configure()` listed `ninja` alongside make/kbuild as a silent no-op.
`ebuild build` routes `backend: ninja` with no `targets` into the
dispatcher (`commands.py`: `if resolved_backend != "ninja" or not
cfg.targets`), which has no ninja configure step. Returning quietly there
is the exact false-success the `RuntimeError` guard was written to stop.
- The `ValueError` message advertised `ALL_BACKENDS`, which contains
`ninja`, so rejecting `ninja` produced a message naming `ninja` as
supported.
## Fix
- Remove the duplicated `else:` and consolidate the two guards into one.
- Add `UnknownBackendError(ValueError, RuntimeError)`. Deriving from both
keeps each pre-existing caller contract intact rather than silently
dropping one, and it is load-bearing beyond the tests: `ebuild build`
handles this through `except RuntimeError`, so a plain `ValueError` would
reach the user as a traceback instead of a clean `exit 1`.
- Name the backends each step actually handles (`CONFIGURE_BACKENDS`,
`BUILD_BACKENDS`, `CLEAN_BACKENDS`) and report those in the message, so it
can no longer contradict the rejection. `clean` keeps `ninja`; it only
removes a build directory and needs no toolchain.
- Drop `ninja` from the `configure()` no-op branch so it raises, carrying the
actionable "requires 'targets' in build.yaml" hint.
Behaviour change: `BackendDispatcher.configure("ninja")` previously returned
None and now raises. Nothing in-tree relies on the old behaviour — the CLI
`configure` command routes ninja to `_configure_ninja_backend` before the
dispatcher is constructed.
## Testing
- `pytest tests/ebuild/test_dispatch.py tests/unit/test_dispatch.py` —
31 passed. Both previously-conflicting suites pass unmodified.
- Full suite: 196 passed, 1 skipped, 11 failed. All 11 failures are
pre-existing and unrelated (`_object_path` missing from `NinjaBackend`,
initramfs integration, build-failure output); they were present before
this change and are unaffected by it.
- End-to-end, a `build.yaml` with `backend: ninja` and no `targets`:
[error] Unknown build backend 'ninja'. BackendDispatcher can
configure: cargo, cmake, kbuild, make, meson. ebuild's own ninja
backend is invoked directly rather than through BackendDispatcher,
and requires 'targets' in build.yaml -- add targets or choose
another backend.
EXIT=1
Clean message, correct exit code, no traceback.
- Six regression tests added covering the dual-inheritance contract, the
ninja rejection, the actionable hint, the non-contradictory backend list,
`clean("ninja")` still working, and cargo/make/kbuild remaining no-ops.
Signed-off-by: Prakhar Maheshwari <mpr@stordocktech.com>
Signed-off-by: Prakhar Maheshwari <prakharmaheshwari96@gmail.com>
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Correct diagnosis and a correct fix — you and #66 found the same thing and
resolved it the same way.
I verified both repair master:
origin/master SyntaxError: invalid syntax, line 133
#66 parses OK
#78 (this) parses OK
#75 SyntaxError — still line 133, despite its title
Tracked as #87.
You also both reached the same conclusion about why the two error paths
existed. #66 puts it this way:
the two blocks also disagreed about the exception —
tests/ebuildexpects
ValueError("Unknown build backend '<name>'")whiletests/unitexpects
RuntimeErrormatching "ninja". Both are legitimate readings: an unrecognized
name is a bad argument, and a "ninja" that reaches the dispatcher is a CLI
routing failure.
and resolves it with UnknownBackendError(ValueError, RuntimeError) so both
suites keep passing. "Unify backend errors" in your title is the same idea.
#66 is already approved and additionally restores
NinjaBackend._object_path(), which a cflags merge had deleted, so it is the one
that will land.
What that leaves you
Nothing here is wrong; it is simply second to arrive at the same place. Rather
than resolving this against #66, I would close it and pick up something that is
not already covered — #85 (eos and eBoot defining colliding CMake target names,
which stops ebuild integration configuring) is open and in this area.
If any part of this diff does something #66 does not, say which and I will look
again — I compared the dispatch.py outcomes and the error-type reasoning, not
every line of the tests.
Two people independently identifying the same splice and choosing the same
resolution is a good signal about the fix. It is also a signal about the process
that let it land: required_status_checks is null on this repository, so
nothing imports the merge result before it becomes master. That is the part
worth fixing once #66 is in.
2 participants
Summary
Fix a syntax error in
ebuild/build/dispatch.pycaused by duplicateelse:clauses inBackendDispatcher.configure().The merge conflict left two different unknown-backend guards in the same
ifchain, causingebuild build,ebuild configure, andebuild cleanto fail during import.Changes
else:and consolidated the backend validation logic.UnknownBackendError, inheriting from bothValueErrorandRuntimeError, to preserve the existing caller contracts.CONFIGURE_BACKENDSBUILD_BACKENDSCLEAN_BACKENDSninjafrom the configure no-op path so unsupportedninjaconfiguration fails with an actionable error.ninjasupported forclean, where no toolchain is required.ValueError/RuntimeErrorinheritanceninjarejection during configureclean("ninja")continuing to workcargo,make, andkbuildno-op behaviorTesting
Targeted tests