fix: avoid ninja object path collisions

[aman-sharma-dev]

Summary

Fix Ninja object-file path collisions when source files in the same target have the same basename but different extensions.

Previously, sources such as src/start.c and src/start.S both mapped to src/start.o, causing Ninja to reject the generated build graph due to duplicate outputs.

Object paths now preserve the source extension, producing start.c.o and start.S.o.

Type of Change

• feat — New feature
• fix — Bug fix
• docs — Documentation only
• style — Formatting, no code change
• refactor — Code restructuring without behavior change
• test — Add or fix tests
• build — Build system or dependency changes
• ci — CI/CD pipeline changes
• perf — Performance improvement

Changes

• Preserve source extensions when generating Ninja object-file paths.
• Add regression coverage for same-basename sources with different extensions and generated output-path consistency.

Testing

• Unit tests pass (ctest --test-dir build --output-on-failure)
• Integration tests pass
• Manual testing performed
• New tests added for new functionality

Focused Ninja backend tests: 19 passed.

Ninja dry-run validation (ninja -n): passed.

Full Python test suite: 667 passed, 10 failed, 7 skipped. The 10 failures were reproduced against the original backend and are unrelated to this change.

Pre-Submission Checklist

• Code compiles without warnings (-Wall -Wextra -Werror for C)
• All existing tests pass
• New tests added for new functionality
• Documentation updated if API changed
• Commit messages follow convention
• Branch is rebased on latest master

Related Issues

Fixes #149

Screenshots / Logs

N/A

Additional Notes

The full-suite failures are pre-existing and reproduce without this change.

Object paths under _build/obj/ change with this fix. Clean existing build directories once after upgrading so static archives cannot retain objects named by the previous layout. Durable archive recreation is tracked separately in #136 and PR #137.

[srpatcha]

Nice catch, and thanks for parametrizing the real-ninja test rather than only asserting on the manifest text — I ran it locally with ninja installed and both the .c/.S case and the existing shared-source case pass. The fix in ebuild/build/ninja_backend.py:188 is the right shape and stays inside the per-target namespace, so it composes with #131 and #125 without conflicts.

Two housekeeping points only:

• TASKS.md:55 — the "Completed" section is a table; the new ### T-006 heading sits above it and the ID collides with the T-006 row #126 adds. A table row (or a new ID) would keep the file consistent.
• • Since object names in compile_commands.json change (main.o → main.c.o), a one-liner under ### Fixed in CHANGELOG.md would help anyone diffing IDE indexes after upgrading.
    Approving; happy to merge once TASKS.md is tidied.

[srpatcha]

Review — ebuild#133 "fix: avoid ninja object path collisions"

head: 5cc570d author: aman-sharma-dev ci: none reported (no check runs on this head)

Verdict: A correct one-line fix for a real ninja failure, and — unusually — every number in the PR's evidence reproduces. util.c and util.S in one target previously both claimed obj/app/src/util.o, which real ninja rejects; keying the object on the full source name fixes it. The one thing the PR does not say is that changing the object naming scheme strands the old objects in every existing build tree, and for static_library targets that produces a duplicate-member archive rather than a clean rebuild. That interacts directly with open PR #137.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	ebuild/build/ninja_backend.py:188 (effect at :210)	Renaming objects src/util.o → src/util.c.o leaves the old artifacts behind — ninja does not delete outputs it no longer declares. For executables that is only wasted disk. For static_library targets the archive rule is $ar rcs $out $in (:210), and ar r replaces members by name and inserts new ones; it never removes a member absent from the input list. So a lib<name>.a built before this change keeps util.o and gains util.c.o: the same translation unit twice. Verified empirically — after that sequence ar t lists both members and nm reports symbol f defined twice in one archive. The consumer then gets a duplicate-symbol link error, or silently links the stale object. A clean build is unaffected; anyone who just pulls is not. This is exactly the defect open PR #137 ("recreate static archives to remove stale objects") fixes.	Land #137 first, or before it: note the requirement in the PR body and in CHANGELOG.md that existing build directories must be removed once (ebuild clean / rm -rf _build). The durable fix is #137's — do not reimplement it here.
2	Low	CHANGELOG.md (absent)	No changelog entry. TASKS.md:54-60 invokes ORCHESTRATION.md:12-14 ("a one-line fix needs no design review or changelog entry") and records the skip explicitly, which is the right process. But the escape hatch is for a change with no observable effect, and this one renames every object file and every .d depfile on disk in every project built by ebuild — see finding 1 for why that is user-visible. The repo's own ### Fixed section already documents a comparable ninja-edge change ("ebuild test now finds Windows test binaries"). Brief item 11.	Add to ## [Unreleased] / ### Fixed: object paths now carry the full source filename (src/start.c → obj/<target>/src/start.c.o), so sources differing only by extension no longer collide; existing build directories must be cleaned once.
3	Low	TASKS.md:52-60	T-006 is written as a prose block wedged between the ## Completed heading and the table header, while T-001 and T-003 — the only other entries in that section — are table rows with ID / Task / Owner / Verified by / Evidence. The section now has two incompatible formats and the new entry is not in the table it precedes.	Move it into the table as a T-006 row, evidence column carrying the counts already written.

