diff --git a/README.md b/README.md index 8b1f5dc..9150bfb 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 7039b08..b95c7f1 100644 --- a/TESTING.md +++ b/TESTING.md @@ -66,3 +66,20 @@ 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. 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 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/ninja_backend.py b/ebuild/build/ninja_backend.py index de417bd..20fb1b5 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,16 @@ 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" + # 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 f"{shlex.quote(sys.executable)} {shlex.quote(str(helper))}" + ).replace("$", "$$") lines = [ f"# Generated by ebuild", f"cc = {self.toolchain.cc}", @@ -204,7 +216,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 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 new file mode 100644 index 0000000..410a77c --- /dev/null +++ b/ebuild/build/recreate_archive.py @@ -0,0 +1,79 @@ +# 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 script by absolute path as:: + + 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 os +import shutil +import subprocess +import sys +from pathlib import Path + + +def _resolve_archiver(archiver: str) -> str | None: + """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 os.path.abspath(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: + print( + "ebuild: recreate_archive: expected $out $ar rcs $out …members", + file=sys.stderr, + ) + return 2 + + archive = Path(args[0]) + archiver = args[1] + # Refuse to destroy a good archive when the archiver cannot run. + 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: + 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([resolved, *args[2:]]) + except OSError as exc: + print(f"ebuild: cannot run archiver {archiver}: {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 c0dd009..3e23955 100644 --- a/tests/unit/test_ninja_backend.py +++ b/tests/unit/test_ninja_backend.py @@ -360,5 +360,382 @@ 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_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"] + ) + 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 "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 + + 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_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"] = "" + + 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, capsys): + """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 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, capsys): + """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" + assert "present but not executable" in capsys.readouterr().err + + 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 + + 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" + 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__": unittest.main()