Skip to content

fix(ebuild): recreate static archives on rebuild (stub-tested) - #138

Open
deveshmaurya1996 wants to merge 4 commits into
embeddedos-org:masterfrom
deveshmaurya1996:fix/incremental-static-library
Open

deveshmaurya1996 wants to merge 4 commits into
embeddedos-org:masterfrom
deveshmaurya1996:fix/incremental-static-library

Conversation

@deveshmaurya1996

Copy link
Copy Markdown

Summary

  • Problem: Removing a source from a static_library target and doing an incremental rebuild left the old .o inside the .a. Apps could still link to deleted symbols while a clean build correctly failed (issue Incremental static-library builds retain removed source objects #136).
  • Root cause: Generated ar_rule was ar rcs $out $in. ar r replaces/adds named members but keeps omitted ones.
  • Fix: ar_rule now runs python -m ebuild.build.recreate_archive $out $ar rcs $out $in, which deletes $out then archives the current $in. A small -m helper avoids fragile -c quoting and reports a clear error if the archiver is missing.
  • Tests: Stub cc/ar regression (same launcher pattern as test_package_efw.py) so it runs on Windows without a host compiler. Confirmed the stub test fails under the old rule (removed.o retained) and passes with the fix.

Type of Change

  • fix - Bug fix
  • test - Add tests
  • build - Build-system change

Testing

  • python -m pytest tests/unit/test_ninja_backend.py::TestStaticArchiveRecreation -v ? 3 passed
  • Old-rule reproduction with the same stubs ? BUG_REPRODUCED (removed.o retained)
  • Focused backend tests: tests/unit/test_ninja_backend.py + tests/ebuild/test_ninja_backend.py ? 22 passed, 1 skipped (header rebuild skipped: no host cc)
  • Full suite: 671 passed, 9 failed, 6 skipped
  • The 9 failures are existing PackageRecipe.to_dict gaps in tests/unit/test_index_sync.py (unchanged by this PR)
  • ruff check on touched Python files: clean
  • git diff --check: clean

Pre-Submission Checklist

  • Regression tests added and run
  • Testing documentation updated
  • Based on current master
  • All existing tests pass - baseline failures documented above
  • DCO-signed conventional commit created
  • Linked issue added

Related Issue

Fixes #136

Notes

Open PR #137 addresses the same issue with a real-compiler regression (skipped without cc/ar). This PR instead uses stub tools so the behavioral check runs on Windows CI and developer machines without a host toolchain, and uses a -m helper for clearer missing-archiver diagnostics.

ar rcs updates an existing archive and keeps members that are no longer
inputs, so removing a source from a static_library could leave its .o
linkable after an incremental rebuild. Recreate the archive when the
Ninja edge runs, and cover it with a stub-tool regression that needs no
host compiler.

Fixes embeddedos-org#136

Signed-off-by: Devesh Maurya <deveshmaurya1996@gmail.com>

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — ebuild#138 "fix(ebuild): recreate static archives to drop removed objects"

head: 7bb7826 author: deveshmaurya1996 ci: pending (test matrix never ran — action_required)

Verdict: The diagnosis of issue #136 is correct and the regression test is well built, but the chosen -m invocation makes every static_library build depend on ebuild being importable by the recorded interpreter at build time. I reproduced a hard build failure from that, so the fix as written is not safe to merge in this form.

Findings

# Severity File:line Finding Recommended fix
1 High ebuild/build/ninja_backend.py:220 The generated ar_rule runs <python> -m ebuild.build.recreate_archive …. Ninja executes it with the user's project directory as cwd, so the module is only found when ebuild is installed into that interpreter's environment. When ebuild is on sys.path via cwd/PYTHONPATH instead — which this repo explicitly supports (pytest.ini: pythonpath = ., "Makes import ebuild work without pip install / editable mode") — every static-library link fails. Verified: with deps installed but ebuild not installed, tests/unit/test_ninja_backend.py fails with ninja: build stopped: subcommand failed / ModuleNotFoundError: No module named 'ebuild' (1 failed, 18 passed). After pip install -e . the same file is 19 passed. Before this PR ar_rule had no Python dependency at all, so this is a new failure mode. Invoke the helper by path, not by module name, so resolution cannot depend on cwd or sys.path: build the command from Path(__file__).with_name("recreate_archive.py") and pass it as a positional script argument (f" command = {python_command} {shlex.quote(str(helper))} $out $ar rcs $out $in", with the same $$$ escaping). This works identically for a wheel install, an editable install and a bare source checkout.
2 Medium tests/unit/test_ninja_backend.py:490 (test_removing_source_drops_archive_member_with_stub_tools) The behavioural test passes only because the environment running it has ebuild importable. It therefore cannot catch finding 1 — the exact failure mode the new rule introduces. Add a case that runs the generated ar_rule with an environment where ebuild is not importable (e.g. env with PYTHONPATH cleared and cwd outside the repo), asserting the archive step still succeeds. With the path-based invocation of finding 1 this passes; with -m it fails.
3 Medium ebuild/build/ninja_backend.py:194-198, TESTING.md build.ninja now embeds an absolute host interpreter path, so the generated manifest is no longer relocatable or reproducible across machines/containers. Master design §9.2 requires "reproducible lockfiles/manifests for production builds". TESTING.md records only "regenerate the manifest if the environment's Python moves"; it does not state the stronger constraint that the interpreter must also be able to import ebuild, and no user-facing doc (README.md) mentions that building a static_library now requires a working Python at build time. Document the build-time Python requirement where users will see it, and state the failure mode explicitly. Finding 1's path-based invocation removes the import half of the problem but not the embedded-path half.
4 Low ebuild/build/recreate_archive.py:119-124 Path(args[0]).unlink(missing_ok=True) runs before the archiver is known to exist, so a missing or broken $ar destroys a previously good .a and then reports failure. test_missing_archiver_is_a_clear_error asserts not archive.exists(), which locks that behaviour in as intended. Resolve the archiver first (shutil.which(args[1]) when it is not an existing path) and return the diagnostic before unlinking; only delete once the archiver is known to be runnable. Update the test to assert the stale archive survives a missing archiver.
5 Low ebuild/build/recreate_archive.py:119 unlink(missing_ok=True) still raises on other OSErrors (permission denied, $out is a directory), which surfaces as an uncaught traceback — the thing the module's docstring says it exists to avoid. Wrap the unlink in the same except OSError handler already used for the archiver call.
6 Low TASKS.md:112-116 The appended "Baseline findings during static-archive validation" section records one PR's local test counts in a shared task file. The nine failures it describes (PackageRecipe.to_dict) already have two dedicated open PRs — #119 and #124 — so this note duplicates tracked work and goes stale the moment either merges. Drop the TASKS.md hunk and reference #119/#124 in the PR body instead. The TESTING.md hunk is the part worth keeping.

Architecture conformance

Conforms. eBuild is Tier 1 – Foundation (§21) and recreate_archive.py lands in ebuild/build/, the owning repo and tier; it imports stdlib only, so no #include/import/link points up a tier and §5.1 is not violated. The helper runs on the build host only and never enters a device image, so "eBuild … is not a runtime dependency" (§5.1) still holds for the target.

The relevant rule is §9.2, "Reproducible lockfiles/manifests for production builds". This change makes the generated build.ninja dependent on both the path and the installed package set of the generating interpreter, so the manifest is no longer a self-contained artifact that ninja alone can execute. Finding 1 is that dependency becoming a build failure; finding 3 is the reproducibility half. The master design does not state whether a generated build manifest must be executable independently of the generator — a proposal covering that gap has been appended to .ai/autoreview/proposals/2026-09.md.

No duplication finding for the escaping: python_command correctly uses shell quoting plus $$$ rather than reusing escape_ninja_path, because a rule command value is shell-parsed while a build-statement path is not.

Proposed changes

Smallest sequence that keeps the fix and removes the failure mode:

  1. In _write_ninja, resolve the helper next to the module and pass it positionally:

    helper = Path(__file__).with_name("recreate_archive.py")
    quoted = (
        subprocess.list2cmdline([sys.executable, str(helper)])
        if sys.platform == "win32"
        else f"{shlex.quote(sys.executable)} {shlex.quote(str(helper))}"
    ).replace("$", "$$")
    ...
    f"  command = {quoted} $out $ar rcs $out $in",

    recreate_archive.py already imports nothing from ebuild, so running it as a script needs no other change.

  2. Update the first test's assertion from "ebuild.build.recreate_archive" in ar_rule to match recreate_archive.py, and add the no-ebuild-on-path case from finding 2.

  3. Apply findings 4 and 5 to main().

  4. Drop the TASKS.md hunk (finding 6).

Note for maintainers, not a change request: #137 fixes the same issue (#136) with an inline python -c, which has no import-path dependency and therefore does not exhibit finding 1. This PR's stated advantage over #137 is avoiding "fragile -c quoting"; the path-based invocation above keeps that advantage without the regression. No fix PR was opened from this review — the correction belongs inside this PR, and a third PR against #136 would duplicate #137.

Verification actually run

Against a scratch clone at 7bb7826, Linux, CPython 3.13 (uv venv):

Check Result
pytest tests/unit/test_ninja_backend.py — deps installed, ebuild not installed FAIL — 1 failed, 18 passed (ModuleNotFoundError: No module named 'ebuild' from the AR edge)
pytest tests/unit/test_ninja_backend.py — after pip install -e . PASS — 19 passed
pytest tests/ (full suite, ebuild installed) 9 failed, 676 passed, 1 skipped — all 9 are the pre-existing PackageRecipe.to_dict gap in tests/unit/test_index_sync.py, unrelated to this PR. The PR's baseline claim is substantively correct; counts differ from the quoted Windows numbers (671/9/6) by platform.
ruff check ebuild/build/recreate_archive.py ebuild/build/ninja_backend.py tests/unit/test_ninja_backend.py PASS — "All checks passed!"
Packaging: python -m ebuild.build.recreate_archive from an unrelated cwd, installed PASS — clean usage error, exit 2

CI state

CI — ebuild has not run on this head: run 34825622485 is completed / action_required, i.e. queued behind maintainer approval. The only green check is policy / Policy / Linked Issue. Every result in the PR body is author-local and unconfirmed by CI, and mergeStateStatus is BLOCKED. The same action_required state applies to PRs #133, #125, #124 and #137, so this is a repo-wide gating condition, not something this author can clear. A maintainer approving the workflow run is what unblocks it.

Not checked

  • Windows behaviour. Both the subprocess.list2cmdline quoting path and the .bat stub launcher are Windows-only and were not executed; this run was Linux. The author's Windows numbers are taken as reported, not verified. cmd.exe's handling of a leading quoted interpreter path inside a Ninja rule is untested here.
  • macOS behaviour. Not executed.
  • Real ar/cc. The behavioural test uses Python stubs by design; no host toolchain path was exercised, so the fix was not confirmed against GNU ar or LLVM ar semantics — only against the stub that models ar r.
  • Old-rule reproduction. The PR's BUG_REPRODUCED claim was not independently re-run; I verified the new rule's behaviour, not the old rule's.
  • mypy. Not run (CI marks it continue-on-error anyway).
  • Issue #136 text. Read only through the PR body's summary.

Automated architecture review of 7bb7826401b6 — 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.

@deveshmaurya1996 deveshmaurya1996 changed the title fix(ebuild): recreate static archives to drop removed objects fix(ebuild): recreate static archives on rebuild (stub-tested) Sep 14, 2026
Address review on embeddedos-org#138: path-based recreate_archive.py avoids requiring
importable ebuild at build time, refuse to unlink when ar is missing,
and cover the no-import failure mode in tests.

Signed-off-by: Devesh Maurya <deveshmaurya1996@gmail.com>
@deveshmaurya1996

Copy link
Copy Markdown
Author

Thanks for the review - agreed on finding 1. Pushed b1a7972 with the requested sequence:

  1. High / path invocation - ar_rule now runs python <absolute>/recreate_archive.py . (quoted + $ escaped), not -m ebuild.build.recreate_archive.
  2. Medium / regression for finding 1 - test_archive_rule_works_without_ebuild_on_path asserts -m fails under python -S (no site-packages / no importable ebuild) while the path form succeeds.
  3. Medium / docs - TESTING.md states the build-time Python requirement and that the helper is path-based so PYTHONPATH-only checkouts still work. Dropped the TASKS.md baseline hunk (tracked by fix(packages): implement PackageRecipe.to_dict() to resolve index syn… #119/fix(packages): add PackageRecipe serialization #124).
  4. Low / preserve archive - resolve/check the archiver before unlink; missing ar leaves the existing .a intact.
  5. Low / unlink errors - unlink is wrapped in the same OSError handler path.

Focused check: TestStaticArchiveRecreation ? 4 passed; ninja backend suites ? 27 passed, 1 skipped.

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — ebuild#138 "fix(ebuild): recreate static archives on rebuild (stub-tested)"

head: b1a7972 author: deveshmaurya1996 ci: pending (test matrix still action_required)

Verdict: Follow-up review. b1a7972 applies the requested path-based invocation and the High finding is resolved — verified by reproducing the old failure and the new pass in one environment. Four of the other five findings are resolved; two residuals remain, both narrow and neither blocking.

Status of the previous review's findings (7bb7826)

# Prior severity Status Evidence
1 High — -m ebuild.build.recreate_archive breaks builds when ebuild is not installed Resolved in b1a7972 ninja_backend.py:194-198 now builds the command from Path(__file__).with_name("recreate_archive.py"). Verified below: same venv, ebuild not installed — 7bb7826 gives 1 failed / 18 passed with ModuleNotFoundError: No module named 'ebuild'; b1a7972 gives 20 passed.
2 Medium — regression test could not catch finding 1 Resolved in b1a7972 tests/unit/test_ninja_backend.py:321 test_archive_rule_works_without_ebuild_on_path runs both forms under python -S with PYTHONPATH cleared and cwd outside the repo; asserts the -m form fails and the path form succeeds. It ran (not skipped) in my run.
3 Medium — build-time Python requirement undocumented; manifest not relocatable Partially addressed — open TESTING.md:20-25 now states the build-time interpreter requirement and the PYTHONPATH case. The reproducibility half is untouched and slightly widened; see finding A.
4 Low — unlink before the archiver is known to exist destroys a good .a Partially addressed — open recreate_archive.py:116-118 now returns before unlinking when the archiver is absent, and test_missing_archiver_preserves_existing_archive locks that in. The present-but-not-executable case still destroys the archive; see finding B.
5 Low — unlink raises on other OSErrors Resolved in b1a7972 recreate_archive.py:120-124 wraps the unlink in the same OSError handler.
6 Low — TASKS.md baseline hunk duplicates #119/#124 Resolved in b1a7972 Hunk dropped; files.txt no longer lists TASKS.md. #119 and #124 are both still open, so deferring to them was correct.

Findings

# Severity File:line Finding Recommended fix
A Medium ebuild/build/ninja_backend.py:194-198, TESTING.md:22-24 The generated ar_rule now embeds two absolute host paths — the interpreter and the helper inside the generating ebuild installation. Actual generated rule: command = /…/.venv/bin/python /…/ebuild/build/recreate_archive.py $out $ar rcs $out $in. At 7bb7826 only the interpreter was embedded and the module resolved dynamically, so this commit makes build.ninja pinned to a specific ebuild install location as well. TESTING.md tells the user to regenerate "if the environment's Python moves" — it does not say "or if ebuild moves", which is now the more likely trigger (pip install -U into a new versioned path, a rebuilt venv, tox/nox, a container mounting the source elsewhere). The failure is a late, confusing ninja error rather than a detected stale manifest. This is the unresolved half of prior finding 3 against master design §9.2 "Reproducible lockfiles/manifests for production builds". Two lines, no redesign: (1) extend the TESTING.md sentence to "Regenerate build.ninja if the environment's Python or the ebuild installation moves." (2) README.md states only "Requires Python 3.8+" (line 61) as an install requirement — add that building a static_library target now invokes Python at build time for every archive edge. The stale-manifest detection asked for in the proposal below is follow-up work, not this PR's job.
B Low ebuild/build/recreate_archive.py:116 if not Path(archiver).exists() and shutil.which(archiver) is None treats exists as runnable. Path(archiver).exists() short-circuits the shutil.which executability check, so an archiver that is present but not executable — or a directory where ar is expected — passes the guard, the archive is unlinked, then subprocess.call raises PermissionError and returns 1 with the good archive already gone. Verified directly: main([archive, <mode-0644 file>, "rcs", archive]) → exit 1, archive.exists() is False, previous contents lost. Same result with a directory as $ar. The new test only covers the not-found path, so this half is unguarded. Check runnability, not presence: resolved = args[1] if (os.path.isfile(args[1]) and os.access(args[1], os.X_OK)) else shutil.which(args[1]), and return the diagnostic when resolved is None. Extend test_missing_archiver_preserves_existing_archive with a chmod 0o644 archiver asserting the stale archive survives.

Architecture conformance

Conforms, and more cleanly than at 7bb7826. eBuild is Tier 1 – Foundation (§21); recreate_archive.py sits in ebuild/build/ — the owning repo and tier — and imports only shutil, subprocess, sys, pathlib, so nothing points up a tier and §5.1 is not violated. The helper runs on the build host and never enters a device image, so "eBuild … is not a runtime dependency" holds for the target.

The path-based invocation is what §5.1's spirit wants at build time too: the manifest no longer depends on the generator's installed package set, only on two filesystem paths. The residual §9.2 reproducibility gap (finding A) is a design gap, not an author error — the master design still gives no rule this PR could have been measured against. That gap is already recorded in .ai/autoreview/proposals/2026-09.md under "2026-09-14 — A generated build manifest must be executable without the generator", filed from the previous review of this same PR. That proposal already prescribes Path(__file__).with_name(...) (exactly what b1a7972 implements) and already requires that "any host-specific absolute path a generated manifest embeds is recorded in the build directory's metadata so a stale manifest is detected and regenerated rather than failing at the point of use", which covers finding A. No new proposal was appended — a duplicate would add noise, not information.

Verification actually run

Scratch clone of the PR head at b1a7972, Linux, CPython 3.12.14 (uv venv). The user's ebuild checkout is dirty and was not touched.

Check Result
pytest tests/unit/test_ninja_backend.py — deps present, ebuild not pip-installed PASS — 20 passed. This is the exact configuration that failed before.
Same file, same venv, at old head 7bb7826 (contrast) FAIL — 1 failed / 18 passed, ModuleNotFoundError: No module named 'ebuild' from the AR edge. Confirms the environment genuinely reproduces the defect, so the pass above is meaningful and not a vacuous green.
pytest tests/ full suite, after uv pip install -e . 9 failed / 677 passed / 1 skipped — all 9 are the pre-existing PackageRecipe.to_dict gap in tests/unit/test_index_sync.py, tracked by open PRs #119 and #124. Unrelated to this PR; the author's baseline claim is correct. (Prior review saw 676 passed; the +1 is this PR's new test.)
ruff check on the three changed files PASS — "All checks passed!"
Generated ar_rule inspected from a real NinjaBackend.generate() Emits the path form; no -m anywhere. Evidence for finding A.
recreate_archive.main with a non-executable archiver Archive destroyed — evidence for finding B.

The author's comment reports "TestStaticArchiveRecreation → 4 passed; ninja backend suites → 27 passed, 1 skipped" without command output. I did not reproduce the 27/1 figure — tests/unit/test_ninja_backend.py is 20 tests, 20 passed, 0 skipped here; the larger count is presumably a wider -k selection. The substance of the claim checks out, so this is noted, not raised as a finding.

CI state

Still blocked, and not by anything the author can fix. On b1a7972, CI — ebuild (run 34829365837), CodeQL (34829365756) and Simulation Test (34829365779) are all completed / action_required — queued behind maintainer approval, exactly as at 7bb7826. The only green check is policy / Policy / Linked Issue. mergeStateStatus is BLOCKED; mergeable is MERGEABLE, so there is no merge conflict — the block is purely the unapproved workflow runs. A maintainer approving the workflow run is what unblocks this PR. Every result above is mine or the author's; none is confirmed by org CI.

Proposed changes

Smallest sequence, in order:

  1. Finding B — swap the existence check for a runnability check in recreate_archive.py:116 and extend the preservation test with a non-executable archiver case.
  2. Finding A — one clause added to the TESTING.md regeneration sentence, and one line in README.md about the build-time Python requirement for static_library targets.

Neither is a blocker. No fix PR was opened from this review: no finding here is rated High, and the brief permits fix PRs only for High findings that are small and provable.

Maintainer note, not a change request: #137 is still open and fixes the same issue (#136) with an inline python -c, which never had finding 1. Two open PRs against one issue is now the largest remaining risk on this work — whichever is preferred, the other should be closed rather than both being carried. #138 is the more thoroughly tested of the two.

Not checked

  • Windows. subprocess.list2cmdline quoting and the .bat stub launcher are Windows-only and were not executed; this run was Linux. cmd.exe's handling of two quoted absolute paths at the head of a Ninja rule command is untested here. The author's Windows numbers are taken as reported, not verified.
  • macOS. Not executed.
  • Real ar/cc. The behavioural test uses Python stubs by design. The fix was not confirmed against GNU ar or LLVM ar semantics — only against a stub modelling ar r update behaviour.
  • Zipapp / frozen installs. Path(__file__).with_name(...) assumes the helper exists on disk next to the module. Not exercised; ebuild ships as a normal package today, so this is a note, not a finding.
  • Paths containing spaces in $out/$ar. Unquoted in the rule, pre-existing from the original $ar rcs $out $in and not a regression; not exercised.
  • mypy. Not run (CI marks it continue-on-error).
  • Issue #136 text. Read only through the PR body and the prior review.
  • Org CI. Has not run on this head; nothing here is CI-confirmed.

Automated architecture review of b1a7972a2c75 — 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.

Require an executable archiver (not merely Path.exists) before deleting
the archive, document regenerating build.ninja when ebuild moves, and
note the build-time Python requirement for static_library targets.

Signed-off-by: Devesh Maurya <deveshmaurya1996@gmail.com>
@deveshmaurya1996

Copy link
Copy Markdown
Author

Thanks - addressed the two residuals in b5878a8:

  • Finding B: _resolve_archiver now requires a runnable file (isfile + X_OK) or shutil.which; directory / non-executable cases fail before unlink. Added test_directory_archiver_preserves_existing_archive and test_non_executable_archiver_preserves_existing_archive (latter skips on Windows where X_OK is vacuous for readable files).
  • Finding A: TESTING.md now says regenerate if Python or the ebuild installation moves; README.md notes that static_library edges invoke Python at build time.

TestStaticArchiveRecreation ? 5 passed, 1 skipped (Windows non-exec case).

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — ebuild#138 "fix(ebuild): recreate static archives on rebuild (stub-tested)"

head: b5878a8 author: deveshmaurya1996 ci: pending (CI — ebuild, CodeQL, Simulation Test all still action_required)

Verdict: Third look. b5878a8 closes both residuals from the b1a7972 review — finding A (docs) fully, finding B (destroying a good archive when $ar is present but not runnable) for the two cases it tests. The guard it added still has one hole I reproduced: a bare archiver name that resolves as a file relative to the build cwd passes the check, the archive is unlinked, and subprocess.call then fails with ENOENT — the same data-loss shape, one resolution path over. Narrow, and not a blocker.

Status of the previous review's findings (b1a7972)

# Prior severity Status Evidence
A Medium — build-time Python requirement undocumented; build.ninja pinned to the ebuild install location Resolved in b5878a8 TESTING.md:83-84 now reads "Regenerate build.ninja if the environment's Python or the ebuild installation moves." README.md:63-65 adds the user-facing sentence that a static_library target invokes that interpreter at build time. Both halves of the prior recommendation are present. The stale-manifest detection half remains follow-up work, as the prior review said, and is already carried by the proposal of 2026-09-14 in .ai/autoreview/proposals/2026-09.md.
B Low — Path(archiver).exists() short-circuits the executability check, so a present-but-not-executable $ar destroys a good .a Addressed for the tested cases; one path still open recreate_archive.py:30-34 _resolve_archiver now requires os.path.isfile(...) and os.access(..., X_OK) before falling back to shutil.which. Verified directly: a chmod 0644 archiver and a directory-as-$ar both return 1 with the archive intact (GOOD preserved in both). test_directory_archiver_preserves_existing_archive and test_non_executable_archiver_preserves_existing_archive lock those in and both ran, not skipped, on this host. The remaining path is finding C below.

Nothing from the earlier 7bb7826 review reopened.

Findings

# Severity File:line Finding Recommended fix
C Low ebuild/build/recreate_archive.py:30-34, 49, 60 _resolve_archiver computes a runnable path and then throws it away — line 49 only tests it against None, and line 60 re-invokes the original args[1:]. The two resolutions do not agree for a bare name that exists in the process cwd: os.path.isfile("ar") is cwd-relative and true, so the guard passes, archive.unlink() runs at line 54, and subprocess.call(["ar", …]) then raises ENOENT because POSIX execvp does not search cwd. The good archive is already gone. Reproduced on this head: executable ar in cwd, PATH=/nonexistent-direbuild: cannot run archiver ar: [Errno 2] No such file or directory: 'ar', exit 1, lib.a destroyed. This is the same failure finding B set out to close, reached through the isfile branch rather than the exists branch. Return an absolute path and use it. return os.path.abspath(archiver) in the isfile branch, then resolved = _resolve_archiver(archiver) at line 49 and subprocess.call([resolved, *args[2:]]) at line 60. Verified this closes it: with the same cwd/PATH setup the resolved form runs the archiver and exits 0 with the archive intact. Add the cwd-only-archiver case to TestStaticArchiveRecreation beside the two new preservation tests.
D Low ebuild/build/recreate_archive.py:50 The diagnostic is cannot run archiver {archiver}: not found for an archiver that is present and merely not executable. Observed: ebuild: cannot run archiver /tmp/…/ar-noexec: not found against a file that exists at that exact path. Per .ai/tooling.md ("Error text says what happened and what the developer can do next") this sends the developer looking for a missing binary instead of at a permission bit — and it is the message the new code path emits most often. Distinguish the two states now that _resolve_archiver can tell them apart: when os.path.exists(archiver) is true, say present but not executable; otherwise keep not found.

Neither is a blocker, and neither is new regression from b5878a8 in the sense of working code broken — C is a pre-existing hole that the new guard narrowed but did not close, D is a wording consequence of the new guard.

Architecture conformance

Conforms, unchanged from the b1a7972 assessment. eBuild is Tier 1 — Foundation (§21); recreate_archive.py sits in ebuild/build/ — the owning repo and tier — and imports only os, shutil, subprocess, sys, pathlib. Nothing points up a tier, so §5.1 is not violated. The helper runs on the build host and never enters a device image, so "eBuild understands the complete graph but is not a runtime dependency" (§5.1) holds for the target.

The §9.2 reproducibility gap ("Reproducible lockfiles/manifests for production builds") is now documented rather than closed — which is the correct outcome for this PR, because the design still states no rule about whether a generated manifest must be executable independently of its generator. That gap is already filed as "2026-09-14 — A generated build manifest must be executable without the generator" in .ai/autoreview/proposals/2026-09.md, from the first review of this same PR. No new proposal appended for this head — a duplicate would add noise, not information.

Verification actually run

Scratch clone at b5878a8, Linux, CPython 3.12.14 (uv venv). The user's ebuild checkout is dirty (TASKS.md, ebuild/cli/integration.py, tests/ebuild/test_integration_initramfs_security.py modified, untracked smart-sensor/) and was not touched; only a read-only git fetch of the PR head ref was performed against it.

Check Result
pytest tests/unit/test_ninja_backend.pyebuild not pip-installed, ninja installed PASS — 22 passed, 0 skipped
Same file with ninja absent 19 passed, 3 skipped — the 3 skips are ninja package not installed (lines 194, 314, 485), not the Windows guard
pytest -k TestStaticArchiveRecreation 5 passed, 1 skipped; both new preservation tests ran and passed
pytest tests/ full suite, after uv pip install -e . 9 failed / 679 passed / 1 skipped — all 9 are the pre-existing PackageRecipe.to_dict gap in tests/unit/test_index_sync.py, tracked by open PRs #119 and #124. Prior review saw 677 passed; +2 is this head's two new tests. No regression.
ruff check on the three changed files PASS — "All checks passed!"
main([archive, <mode-0644 file>, "rcs", archive]) exit 1, archive preserved — prior finding B closed for this case
main([archive, <directory>, "rcs", archive]) exit 1, archive preserved
Executable ar in cwd, PATH=/nonexistent-dir exit 1, archive destroyed — evidence for finding C
Same, with os.path.abspath resolution applied exit 0, archive intact — confirms finding C's fix

Correction to the author's comment: it reports "TestStaticArchiveRecreation → 5 passed, 1 skipped (Windows non-exec case)". The counts match here, but the skip is not the Windows case — test_non_executable_archiver_preserves_existing_archive passed. The skipped test is test_removing_source_drops_archive_member_with_stub_tools, skipped for ninja package not installed. That matters because that skipped test is the behavioural one — the test that actually proves removed members drop out of the archive. In an environment without the ninja package the suite goes green while never exercising the defect this PR exists to fix. Worth pinning ninja into the test extra so the proof cannot silently opt out; I have not raised it as a finding because the skip guard predates this PR.

CI state

Blocked, and still not by anything the author can fix. On b5878a8: CI — ebuild (run 34935350458), CodeQL (34935350574) and Simulation Test (34935350451) are all completed / action_required — queued behind maintainer approval, the same state as at 7bb7826 and b1a7972. The only green check is policy / Policy / Linked Issue (34935349606). mergeStateStatus is BLOCKED, mergeable is MERGEABLE — no merge conflict. A maintainer approving the workflow run is what unblocks this PR. Every result in this report is mine or the author's; none is confirmed by org CI, on any of the three heads reviewed.

Proposed changes

  1. Finding C — return os.path.abspath(archiver) in _resolve_archiver's isfile branch; use the returned value at lines 49 and 60; add the cwd-only-archiver test.
  2. Finding D — split the "not found" message from "present but not executable" using the state _resolve_archiver already distinguishes.

No fix PR was opened: neither finding is High, and the brief permits fix PRs only for High findings that are small and provable.

Maintainer note, not a change request: #137 is still open and fixes the same issue (#136), with an inline python -c that never had the original import-path defect. Two open PRs against one issue remains the largest carried risk on this work; #138 is the more thoroughly tested of the two. Picking one and closing the other is a maintainer decision this pipeline will not make.

Not checked

  • Windows. subprocess.list2cmdline quoting, the .bat stub launcher and the os.access(X_OK) skip guard in the new test are Windows-only and were not executed; this run was Linux. The author's Windows numbers are taken as reported, not verified.
  • macOS. Not executed.
  • Real ar/cc. The behavioural test uses Python stubs by design. Not confirmed against GNU ar or LLVM ar semantics — only against a stub modelling ar r update behaviour.
  • Zipapp / frozen installs. Path(__file__).with_name(...) assumes the helper exists on disk next to the module. Not exercised.
  • Paths containing spaces in $out/$ar. Unquoted in the rule; pre-existing, not a regression, not exercised.
  • mypy. NOT RUN (CI marks it continue-on-error).
  • Org CI. Has not run on this head or either previous one. Nothing in this PR's history is CI-confirmed.
  • Issue #136 text. Read only through the PR body and the prior reviews.

Automated architecture review of b5878a8eee8f — 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.

Resolve cwd-relative archivers to an absolute path before invoking them
so the guard and subprocess.call agree, and distinguish not-found from
present-but-not-executable diagnostics.

Signed-off-by: Devesh Maurya <deveshmaurya1996@gmail.com>
@deveshmaurya1996

Copy link
Copy Markdown
Author

Addressed findings C and D in 29e8571:

  • C: _resolve_archiver returns os.path.abspath for cwd-relative executables; invoke uses subprocess.call([resolved, *args[2:]]). Added test_cwd_only_archiver_is_resolved_absolutely.
  • D: diagnostics now say present but not executable vs not found.

TestStaticArchiveRecreation ? 6 passed, 1 skipped (Windows non-exec X_OK).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incremental static-library builds retain removed source objects

2 participants