What I verified, and what it confirms

Everything the PR body and TASKS.md claim about test results reproduces on this host. Stated plainly because it is rarer than it should be:

Claim in the PR	Result here
"Focused regression suite: 19 passed"	19 passed — pytest tests/unit/test_ninja_backend.py -q.
"before the fix, 3 failed and 16 passed"	3 failed, 16 passed — reverting :188 to .with_suffix(".o") fails exactly test_shared_source_manifest_is_valid_ninja[app_sources1] and both test_object_outputs_preserve_source_extensions cases. The [app_sources1] case is the real-ninja dry run over util.c + util.S, so the bug is demonstrated against ninja itself, not against an assertion.
"Changed-file Ruff ... passed"	All checks passed — ruff check ebuild/build/ninja_backend.py tests/unit/test_ninja_backend.py.
"Full suite: 667 passed, 10 failed, 7 skipped ... nine missing PackageRecipe.to_dict() errors and one environment permission error"	674 passed, 9 failed, 1 skipped here. The 9 failures are precisely the to_dict ones in tests/unit/test_index_sync.py; the 10th (permission) and the skip-count difference are host-specific and do not contradict the claim. Those 9 are unrelated to this PR and are what #124 fixes.

The parametrisation of test_shared_source_manifest_is_valid_ninja is the right shape for this: it keeps the existing main.c/util.c case and adds the extension-collision case to the same real-ninja assertion rather than asserting on generated text only.

Architecture conformance

Conforms. §21 places ebuild in Tier 1 — Foundation, and object-path layout for the ninja backend is squarely eBuild's own concern. §5.1 is not engaged: the diff adds no import, no link edge and no manifest entry, so no dependency points anywhere, let alone up a tier. §9.1 makes eBuild the component that owns Configure/Build; generating the build manifest is that responsibility, not a leak of it.

Brief item 8 (API/wire compatibility) is the one worth naming and the PR is silent on it: the object-path layout under _build/obj/ is not a declared compatibility contract in §23.2 — that table covers the eBuild project format, not the build directory — so this is not a contract break. It is still an on-disk format change with a migration consequence, which is findings 1 and 2.

Proposed changes

1. Sequence with #137, or state the one-time clean requirement in the body and the changelog (finding 1). Smallest correct sequence: #137 merges, then this.
2. Add the ### Fixed changelog line (finding 2).
3. Move the T-006 note into the ## Completed table (finding 3).

No change is needed to ninja_backend.py:188 itself — src + ".o" is the right expression, and building the Path from the joined string rather than with_suffix is what makes it correct for multi-dot names too.

Not checked

• CI — nothing to check. GitHub reports no check runs at all on head 5cc570de. I therefore have no CI evidence for anything; all results above are local, against a git archive export of this head in a clean venv built from pyproject.toml[dev]. The local ebuild clone has a dirty tree and was left untouched.
• mypy — NOT RUN. Not installed on this host.
• Windows and macOS — NOT RUN. TASKS.md's neighbouring entries cite a Windows host; the _exe_suffix/.dll/.dylib branches at ninja_backend.py:268-276 were not exercised. Finding 1 reasons about ar and was verified with GNU ar on Linux only; the archive behaviour of llvm-ar and of MSVC lib.exe under a cross toolchain was not tested.
• A real incremental rebuild across the rename — NOT RUN as an end-to-end build. Finding 1's mechanism was verified directly with cc/ar/nm on a minimal archive, not by building an ebuild project before and after the change.
• I did not check whether any project template, example, or .gitignore under templates/ or examples/ names object files by their old path; grep found no obj/ references in docs/ or README.md, but I did not sweep every tree.

---

Automated architecture review of 5cc570de88f3 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[aman-sharma-dev]

