From 6e873ac8fb1558aaf14a827b90db8bf7470b0130 Mon Sep 17 00:00:00 2001 From: srpatcha Date: Fri, 28 Aug 2026 23:30:41 -0700 Subject: [PATCH] =?UTF-8?q?fix(build):=20unbreak=20master=20=E2=80=94=20eb?= =?UTF-8?q?uild=20build=20raised=20a=20traceback=20on=20every=20project?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `master` cannot run. Not a stale badge — reproduced from a clean checkout: $ ebuild new hi && cd hi && ebuild build File ".../ebuild/build/dispatch.py", line 133 else: ^^^^ SyntaxError: invalid syntax $ pytest 2 errors during collection Four defects, each found by running the tool rather than reading it. Every one of them reached master because the file it lives in is imported lazily, so nothing on the merge path executed it. 1. `dispatch.py` had two consecutive `else:` blocks in `configure()` — one raising ValueError, one RuntimeError. Python does not parse a module until something imports it, so this sat on master reachable only by the command that touches it, and it also made `tests/unit` uncollectable. The RuntimeError arm is kept: `tests/unit/test_dispatch.py` documents the intent, which is that an unhandled backend fails loudly rather than silently doing nothing and letting the caller report a false success. 2. `configure()` then listed "ninja" among the backends that need no configure step, so `configure("ninja")` silently did nothing — precisely the regression that test guards against. ebuild's own ninja backend is generated and invoked by the CLI, never dispatched here, so arriving with it is a routing mistake and is now reported. 3. `NinjaBackend._object_path` was called from two places and defined in neither: AttributeError on every ninja build, immediately after the SyntaxError was cleared. Objects are namespaced by target name, because a source shared by two targets must produce two distinct objects — ninja rejects two edges writing one output, and the targets may use different cflags. The path is flattened rather than mirrored so that a source from outside the project cannot place its object outside the build directory, where `clean` would not find it. 4. ninja was invoked as `python -m ninja`, which only works with the PyPI wheel installed, so a machine with a real ninja on PATH failed with "No module named ninja". `ninja_command()` prefers the executable. Also, `build()` and `clean()` raised ValueError for the same condition `configure()` reports as RuntimeError, so no single `except` guarded the dispatcher. All three now raise RuntimeError, and the two test files that disagreed about which to expect agree. On shared libraries: the `link_shared` rule was declared and never used — edges went through the generic `link` rule with -shared pushed into ldflags. That works, but leaves the rule dead and broke `test_shared_library_uses_shared_link_rule`. Edges use `link_shared` now, and the platform's flag lives in the rule, so darwin's -dynamiclib is decided in one place instead of at the call site. `test_static_library_unaffected` asserted "-shared" was absent from the whole file, which cannot hold while the rule preamble declares it; it checks build edges instead. before pytest: 2 errors during collection — suite cannot run after 202 passed `ebuild build` now reaches the compiler and reports a real compile error instead of a Python traceback. The remaining first-run gap — templates including with no path to it — is #64's scope, not this fix's. Co-Authored-By: Claude Opus 5 (1M context) --- ebuild/build/dispatch.py | 34 +++++++++++++++++----------- ebuild/build/ninja_backend.py | 39 +++++++++++++++++++++++++++----- tests/ebuild/test_dispatch.py | 12 ++++++---- tests/unit/test_ninja_backend.py | 18 +++++++++++---- 4 files changed, 76 insertions(+), 27 deletions(-) diff --git a/ebuild/build/dispatch.py b/ebuild/build/dispatch.py index 0d708df..a6a10cf 100644 --- a/ebuild/build/dispatch.py +++ b/ebuild/build/dispatch.py @@ -11,6 +11,7 @@ import logging import subprocess +import shutil import sys from pathlib import Path from typing import Any, Dict, List, Optional @@ -73,6 +74,19 @@ def _run_or_log( return subprocess.run(cmd, check=check, cwd=cwd) +def ninja_command() -> list: + """How to invoke ninja on this machine. + + A real ninja on PATH is preferred; `python -m ninja` only works when the + PyPI `ninja` wheel is installed, so hardcoding it made a machine with + ninja properly installed fail with "No module named ninja". + """ + exe = shutil.which("ninja") + if exe: + return [exe] + return [sys.executable, "-m", "ninja"] + + class BackendDispatcher: """Dispatch configure/build/clean to external build systems. @@ -100,7 +114,7 @@ def configure( dry_run: If True, log commands instead of executing them. Raises: - ValueError: If the backend is not recognized. + RuntimeError: If the backend is not recognized. """ config = config or {} self.build_dir.mkdir(parents=True, exist_ok=True) @@ -121,15 +135,9 @@ def configure( elif backend == "cargo": pass # Cargo does not have a separate configure step - elif backend in ("make", "kbuild", "ninja"): + elif backend in ("make", "kbuild"): pass # No separate configure step - else: - raise ValueError( - f"Unknown build backend '{backend}'. " - f"Supported backends: {', '.join(sorted(ALL_BACKENDS))}" - ) - else: raise RuntimeError( f"BackendDispatcher cannot configure backend '{backend}'. " @@ -154,7 +162,7 @@ def build( dry_run: If True, log commands instead of executing them. Raises: - ValueError: If the backend is not recognized. + RuntimeError: If the backend is not recognized. """ config = config or {} @@ -185,7 +193,7 @@ def build( _run_or_log(cmd, dry_run) else: - raise ValueError( + raise RuntimeError( f"Unknown build backend '{backend}'. " f"Supported backends: {', '.join(sorted(ALL_BACKENDS))}" ) @@ -203,7 +211,7 @@ def clean( dry_run: If True, log commands instead of executing them. Raises: - ValueError: If the backend is not recognized. + RuntimeError: If the backend is not recognized. """ if backend == "cmake": _run_or_log( @@ -232,12 +240,12 @@ def clean( ) elif backend == "ninja": _run_or_log( - [sys.executable, "-m", "ninja", "-C", str(self.build_dir), "-t", "clean"], + ninja_command() + ["-C", str(self.build_dir), "-t", "clean"], dry_run, check=False, ) else: - raise ValueError( + raise RuntimeError( f"Unknown build backend '{backend}'. " f"Supported backends: {', '.join(sorted(ALL_BACKENDS))}" ) diff --git a/ebuild/build/ninja_backend.py b/ebuild/build/ninja_backend.py index 5aa2f37..43f142e 100644 --- a/ebuild/build/ninja_backend.py +++ b/ebuild/build/ninja_backend.py @@ -9,6 +9,7 @@ from __future__ import annotations import json +import re import sys from dataclasses import dataclass, field from pathlib import Path @@ -30,6 +31,14 @@ class PackagePaths: "-fno-pie", "-fno-PIE"} +def _shared_flag() -> str: + """The compiler flag that builds a shared object on this platform. + + macOS links dynamic libraries with -dynamiclib; ELF platforms use -shared. + """ + return "-dynamiclib" if sys.platform == "darwin" else "-shared" + + class NinjaBackend: """Generate build.ninja from a ProjectConfig and resolved toolchain. @@ -104,6 +113,24 @@ def _resolve_target_cflags(self, target) -> List[str]: return cflags + def _object_path(self, target, src: str) -> Path: + """Object file for one source within one target. + + Called from two places and defined in neither, so every ninja build + raised AttributeError before this existed. + + Namespaced by target name: a source shared by two targets must produce + two distinct objects. Ninja rejects two edges writing the same output, + and the targets may compile it with different cflags. + + The source's directory structure is flattened into the filename rather + than mirrored beneath the build directory. Mirroring lets a source from + outside the project -- ``../shared/util.c`` -- place its object outside + the build directory too, where ``clean`` will not find it. + """ + flat = re.sub(r"[^A-Za-z0-9_.-]", "_", str(src).replace("\\", "/")) + return self.build_dir / "obj" / target.name / f"{flat}.o" + def _write_ninja(self) -> None: """Write the build.ninja file.""" ninja_path = self.build_dir / "build.ninja" @@ -124,7 +151,7 @@ def _write_ninja(self) -> None: " description = LINK $out", "", "rule link_shared", - " command = $cc -shared $ldflags $in -o $out $libs", + f" command = $cc {_shared_flag()} $ldflags $in -o $out $libs", " description = LINK_SHARED $out", "", "rule ar_rule", @@ -190,11 +217,11 @@ def _write_ninja(self) -> None: if target.target_type == "static_library": lines.append(f"build {out}: ar_rule {' '.join(obj_files)}") else: - # Shared libraries need the platform's "build a shared - # object" flag and the same -L/-l wiring executables get, - # neither of which the generic `link` rule provides. + # Shared libraries need the same -L/-l wiring executables + # get, which the rule preamble alone does not supply. The + # shared-object flag itself lives in the link_shared rule, + # so it must not be repeated here. ldflags = list(target.ldflags) - ldflags.insert(0, "-dynamiclib" if sys.platform == "darwin" else "-shared") libs = [] for pkg_name in target.uses: pkg = self.package_paths.get(pkg_name) @@ -204,7 +231,7 @@ def _write_ninja(self) -> None: for lib in pkg.libraries: libs.append(f"-l{lib}") - lines.append(f"build {out}: link {' '.join(obj_files)}") + lines.append(f"build {out}: link_shared {' '.join(obj_files)}") if ldflags: lines.append(f" ldflags = {' '.join(ldflags)}") if libs: diff --git a/tests/ebuild/test_dispatch.py b/tests/ebuild/test_dispatch.py index aa71c39..fd49e35 100644 --- a/tests/ebuild/test_dispatch.py +++ b/tests/ebuild/test_dispatch.py @@ -65,21 +65,25 @@ def test_cmake_takes_priority_over_makefile(self, tmp_path): class TestUnknownBackend: - """Unknown backends must raise ValueError rather than silently skip.""" + """Unknown backends must raise rather than silently skip. + + All three methods raise RuntimeError for this one condition, so a caller + can guard the whole dispatcher with a single ``except RuntimeError``. + """ def test_configure_unknown_raises(self, tmp_path): d = BackendDispatcher(tmp_path, tmp_path / "build") - with pytest.raises(ValueError, match="Unknown build backend 'bazel'"): + with pytest.raises(RuntimeError, match="bazel"): d.configure("bazel") def test_build_unknown_raises(self, tmp_path): d = BackendDispatcher(tmp_path, tmp_path / "build") - with pytest.raises(ValueError, match="Unknown build backend"): + with pytest.raises(RuntimeError, match="Unknown build backend"): d.build("gradle") def test_clean_unknown_raises(self, tmp_path): d = BackendDispatcher(tmp_path, tmp_path / "build") - with pytest.raises(ValueError, match="Unknown build backend"): + with pytest.raises(RuntimeError, match="Unknown build backend"): d.clean("scons") diff --git a/tests/unit/test_ninja_backend.py b/tests/unit/test_ninja_backend.py index e973ca0..b10c7c1 100644 --- a/tests/unit/test_ninja_backend.py +++ b/tests/unit/test_ninja_backend.py @@ -40,9 +40,12 @@ def test_shared_library_gets_shared_flag(self): shared_flag = "-dynamiclib" if sys.platform == "darwin" else "-shared" self.assertIn(shared_flag, ninja) - # It must use the `link` rule (compiler driver), not `ar_rule`. + # It must go through a compiler-driver rule, not the `ar` archiver. + # link_shared is that rule, and it carries the shared-object flag, so + # the flag is never repeated in the edge's ldflags. lib_line = next(line for line in ninja.splitlines() if "libmylib" in line and line.startswith("build")) - self.assertIn(": link ", lib_line) + self.assertIn(": link_shared ", lib_line) + self.assertNotIn(": ar_rule", lib_line) def test_shared_library_gets_lib_dirs_and_libs(self): target = TargetConfig( @@ -62,8 +65,15 @@ def test_static_library_unaffected(self): ninja = self._generate("static", target) self.assertIn(": ar_rule", ninja) - self.assertNotIn("-shared", ninja) - self.assertNotIn("-dynamiclib", ninja) + + # The link_shared *rule* is always declared in the preamble, so the + # bare string "-shared" appears in every generated file. What must be + # absent is any build *edge* that uses it. + edges = [line for line in ninja.splitlines() if line.startswith("build ")] + self.assertTrue(edges, "no build edges were generated") + for edge in edges: + self.assertNotIn(": link_shared ", edge) + self.assertNotIn(": link ", edge) if __name__ == "__main__":