From 7bb7826401b6b6d200b12090d38c01a2819c74a2 Mon Sep 17 00:00:00 2001 From: Devesh Maurya Date: Mon, 14 Sep 2026 14:28:14 +0530 Subject: [PATCH 1/4] fix(ebuild): recreate static archives to drop removed objects 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 #136 Signed-off-by: Devesh Maurya --- TASKS.md | 7 ++ TESTING.md | 14 +++ ebuild/build/ninja_backend.py | 16 ++- ebuild/build/recreate_archive.py | 44 ++++++++ tests/unit/test_ninja_backend.py | 188 +++++++++++++++++++++++++++++++ 5 files changed, 268 insertions(+), 1 deletion(-) create mode 100644 ebuild/build/recreate_archive.py diff --git a/TASKS.md b/TASKS.md index 09f6bcb9..f281ec98 100644 --- a/TASKS.md +++ b/TASKS.md @@ -109,3 +109,10 @@ These commands were derived from the manifests at the repository root. Confirm o [ORCHESTRATION.md](./ORCHESTRATION.md) is met and the verification commands were actually run. - `blocked` requires a note naming what it is blocked on and who can unblock it. + +## Baseline findings during static-archive validation + +Unchanged commit `76970c9` and the archive fix share the same nine failures: +missing `PackageRecipe.to_dict` in `tests/unit/test_index_sync.py`. Full suite +after this change: **671 passed, 9 failed, 6 skipped** on Windows with Python +3.13. Those nine failures are outside the archive fix. diff --git a/TESTING.md b/TESTING.md index 7039b083..64ddabc1 100644 --- a/TESTING.md +++ b/TESTING.md @@ -66,3 +66,17 @@ passed, failed and skipped. A skipped test is not a passing test. Any test that could not be run in this environment is named, with the reason, and marked `NOT RUN` or `UNKNOWN` per [VERIFY.md](./VERIFY.md). + +## Static-library source removal regression + +Run `python -m pytest tests/unit/test_ninja_backend.py::TestStaticArchiveRecreation -v`. + +The behavioral test drives Ninja with Python stub `cc`/`ar` tools (same launcher +pattern as `tests/unit/test_package_efw.py`), so it does not need a host C +toolchain. The stub archiver keeps omitted members the way real `ar r` does; +after a source is removed, the rebuilt archive must not list that object. + +`ar_rule` recreates the archive when its build step runs: updating an existing +archive with `ar rcs` alone retains members removed from the source list. +`build.ninja` embeds the generating interpreter path for that helper, so +regenerate the manifest if the environment's Python moves. diff --git a/ebuild/build/ninja_backend.py b/ebuild/build/ninja_backend.py index de417bdc..93e86bbc 100644 --- a/ebuild/build/ninja_backend.py +++ b/ebuild/build/ninja_backend.py @@ -9,6 +9,8 @@ from __future__ import annotations import json +import shlex +import subprocess import sys from dataclasses import dataclass, field from pathlib import Path @@ -187,6 +189,13 @@ def _object_path(self, target, src: str) -> Path: def _write_ninja(self) -> None: """Write the build.ninja file.""" ninja_path = self.build_dir / "build.ninja" + # Quote the generating interpreter for the shell Ninja will use, and + # escape `$` so Ninja does not treat path fragments as variables. + python_command = ( + subprocess.list2cmdline([sys.executable]) + if sys.platform == "win32" + else shlex.quote(sys.executable) + ).replace("$", "$$") lines = [ f"# Generated by ebuild", f"cc = {self.toolchain.cc}", @@ -204,7 +213,12 @@ def _write_ninja(self) -> None: " description = LINK $out", "", "rule ar_rule", - " command = $ar rcs $out $in", + # `ar rcs` replaces named members but keeps omitted ones, so an + # incremental rebuild after removing a source can leave its .o in + # the archive. recreate_archive deletes $out first. A -m module + # keeps this portable (no `rm`) and avoids fragile -c quoting. + f" command = {python_command} -m ebuild.build.recreate_archive " + "$out $ar rcs $out $in", " description = AR $out", "", # A shared_library edge names this rule. Without the rule the diff --git a/ebuild/build/recreate_archive.py b/ebuild/build/recreate_archive.py new file mode 100644 index 00000000..bb0cf560 --- /dev/null +++ b/ebuild/build/recreate_archive.py @@ -0,0 +1,44 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 EoS Project + +"""Recreate a static archive for Ninja's ar_rule. + +``ar rcs $out $in`` updates an existing archive: it replaces or adds the +members named in ``$in``, but keeps any other members already present. After a +source is removed from a static_library target, an incremental rebuild can +therefore leave the old ``.o`` inside ``$out``. + +Ninja invokes this module as:: + + python -m ebuild.build.recreate_archive $out $ar rcs $out $in + +so the archive is deleted first and then rebuilt from the current object list. +A Python helper is used instead of ``rm`` so the same rule works on Windows. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + + +def main(argv: list[str] | None = None) -> int: + args = list(sys.argv[1:] if argv is None else argv) + if len(args) < 4: + print( + "ebuild: recreate_archive: expected $out $ar rcs $out …members", + file=sys.stderr, + ) + return 2 + + Path(args[0]).unlink(missing_ok=True) + try: + return subprocess.call(args[1:]) + except OSError as exc: + print(f"ebuild: cannot run archiver {args[1]}: {exc}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/unit/test_ninja_backend.py b/tests/unit/test_ninja_backend.py index c0dd0090..74861e31 100644 --- a/tests/unit/test_ninja_backend.py +++ b/tests/unit/test_ninja_backend.py @@ -360,5 +360,193 @@ def test_escaping_does_not_leak_into_compile_commands(self, tmp_path): assert "$ " not in entry["command"] +def _stub_tool(tmp_path, name: str, script_body: str) -> Path: + """A host tool stand-in driven by sys.executable. + + Same pattern as ``tests/unit/test_package_efw.py``: Windows cannot run a + ``#!/bin/sh`` script, so the launcher is a ``.bat`` there and a shell + wrapper elsewhere. + """ + import os + import stat + + script = tmp_path / f"_{name}_impl.py" + script.write_text(script_body, encoding="utf-8") + if os.name == "nt": + tool = tmp_path / f"{name}.bat" + tool.write_text( + f'@echo off\r\n"{sys.executable}" "{script}" %*\r\n', encoding="utf-8" + ) + else: + tool = tmp_path / name + tool.write_text( + f'#!/bin/sh\nexec "{sys.executable}" "{script}" "$@"\n', encoding="utf-8" + ) + tool.chmod(tool.stat().st_mode | stat.S_IEXEC) + return tool + + +def _stub_cc(tmp_path) -> Path: + """Compile by writing the object and a trivial depfile — no real compiler.""" + return _stub_tool( + tmp_path, + "cc", + "\n".join( + [ + "import pathlib, sys", + "argv = sys.argv[1:]", + "out = dep = src = None", + "i = 0", + "while i < len(argv):", + " if argv[i] == '-o' and i + 1 < len(argv):", + " out = argv[i + 1]; i += 2", + " elif argv[i] == '-MF' and i + 1 < len(argv):", + " dep = argv[i + 1]; i += 2", + " elif argv[i] == '-c' and i + 1 < len(argv):", + " src = argv[i + 1]; i += 2", + " else:", + " i += 1", + "path = pathlib.Path(out)", + "path.parent.mkdir(parents=True, exist_ok=True)", + "path.write_bytes(b'obj:' + pathlib.Path(src).name.encode())", + "if dep:", + " pathlib.Path(dep).write_text(f'{out}: {src}\\n', encoding='utf-8')", + "", + ] + ), + ) + + +def _stub_ar(tmp_path) -> Path: + """Archiver with real ``ar r`` update semantics: omitted members stay. + + That is the defect under test. If the Ninja rule only runs ``ar rcs`` + against an existing archive, removed objects survive. Recreating the + archive first makes this stub retain only the current inputs. + """ + return _stub_tool( + tmp_path, + "ar", + "\n".join( + [ + "import pathlib, sys", + "op = sys.argv[1]", + "archive = pathlib.Path(sys.argv[2])", + "members = sys.argv[3:]", + "if op == 't':", + " sys.stdout.write(archive.read_text(encoding='utf-8') if archive.exists() else '')", + " raise SystemExit(0)", + "if 'r' not in op:", + " raise SystemExit(f'unsupported ar op: {op}')", + "names = {}", + "if archive.exists():", + " for line in archive.read_text(encoding='utf-8').splitlines():", + " if line:", + " names[pathlib.Path(line).name] = pathlib.Path(line).name", + "for member in members:", + " names[pathlib.Path(member).name] = pathlib.Path(member).name", + "archive.parent.mkdir(parents=True, exist_ok=True)", + "archive.write_text(('\\n'.join(names.values()) + '\\n') if names else '', encoding='utf-8')", + "", + ] + ), + ) + + +@pytest.mark.ebuild +class TestStaticArchiveRecreation: + """Removing a static-library source must drop its object from the archive.""" + + def test_ar_rule_recreates_via_helper_module(self, tmp_path): + """The generated rule must delete-then-archive, not only ``ar rcs``.""" + target = TargetConfig( + name="helpers", target_type="static_library", sources=["keep.c"] + ) + config = ProjectConfig( + name="proj", version="1.0", targets=[target], source_dir=tmp_path + ) + build_dir = tmp_path / "build" + NinjaBackend(config, build_dir, _toolchain()).generate() + ninja = (build_dir / "build.ninja").read_text(encoding="utf-8") + + ar_rule = ninja.split("rule ar_rule\n", 1)[1].split("\nrule ", 1)[0] + assert "ebuild.build.recreate_archive" in ar_rule + assert ar_rule.strip().startswith("command =") + # The bare update form is exactly the bug; it must not be the rule body. + assert "command = $ar rcs $out $in" not in ninja + + def test_removing_source_drops_archive_member_with_stub_tools(self, tmp_path): + """Incremental rebuild must not keep a removed object in the archive. + + Uses Python stub ``cc``/``ar`` tools so this runs without a host + toolchain (including on Windows CI runners that have no gcc). + """ + pytest.importorskip("ninja", reason="ninja package not installed") + + source_dir = tmp_path / "project" + source_dir.mkdir() + (source_dir / "keep.c").write_text("int keep(void) { return 1; }\n", encoding="utf-8") + (source_dir / "removed.c").write_text( + "int removed(void) { return 42; }\n", encoding="utf-8" + ) + + cc = _stub_cc(tmp_path) + ar = _stub_ar(tmp_path) + library = TargetConfig( + name="helpers", + target_type="static_library", + sources=["keep.c", "removed.c"], + ) + config = ProjectConfig( + name="archive-regression", + version="1.0", + source_dir=source_dir, + targets=[library], + ) + build_dir = source_dir / "build" + archive = build_dir / "libhelpers.a" + toolchain = SimpleNamespace(cc=str(cc), cxx=str(cc), ar=str(ar)) + + def build(): + NinjaBackend(config, build_dir, toolchain).generate() + result = subprocess.run( + [sys.executable, "-m", "ninja", "-f", str(build_dir / "build.ninja")], + cwd=str(source_dir), + capture_output=True, + text=True, + ) + assert result.returncode == 0, result.stdout + result.stderr + + def members(): + return subprocess.check_output( + [str(ar), "t", str(archive)], text=True + ).splitlines() + + build() + assert set(members()) == {"keep.o", "removed.o"} + + library.sources.remove("removed.c") + (source_dir / "removed.c").unlink() + # Drop the stale object file too: Ninja would not rebuild it, but a + # real ``ar rcs`` update would still leave its member in the archive. + removed_obj = build_dir / "obj" / "helpers" / "removed.o" + if removed_obj.exists(): + removed_obj.unlink() + + build() + assert "removed.o" not in members(), members() + assert "keep.o" in members(), members() + + def test_missing_archiver_is_a_clear_error(self, tmp_path): + """A missing `$ar` must not surface as an uncaught Python traceback.""" + from ebuild.build.recreate_archive import main + + archive = tmp_path / "lib.a" + archive.write_text("stale\n", encoding="utf-8") + code = main([str(archive), str(tmp_path / "no-such-ar"), "rcs", str(archive)]) + assert code == 1 + assert not archive.exists() + + if __name__ == "__main__": unittest.main() From b1a7972a2c7540a74809bd8c33eff3f54a9afd8a Mon Sep 17 00:00:00 2001 From: Devesh Maurya Date: Mon, 14 Sep 2026 15:12:06 +0530 Subject: [PATCH 2/4] fix(ebuild): invoke archive helper by path, not -m Address review on #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 --- TASKS.md | 7 --- TESTING.md | 9 ++- ebuild/build/ninja_backend.py | 21 ++++--- ebuild/build/recreate_archive.py | 23 +++++-- tests/unit/test_ninja_backend.py | 105 +++++++++++++++++++++++++++++-- 5 files changed, 136 insertions(+), 29 deletions(-) diff --git a/TASKS.md b/TASKS.md index f281ec98..09f6bcb9 100644 --- a/TASKS.md +++ b/TASKS.md @@ -109,10 +109,3 @@ These commands were derived from the manifests at the repository root. Confirm o [ORCHESTRATION.md](./ORCHESTRATION.md) is met and the verification commands were actually run. - `blocked` requires a note naming what it is blocked on and who can unblock it. - -## Baseline findings during static-archive validation - -Unchanged commit `76970c9` and the archive fix share the same nine failures: -missing `PackageRecipe.to_dict` in `tests/unit/test_index_sync.py`. Full suite -after this change: **671 passed, 9 failed, 6 skipped** on Windows with Python -3.13. Those nine failures are outside the archive fix. diff --git a/TESTING.md b/TESTING.md index 64ddabc1..53075124 100644 --- a/TESTING.md +++ b/TESTING.md @@ -77,6 +77,9 @@ toolchain. The stub archiver keeps omitted members the way real `ar r` does; after a source is removed, the rebuilt archive must not list that object. `ar_rule` recreates the archive when its build step runs: updating an existing -archive with `ar rcs` alone retains members removed from the source list. -`build.ninja` embeds the generating interpreter path for that helper, so -regenerate the manifest if the environment's Python moves. +archive with `ar rcs` alone retains members removed from the source list. The +rule invokes `recreate_archive.py` by absolute path through the generating +Python interpreter, so Ninja needs that interpreter at build time for every +`static_library` edge. Regenerate `build.ninja` if the environment's Python +moves. The helper is not imported as `ebuild.*`, so a bare checkout that only +puts the package on `PYTHONPATH` still works. diff --git a/ebuild/build/ninja_backend.py b/ebuild/build/ninja_backend.py index 93e86bbc..20fb1b5b 100644 --- a/ebuild/build/ninja_backend.py +++ b/ebuild/build/ninja_backend.py @@ -189,12 +189,15 @@ def _object_path(self, target, src: str) -> Path: def _write_ninja(self) -> None: """Write the build.ninja file.""" ninja_path = self.build_dir / "build.ninja" - # Quote the generating interpreter for the shell Ninja will use, and - # escape `$` so Ninja does not treat path fragments as variables. - python_command = ( - subprocess.list2cmdline([sys.executable]) + # Invoke recreate_archive.py by absolute path so Ninja does not need + # `ebuild` on sys.path (cwd/PYTHONPATH checkouts, not only installs). + # Quote for the shell Ninja will use, and escape `$` so Ninja does not + # treat path fragments as variables. + helper = Path(__file__).with_name("recreate_archive.py") + archive_command = ( + subprocess.list2cmdline([sys.executable, str(helper)]) if sys.platform == "win32" - else shlex.quote(sys.executable) + else f"{shlex.quote(sys.executable)} {shlex.quote(str(helper))}" ).replace("$", "$$") lines = [ f"# Generated by ebuild", @@ -215,10 +218,10 @@ def _write_ninja(self) -> None: "rule ar_rule", # `ar rcs` replaces named members but keeps omitted ones, so an # incremental rebuild after removing a source can leave its .o in - # the archive. recreate_archive deletes $out first. A -m module - # keeps this portable (no `rm`) and avoids fragile -c quoting. - f" command = {python_command} -m ebuild.build.recreate_archive " - "$out $ar rcs $out $in", + # the archive. recreate_archive deletes $out first. A path-based + # helper keeps this portable (no `rm`) without requiring `import + # ebuild` at build time. + f" command = {archive_command} $out $ar rcs $out $in", " description = AR $out", "", # A shared_library edge names this rule. Without the rule the diff --git a/ebuild/build/recreate_archive.py b/ebuild/build/recreate_archive.py index bb0cf560..a4e9e68b 100644 --- a/ebuild/build/recreate_archive.py +++ b/ebuild/build/recreate_archive.py @@ -8,16 +8,19 @@ source is removed from a static_library target, an incremental rebuild can therefore leave the old ``.o`` inside ``$out``. -Ninja invokes this module as:: +Ninja invokes this script by absolute path as:: - python -m ebuild.build.recreate_archive $out $ar rcs $out $in + python /path/to/recreate_archive.py $out $ar rcs $out $in so the archive is deleted first and then rebuilt from the current object list. A Python helper is used instead of ``rm`` so the same rule works on Windows. +The script is run by path (not ``python -m``) so a source checkout that only +puts ebuild on ``PYTHONPATH`` still builds static libraries. """ from __future__ import annotations +import shutil import subprocess import sys from pathlib import Path @@ -32,11 +35,23 @@ def main(argv: list[str] | None = None) -> int: ) return 2 - Path(args[0]).unlink(missing_ok=True) + archive = Path(args[0]) + archiver = args[1] + # Refuse to destroy a good archive when the archiver cannot run. + if not Path(archiver).exists() and shutil.which(archiver) is None: + print(f"ebuild: cannot run archiver {archiver}: not found", file=sys.stderr) + return 1 + + try: + archive.unlink(missing_ok=True) + except OSError as exc: + print(f"ebuild: cannot remove archive {archive}: {exc}", file=sys.stderr) + return 1 + try: return subprocess.call(args[1:]) except OSError as exc: - print(f"ebuild: cannot run archiver {args[1]}: {exc}", file=sys.stderr) + print(f"ebuild: cannot run archiver {archiver}: {exc}", file=sys.stderr) return 1 diff --git a/tests/unit/test_ninja_backend.py b/tests/unit/test_ninja_backend.py index 74861e31..f8088c2d 100644 --- a/tests/unit/test_ninja_backend.py +++ b/tests/unit/test_ninja_backend.py @@ -457,8 +457,8 @@ def _stub_ar(tmp_path) -> Path: class TestStaticArchiveRecreation: """Removing a static-library source must drop its object from the archive.""" - def test_ar_rule_recreates_via_helper_module(self, tmp_path): - """The generated rule must delete-then-archive, not only ``ar rcs``.""" + def test_ar_rule_invokes_helper_by_path(self, tmp_path): + """The generated rule must delete-then-archive via the helper script.""" target = TargetConfig( name="helpers", target_type="static_library", sources=["keep.c"] ) @@ -470,7 +470,8 @@ def test_ar_rule_recreates_via_helper_module(self, tmp_path): ninja = (build_dir / "build.ninja").read_text(encoding="utf-8") ar_rule = ninja.split("rule ar_rule\n", 1)[1].split("\nrule ", 1)[0] - assert "ebuild.build.recreate_archive" in ar_rule + assert "recreate_archive.py" in ar_rule + assert "-m ebuild.build.recreate_archive" not in ar_rule assert ar_rule.strip().startswith("command =") # The bare update form is exactly the bug; it must not be the rule body. assert "command = $ar rcs $out $in" not in ninja @@ -537,15 +538,107 @@ def members(): assert "removed.o" not in members(), members() assert "keep.o" in members(), members() - def test_missing_archiver_is_a_clear_error(self, tmp_path): - """A missing `$ar` must not surface as an uncaught Python traceback.""" + def test_archive_rule_works_without_ebuild_on_path(self, tmp_path): + """The AR helper must not require ``import ebuild`` at build time. + + ``python -m ebuild.build.recreate_archive`` fails when ebuild is only + reachable via cwd/PYTHONPATH (or when site-packages are disabled). + Invoking the helper by absolute path must still succeed. + """ + import os + + source_dir = tmp_path / "project" + source_dir.mkdir() + (source_dir / "lib.c").write_text("int value(void) { return 1; }\n", encoding="utf-8") + + ar = _stub_ar(tmp_path) + config = ProjectConfig( + name="no-import", + version="1.0", + source_dir=source_dir, + targets=[ + TargetConfig( + name="helpers", + target_type="static_library", + sources=["lib.c"], + ) + ], + ) + build_dir = source_dir / "build" + NinjaBackend( + config, build_dir, SimpleNamespace(cc="cc", cxx="c++", ar=str(ar)) + ).generate() + + ninja = (build_dir / "build.ninja").read_text(encoding="utf-8") + ar_rule = ninja.split("rule ar_rule\n", 1)[1].split("\nrule ", 1)[0] + assert "recreate_archive.py" in ar_rule + assert "-m ebuild" not in ar_rule + + from ebuild.build import recreate_archive as recreate_mod + + helper = Path(recreate_mod.__file__).resolve() + archive = build_dir / "libhelpers.a" + obj = build_dir / "obj" / "helpers" / "lib.o" + obj.parent.mkdir(parents=True) + obj.write_bytes(b"obj:lib.c") + + outside = tmp_path / "outside" + outside.mkdir() + env = {k: v for k, v in os.environ.items() if k != "PYTHONPATH"} + env["PYTHONPATH"] = "" + + # -S drops site-packages so an editable install of ebuild is invisible, + # matching a checkout that only puts the package on PYTHONPATH. + module_form = subprocess.run( + [ + sys.executable, + "-S", + "-m", + "ebuild.build.recreate_archive", + str(archive), + str(ar), + "rcs", + str(archive), + str(obj), + ], + cwd=str(outside), + env=env, + capture_output=True, + text=True, + ) + assert module_form.returncode != 0 + assert "No module named 'ebuild'" in module_form.stderr + module_form.stdout + + path_form = subprocess.run( + [ + sys.executable, + "-S", + str(helper), + str(archive), + str(ar), + "rcs", + str(archive), + str(obj), + ], + cwd=str(outside), + env=env, + capture_output=True, + text=True, + ) + assert path_form.returncode == 0, path_form.stdout + path_form.stderr + assert archive.exists() + assert "lib.o" in archive.read_text(encoding="utf-8") + + def test_missing_archiver_preserves_existing_archive(self, tmp_path): + """A missing `$ar` must fail without deleting a previously good archive.""" from ebuild.build.recreate_archive import main archive = tmp_path / "lib.a" archive.write_text("stale\n", encoding="utf-8") code = main([str(archive), str(tmp_path / "no-such-ar"), "rcs", str(archive)]) assert code == 1 - assert not archive.exists() + assert archive.exists() + assert archive.read_text(encoding="utf-8") == "stale\n" if __name__ == "__main__": From b5878a8eee8ffaddbc579a42f105c3fc3839693d Mon Sep 17 00:00:00 2001 From: Devesh Maurya Date: Tue, 15 Sep 2026 11:34:29 +0530 Subject: [PATCH 3/4] fix(ebuild): check archiver is runnable before unlinking 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 --- README.md | 3 +++ TESTING.md | 6 +++--- ebuild/build/recreate_archive.py | 10 +++++++++- tests/unit/test_ninja_backend.py | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 47 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 8b1f5dcb..9150bfb2 100644 --- a/README.md +++ b/README.md @@ -60,6 +60,9 @@ Observed in the source tree: Requires Python 3.8+. +Building a `static_library` target also invokes that Python interpreter at +build time: the generated Ninja `ar_rule` runs a small helper to recreate the +archive so removed object members cannot linger. ```bash pip install -e . # from the repo root # or: diff --git a/TESTING.md b/TESTING.md index 53075124..b95c7f19 100644 --- a/TESTING.md +++ b/TESTING.md @@ -80,6 +80,6 @@ after a source is removed, the rebuilt archive must not list that object. archive with `ar rcs` alone retains members removed from the source list. The rule invokes `recreate_archive.py` by absolute path through the generating Python interpreter, so Ninja needs that interpreter at build time for every -`static_library` edge. Regenerate `build.ninja` if the environment's Python -moves. The helper is not imported as `ebuild.*`, so a bare checkout that only -puts the package on `PYTHONPATH` still works. +`static_library` edge. Regenerate `build.ninja` if the environment's Python or +the ebuild installation moves. The helper is not imported as `ebuild.*`, so a +bare checkout that only puts the package on `PYTHONPATH` still works. diff --git a/ebuild/build/recreate_archive.py b/ebuild/build/recreate_archive.py index a4e9e68b..ec44701c 100644 --- a/ebuild/build/recreate_archive.py +++ b/ebuild/build/recreate_archive.py @@ -20,12 +20,20 @@ from __future__ import annotations +import os import shutil import subprocess import sys from pathlib import Path +def _resolve_archiver(archiver: str) -> str | None: + """Return a runnable archiver path, or None if it cannot be executed.""" + if os.path.isfile(archiver) and os.access(archiver, os.X_OK): + return archiver + return shutil.which(archiver) + + def main(argv: list[str] | None = None) -> int: args = list(sys.argv[1:] if argv is None else argv) if len(args) < 4: @@ -38,7 +46,7 @@ def main(argv: list[str] | None = None) -> int: archive = Path(args[0]) archiver = args[1] # Refuse to destroy a good archive when the archiver cannot run. - if not Path(archiver).exists() and shutil.which(archiver) is None: + if _resolve_archiver(archiver) is None: print(f"ebuild: cannot run archiver {archiver}: not found", file=sys.stderr) return 1 diff --git a/tests/unit/test_ninja_backend.py b/tests/unit/test_ninja_backend.py index f8088c2d..6c5726ef 100644 --- a/tests/unit/test_ninja_backend.py +++ b/tests/unit/test_ninja_backend.py @@ -640,6 +640,38 @@ def test_missing_archiver_preserves_existing_archive(self, tmp_path): assert archive.exists() assert archive.read_text(encoding="utf-8") == "stale\n" + def test_directory_archiver_preserves_existing_archive(self, tmp_path): + """A directory where ``ar`` was expected must not destroy a good archive.""" + from ebuild.build.recreate_archive import main + + archive = tmp_path / "lib.a" + archive.write_text("stale\n", encoding="utf-8") + as_dir = tmp_path / "ar-as-dir" + as_dir.mkdir() + code = main([str(archive), str(as_dir), "rcs", str(archive)]) + assert code == 1 + assert archive.read_text(encoding="utf-8") == "stale\n" + + def test_non_executable_archiver_preserves_existing_archive(self, tmp_path): + """A present but non-executable `$ar` must not destroy a good archive.""" + import os + import stat + + from ebuild.build.recreate_archive import main + + blocked = tmp_path / "ar-not-exec" + blocked.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + blocked.chmod(stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH) + if os.access(blocked, os.X_OK): + # Windows treats readable files as executable. + pytest.skip("os.access(X_OK) is true for readable files on this platform") + + archive = tmp_path / "lib.a" + archive.write_text("stale\n", encoding="utf-8") + code = main([str(archive), str(blocked), "rcs", str(archive)]) + assert code == 1 + assert archive.read_text(encoding="utf-8") == "stale\n" + if __name__ == "__main__": unittest.main() From 29e8571332dd69d70a4e5e2af45fb4083fcae0d0 Mon Sep 17 00:00:00 2001 From: Devesh Maurya Date: Tue, 15 Sep 2026 12:11:12 +0530 Subject: [PATCH 4/4] fix(ebuild): use absolute archiver path and clearer errors 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 --- ebuild/build/recreate_archive.py | 22 +++++++--- tests/unit/test_ninja_backend.py | 74 +++++++++++++++++++++++++++++--- 2 files changed, 86 insertions(+), 10 deletions(-) diff --git a/ebuild/build/recreate_archive.py b/ebuild/build/recreate_archive.py index ec44701c..410a77c5 100644 --- a/ebuild/build/recreate_archive.py +++ b/ebuild/build/recreate_archive.py @@ -28,9 +28,15 @@ def _resolve_archiver(archiver: str) -> str | None: - """Return a runnable archiver path, or None if it cannot be executed.""" + """Return an absolute runnable archiver path, or None. + + A bare name that is executable in the process cwd must become absolute: + ``subprocess.call(["ar", …])`` uses PATH search and does not look in cwd, + so returning the relative name would pass the guard then fail after the + archive was already deleted. + """ if os.path.isfile(archiver) and os.access(archiver, os.X_OK): - return archiver + return os.path.abspath(archiver) return shutil.which(archiver) @@ -46,8 +52,14 @@ def main(argv: list[str] | None = None) -> int: archive = Path(args[0]) archiver = args[1] # Refuse to destroy a good archive when the archiver cannot run. - if _resolve_archiver(archiver) is None: - print(f"ebuild: cannot run archiver {archiver}: not found", file=sys.stderr) + resolved = _resolve_archiver(archiver) + if resolved is None: + reason = ( + "present but not executable" + if os.path.exists(archiver) + else "not found" + ) + print(f"ebuild: cannot run archiver {archiver}: {reason}", file=sys.stderr) return 1 try: @@ -57,7 +69,7 @@ def main(argv: list[str] | None = None) -> int: return 1 try: - return subprocess.call(args[1:]) + return subprocess.call([resolved, *args[2:]]) except OSError as exc: print(f"ebuild: cannot run archiver {archiver}: {exc}", file=sys.stderr) return 1 diff --git a/tests/unit/test_ninja_backend.py b/tests/unit/test_ninja_backend.py index 6c5726ef..3e23955d 100644 --- a/tests/unit/test_ninja_backend.py +++ b/tests/unit/test_ninja_backend.py @@ -587,8 +587,6 @@ def test_archive_rule_works_without_ebuild_on_path(self, tmp_path): env = {k: v for k, v in os.environ.items() if k != "PYTHONPATH"} env["PYTHONPATH"] = "" - # -S drops site-packages so an editable install of ebuild is invisible, - # matching a checkout that only puts the package on PYTHONPATH. module_form = subprocess.run( [ sys.executable, @@ -629,7 +627,7 @@ def test_archive_rule_works_without_ebuild_on_path(self, tmp_path): assert archive.exists() assert "lib.o" in archive.read_text(encoding="utf-8") - def test_missing_archiver_preserves_existing_archive(self, tmp_path): + def test_missing_archiver_preserves_existing_archive(self, tmp_path, capsys): """A missing `$ar` must fail without deleting a previously good archive.""" from ebuild.build.recreate_archive import main @@ -639,8 +637,9 @@ def test_missing_archiver_preserves_existing_archive(self, tmp_path): assert code == 1 assert archive.exists() assert archive.read_text(encoding="utf-8") == "stale\n" + assert "not found" in capsys.readouterr().err - def test_directory_archiver_preserves_existing_archive(self, tmp_path): + def test_directory_archiver_preserves_existing_archive(self, tmp_path, capsys): """A directory where ``ar`` was expected must not destroy a good archive.""" from ebuild.build.recreate_archive import main @@ -651,8 +650,9 @@ def test_directory_archiver_preserves_existing_archive(self, tmp_path): code = main([str(archive), str(as_dir), "rcs", str(archive)]) assert code == 1 assert archive.read_text(encoding="utf-8") == "stale\n" + assert "present but not executable" in capsys.readouterr().err - def test_non_executable_archiver_preserves_existing_archive(self, tmp_path): + def test_non_executable_archiver_preserves_existing_archive(self, tmp_path, capsys): """A present but non-executable `$ar` must not destroy a good archive.""" import os import stat @@ -671,6 +671,70 @@ def test_non_executable_archiver_preserves_existing_archive(self, tmp_path): code = main([str(archive), str(blocked), "rcs", str(archive)]) assert code == 1 assert archive.read_text(encoding="utf-8") == "stale\n" + assert "present but not executable" in capsys.readouterr().err + + def test_cwd_only_archiver_is_resolved_absolutely(self, tmp_path, monkeypatch): + """A bare archiver name that only exists in cwd must still run. + + ``os.path.isfile("ar")`` is cwd-relative, but ``subprocess.call(["ar"])`` + searches PATH and skips cwd. Resolving to an absolute path keeps the + guard and the invoke agreeing so a good archive is not destroyed. + """ + import os + import stat + + from ebuild.build.recreate_archive import main + + ar_impl = tmp_path / "_ar_impl.py" + ar_impl.write_text( + "\n".join( + [ + "import pathlib, sys", + "op = sys.argv[1]", + "archive = pathlib.Path(sys.argv[2])", + "members = sys.argv[3:]", + "if op == 't':", + " sys.stdout.write(", + " archive.read_text(encoding='utf-8') if archive.exists() else '')", + " raise SystemExit(0)", + "names = {pathlib.Path(m).name for m in members}", + "archive.write_text(", + " ('\\n'.join(sorted(names)) + '\\n') if names else '',", + " encoding='utf-8')", + "", + ] + ), + encoding="utf-8", + ) + if os.name == "nt": + archiver_name = "ar.bat" + (tmp_path / archiver_name).write_text( + f'@echo off\r\n"{sys.executable}" "{ar_impl}" %*\r\n', + encoding="utf-8", + ) + else: + archiver_name = "ar" + ar = tmp_path / archiver_name + ar.write_text( + f'#!/bin/sh\nexec "{sys.executable}" "{ar_impl}" "$@"\n', + encoding="utf-8", + ) + ar.chmod(ar.stat().st_mode | stat.S_IEXEC) + + empty_path = tmp_path / "empty-path" + empty_path.mkdir() + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("PATH", str(empty_path)) + + archive = tmp_path / "lib.a" + archive.write_text("stale\n", encoding="utf-8") + obj = tmp_path / "keep.o" + obj.write_bytes(b"obj:keep.c") + + code = main([str(archive), archiver_name, "rcs", str(archive), str(obj)]) + assert code == 0, "cwd-only archiver must run via absolute resolution" + assert "keep.o" in archive.read_text(encoding="utf-8") + assert "stale" not in archive.read_text(encoding="utf-8") if __name__ == "__main__":