The linked-issue policy check is failing because #133 currently has no closing issue. I found #136, but I don't think Fixes #136 is semantically correct since #137 is the PR addressing that issue. Is there an existing issue you want #133 to close, or should #133 be linked differently?

[srpatcha]

Review — ebuild#133 "fix: avoid ninja object path collisions"

head: d1db5e7 author: aman-sharma-dev ci: fail (policy / Policy / Linked Issue)

Verdict: Follow-up review. All three findings from the last review are addressed; ninja_backend.py and test_ninja_backend.py are byte-identical to the head I reviewed at 5cc570de, so the new commits are documentation only and introduce no new defect. I re-ran the suite at this head and every number in the PR's evidence reproduces. No new findings. The only thing left is the linked-issue policy check, which you have already raised in the thread — answer below rather than a finding.

Status of the previous review's findings

Prior #	Status	Evidence
1 (High) — object rename strands old .o files; ar rcs never removes a member, so a pre-existing lib<name>.a keeps both util.o and util.c.o	Addressed	The recommendation was "land #137 first, or note the requirement in the PR body and CHANGELOG.md". The changelog now carries it: "Clean existing build directories once after upgrading so static archives cannot retain objects named by the previous layout." TASKS.md T-006 repeats it and correctly attributes the durable fix — "durable archive recreation is tracked by #136 / PR #137" — rather than reimplementing it here, which is what the finding asked for. I confirmed #137 has not landed: origin/master:ebuild/build/ninja_backend.py:207 is still command = $ar rcs $out $in, and #136 is still open. So the hazard is real and the interim treatment is the right one. The PR body itself still does not mention the one-time clean; the changelog is the artifact users actually read, so I am not holding this open.
2 (Low) — no changelog entry	Resolved in d1db5e74	CHANGELOG.md ## [Unreleased] / ### Fixed gains a five-line entry naming the old and new object paths and the clean requirement.
3 (Low) — T-006 written as prose wedged above the ## Completed table	Resolved in d1db5e74	T-006 is now a row in that table, in its ID / Task / Owner / Verified by / Evidence column order, alongside T-001 and T-003. The section has one format again.

Findings

None. No new defect in the new commits, and nothing left open from the last review.

Two notes that are not findings:

• The parametrised tests live in TestObjectPathNamespacing (tests/unit/test_ninja_backend.py:130), which is a plain class, not a unittest.TestCase — so @pytest.mark.parametrize and the tmp_path/monkeypatch fixtures work as intended. I checked because the two unittest.TestCase classes above it in the same file would have silently broken both. They collect and run: 19 tests, 4 of them parametrised cases.
• _object_path building the Path from src + ".o" rather than with_suffix(".o") is still the right expression, and is what keeps multi-dot source names (foo.pb.c) correct too.

Re-verified at this head

Everything the PR body and TASKS.md claim reproduces. Stated because the last review said the same and the claims did not drift when the branch moved:

Claim in the PR	Result here
"Focused Ninja backend tests: 19 passed"	19 passed in 0.16s — pytest tests/unit/test_ninja_backend.py -q.
"before the fix, 3 failed, 16 passed"	3 failed, 16 passed — reverting :188 to .with_suffix(".o") in a copy of the tree fails exactly test_shared_source_manifest_is_valid_ninja[app_sources1] and both test_object_outputs_preserve_source_extensions cases, and nothing else. [app_sources1] is the real-ninja dry run over util.c + util.S, so the bug is demonstrated against ninja, not against an assertion.
"Ninja dry-run validation (ninja -n): passed"	Confirmed — /usr/bin/ninja is present on this host and the dry-run case is one of the 19.
"Full Python test suite: 667 passed, 10 failed, 7 skipped; the 10 reproduce against the original backend"	9 failed, 674 passed, 1 skipped here, against 9 failed, 671 passed, 1 skipped on an unpatched origin/master export in the same venv. Identical failure set — all nine are the pre-existing AttributeError: 'PackageRecipe' object has no attribute 'to_dict' in tests/unit/test_index_sync.py, which #124 fixes — and exactly +3 passing tests, which is the three cases this PR adds. The author's 10th failure is a host-specific permission error and the skip-count differs by host; neither contradicts the claim.
"Changed-file Ruff passed"	All checks passed, exit 0 — ruff 0.16.5 check ebuild/build/ninja_backend.py tests/unit/test_ninja_backend.py. Repository-wide, patched and unpatched trees both report the same 4 pre-existing errors, so this PR adds none.

