From 074fa17a5575b032f9b5092b9622b2dac48a2ea1 Mon Sep 17 00:00:00 2001 From: Prakhar Maheshwari Date: Sun, 30 Aug 2026 17:42:41 +0530 Subject: [PATCH] fix(ebuild): restore NinjaBackend._object_path dropped in merge ## Problem `NinjaBackend.generate()` cannot produce a build. Both writers call `self._object_path(...)`, but the method does not exist: ebuild/build/ninja_backend.py:143: in _write_ninja obj = str(self._object_path(target, src)) AttributeError: 'NinjaBackend' object has no attribute '_object_path' This is ebuild's own backend -- the path `ebuild build` takes for any `build.yaml` that declares `targets`. It is unusable, and 5 tests fail on it purely incidentally, none of which is actually about object paths. `_object_path()` and its 137 lines of tests were added by 9cb7c89 ("give each target its own object files"). Merge 5ef3a61 kept both call sites but dropped the method, and replaced `tests/unit/test_ninja_backend.py` wholesale, taking the regression tests with it. With no test left asserting the behaviour, nothing caught the loss -- it surfaced only as an `AttributeError` in unrelated tests. ## Fix Restore `_object_path()` verbatim from 9cb7c89. Objects are named `obj//.o` rather than from the source alone. Sharing a source between targets is ordinary -- a library and a test binary using one helper, or one source built twice with different defines. Naming the object from the source alone makes both targets claim the same output, which ninja rejects outright, and silently drops one target's cflags before it gets that far. Objects under `obj//` match what CMake, Meson and Bazel do for the same reason. Target output names are unchanged, so nothing downstream moves. ## Testing Restored the three dropped regression tests as `TestObjectPathNamespacing` in `tests/unit/test_ninja_backend.py`, adapted to that file's current layout: - `test_shared_source_gets_one_object_per_target` -- a source used by two targets yields two distinct compile edges, and both targets' defines survive. - `test_shared_source_manifest_is_valid_ninja` -- runs real `ninja -n` against the generated manifest. Looking right is not the same as loading. - `test_compile_commands_distinguishes_shared_source_entries` -- the two compilation-database entries for a shared source name different objects. The third was strengthened over the original: it asserted only that the two command strings differ, which held even with the bug present, because the two targets carry different `-D` flags. It now compares the `-o` paths, the property a consumer actually keys on. Verified the tests detect the bug rather than merely passing. With the pre-9cb7c89 source-keyed naming reinstated, all three fail, and real ninja gives the original error: ninja: error: _build\build.ninja:34: multiple rules generate _build/src/util.o With the fix: 3 passed. Full suite: 205 passed, 6 failed (was 11 failed before this change). All 6 remaining failures are pre-existing and unrelated to object paths. Note: `ninja>=1.11` is a declared dependency but was absent from the environment, so ninja-dependent tests had been silently skipping. Installing it unmasked a further pre-existing defect -- generated paths are not escaped, so a build directory containing a space produces an invalid manifest ("expected build command name"). Confirmed independent of this change by reproducing it with the old object naming in place. It will be sent separately. Signed-off-by: Prakhar Maheshwari Signed-off-by: Prakhar Maheshwari --- CHANGELOG.md | 8 ++ ebuild/build/ninja_backend.py | 16 ++++ tests/unit/test_ninja_backend.py | 127 +++++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59f6dde3..46d39f08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,14 @@ ## [Unreleased] ### Fixed +- **`NinjaBackend` could not generate anything.** `_object_path()` is called by + both `_write_ninja()` and `_write_compile_commands()`, but the method itself + was dropped in a merge, so every `generate()` raised + `AttributeError: 'NinjaBackend' object has no attribute '_object_path'`. + The method and its regression tests are restored: objects are named + `obj//.o`, so a source listed by two targets compiles once + per target instead of both targets claiming one output — which ninja rejects + with `multiple rules generate ...` (`ebuild/build/ninja_backend.py`). - **`ebuild.build.dispatch` was unimportable.** Two branches independently added an unhandled-backend `else` clause to `BackendDispatcher.configure()`; the merge kept both, leaving a second `else` after the first and a `SyntaxError` diff --git a/ebuild/build/ninja_backend.py b/ebuild/build/ninja_backend.py index 5aa2f374..4ef73ce4 100644 --- a/ebuild/build/ninja_backend.py +++ b/ebuild/build/ninja_backend.py @@ -58,6 +58,22 @@ def generate(self) -> None: self._write_ninja() self._write_compile_commands() + def _object_path(self, target, src: str) -> Path: + """Object file path for *src* as compiled by *target*. + + Object paths are namespaced by target name. Two targets may legitimately + list the same source: a library and a test binary sharing a helper, or + one source built twice with different defines. Each needs its own + object, because each compiles with its own cflags. Keying only on the + source made both targets claim one output, which ninja rejects with + "multiple rules generate ...". + + Example: + >>> backend._object_path(target, "src/main.c") # target.name == "app" + PosixPath('_build/obj/app/src/main.o') + """ + return (self.build_dir / "obj" / target.name / src).with_suffix(".o") + def _get_toolchain_cflags(self) -> List[str]: """Return toolchain-level cflags including sysroot. diff --git a/tests/unit/test_ninja_backend.py b/tests/unit/test_ninja_backend.py index e973ca06..d5e09ac7 100644 --- a/tests/unit/test_ninja_backend.py +++ b/tests/unit/test_ninja_backend.py @@ -3,13 +3,18 @@ """Unit tests for ebuild.build.ninja_backend.NinjaBackend.""" +import json +import subprocess import sys import tempfile import unittest from pathlib import Path from types import SimpleNamespace +import pytest + from ebuild.build.ninja_backend import NinjaBackend +from ebuild.build.toolchain import ResolvedToolchain from ebuild.core.config import ProjectConfig, TargetConfig @@ -66,5 +71,127 @@ def test_static_library_unaffected(self): self.assertNotIn("-dynamiclib", ninja) +@pytest.mark.ebuild +class TestObjectPathNamespacing: + """Object files are namespaced by target: ``obj//.o``. + + Two targets may legitimately list the same source -- a library and a test + binary sharing a helper, or one source built twice with different defines. + Naming an object from the source alone makes both targets claim one + output, which ninja rejects with "multiple rules generate ...", and + silently drops one target's cflags before it ever gets there. + """ + + @staticmethod + def _shared_source_config(tmp_path, app_sources): + return ProjectConfig( + name="shared_source", + version="1.0.0", + targets=[ + TargetConfig( + name="util", + target_type="static_library", + sources=["src/util.c"], + defines=["BUILD_LIB=1"], + ), + TargetConfig( + name="app", + target_type="executable", + sources=app_sources, + defines=["BUILD_APP=1"], + ), + ], + source_dir=tmp_path, + ) + + def test_shared_source_gets_one_object_per_target(self, tmp_path): + """A source used by two targets must compile to two distinct objects.""" + config = self._shared_source_config(tmp_path, ["src/main.c", "src/util.c"]) + + build_dir = tmp_path / "_build" + NinjaBackend(config, build_dir, ResolvedToolchain()).generate() + + ninja_content = (build_dir / "build.ninja").read_text(encoding="utf-8") + + # Split on the rule separator rather than the first colon: on Windows + # the object path starts with a drive letter. + outputs = [ + line[len("build "):].split(": cc ", 1)[0].strip() + for line in ninja_content.splitlines() + if line.startswith("build ") and ": cc " in line + ] + assert len(outputs) == 3, f"expected 3 compile edges, got {outputs}" + assert len(set(outputs)) == 3, f"duplicate object outputs: {outputs}" + + # Each target's defines must survive onto its own object. + assert "-DBUILD_LIB=1" in ninja_content + assert "-DBUILD_APP=1" in ninja_content + + def test_shared_source_manifest_is_valid_ninja(self, tmp_path, monkeypatch): + """The generated manifest must load in real ninja, not just look right. + + Ninja treats two edges producing one output as an error, so this is + the check that actually proves the generated build is usable. + + The build directory is relative and the working directory is the + project root, mirroring how ``ebuild build`` invokes the backend. + """ + pytest.importorskip("ninja", reason="ninja package not installed") + + src_dir = tmp_path / "src" + src_dir.mkdir() + (src_dir / "util.c").write_text( + "int util_answer(void) { return 42; }\n", encoding="utf-8" + ) + (src_dir / "main.c").write_text( + "int util_answer(void);\n" + "int main(void) { return util_answer() == 42 ? 0 : 1; }\n", + encoding="utf-8", + ) + + config = self._shared_source_config(tmp_path, ["src/main.c", "src/util.c"]) + + monkeypatch.chdir(tmp_path) + build_dir = Path("_build") + NinjaBackend(config, build_dir, ResolvedToolchain()).generate() + + # -n parses and validates the manifest without running the compiler, + # so this test needs no toolchain on the machine running it. + result = subprocess.run( + [sys.executable, "-m", "ninja", "-f", str(build_dir / "build.ninja"), "-n"], + cwd=str(tmp_path), + capture_output=True, + text=True, + ) + assert result.returncode == 0, ( + "ninja rejected the generated manifest:\n" + f"{result.stdout}\n{result.stderr}" + ) + + def test_compile_commands_distinguishes_shared_source_entries(self, tmp_path): + """compile_commands.json entries for a shared source must differ. + + Two targets compiling one file legitimately produce two entries; each + needs its own -o so a consumer can tell them apart. + """ + config = self._shared_source_config(tmp_path, ["src/util.c"]) + + build_dir = tmp_path / "_build" + NinjaBackend(config, build_dir, ResolvedToolchain()).generate() + + cc_data = json.loads( + (build_dir / "compile_commands.json").read_text(encoding="utf-8") + ) + shared = [e for e in cc_data if e["file"] == "src/util.c"] + assert len(shared) == 2 + assert shared[0]["command"] != shared[1]["command"] + + # Differing commands alone are not enough -- the per-target defines + # would differ regardless. It is the object each entry names that has + # to be distinct, which is what a consumer keys on. + objects = [e["command"].split(" -o ", 1)[1].strip() for e in shared] + assert len(set(objects)) == 2, f"entries name the same object: {objects}" + + if __name__ == "__main__": unittest.main()