On the linked-issue check — answering the question in the thread

Your reading is right, and the registry check is stricter than it looks. The job log says exactly what it wants:

policy error: Pull request embeddedos-org/ebuild#133 must close at least one same-repository issue; no closing issues were recognized. Use Fixes #123, Closes #123, or Resolves #123 in the pull request body

So it needs a closing keyword against a same-repo issue — a bare #NN reference or a Related to line will not satisfy it, and ## Related Issues: N/A certainly will not.

Fixes #136 would be wrong for the reason you gave: #136 is "Incremental static-library builds retain removed source objects", which is the ar rcs behaviour, and #137 is the PR that closes it. Linking #133 to it would auto-close #136 on merge while the archive defect is still live on master.

I listed the six open issues on embeddedos-org/ebuild (#136, #134, #128, #87, #83, #81) and none of them describes this defect — same-basename sources with different extensions colliding at one object path. There is nothing correct to link. File a new issue for it (the changelog entry you just wrote is most of the text) and put Fixes #NN under ## Related Issues. That is the whole remaining distance to green; mergeable is MERGEABLE and I confirmed the diff applies cleanly to origin/master, so there is no conflict behind the BLOCKED state — only this check and REVIEW_REQUIRED.

Worth flagging to the maintainer rather than to you: #125 is failing the identical check for the identical reason. The policy landed on master after both branches were cut, so every in-flight PR that predates it is now red through no fault of its author.

Architecture conformance

Conforms; re-checked against the mirror rather than recalled.

§21 places ebuild in Tier 1 — Foundation, and object-path layout for the ninja backend is eBuild's own concern. §5.1 is not engaged: the diff adds no import, no link edge and no manifest entry, so nothing points up a tier. §9.1 makes eBuild the owner of Configure/Build; generating the build manifest is that responsibility, not a leak of it.

Brief item 8 (API/wire compatibility) is the one worth naming, and the position has improved since the last review: the on-disk layout under _build/obj/ is not a declared compatibility contract in §23.2 — that table covers the eBuild project format, not the build directory — so this is not a contract break. It is still an on-disk format change with a migration consequence, and unlike last time the PR now states that consequence in the changelog. Brief item 4 (weakened checks): none; no test disabled, no lint loosened, no assertion removed. Brief item 9 (performance): none; _object_path is one string concatenation per source at configure time.

Proposed changes

1. File an issue for the object-path collision and add Fixes #NN to the PR body. Nothing in the tree changes.
2. Optional, one line: add the one-time-clean sentence to ## Additional Notes so a reviewer reading only the PR sees what the changelog says.

The code is finished. Sequencing with #137 is a maintainer call, not a blocker on this PR — landing #133 first is safe for anyone who cleans their build directory, and #137 makes the clean unnecessary.

Not checked

• The commits between 5cc570de and d1db5e74 — NOT INSPECTED as commits. The local clone does not contain object d1db5e74, and the ebuild working tree is dirty (TASKS.md, ebuild/cli/integration.py, tests/ebuild/test_integration_initramfs_security.py), so per the rules of engagement I did not fetch into it. I compared end states instead: sha256 of ebuild/build/ninja_backend.py (beb202c1…) and tests/unit/test_ninja_backend.py (1293954d…) are identical between a git archive of 5cc570de and origin/master + this bundle's diff.patch. Commit messages and any intermediate commit are unreviewed.
• mypy — NOT RUN. Not installed and not resolvable on this host.
• Windows and macOS — NOT RUN. The _exe_suffix/.dll/.dylib branches were not exercised; all results above are Linux/CPython 3.12.14.
• The ar duplicate-member mechanism — NOT RE-RUN at this head. It was verified in the previous review with GNU ar on Linux; I did not repeat it, and llvm-ar and MSVC lib.exe remain untested.
• A real incremental rebuild across the rename — NOT RUN as an end-to-end ebuild build, at either head.
• CI beyond the policy job — NOTHING TO CHECK. checks.txt reports exactly one check run on this head, the failing policy job. No build, test or lint lane ran on d1db5e74 at all, so every result above is local, not CI evidence.
• I did not sweep templates/ or examples/ for hard-coded old-style object paths.

Environment for the local runs: uv venv on CPython 3.12.14 with pytest 9.1.1, ebuild installed -e from a git archive export of origin/master with this bundle's diff.patch applied (git apply --check clean). The local ebuild clone was read with git archive/git show only and left untouched.

---

Automated architecture review of d1db5e7402e6 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.