From a65206b40b703cd14b91122c3b19a9461397ceab Mon Sep 17 00:00:00 2001 From: Thompson Opeyemi <57995305+thompsondev@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:52:40 +0100 Subject: [PATCH 1/7] fix(ebuild): ask ninja for the Windows test binary Native type: test targets link as name.exe on Windows, but ebuild test asked ninja to build the unsuffixed path. Pytest collection also aborted when gcc was missing. Signed-off-by: Thompson Opeyemi <57995305+thompsondev@users.noreply.github.com> Co-authored-by: Cursor --- CHANGELOG.md | 12 +++++++ README.md | 5 +++ TASKS.md | 3 +- docs/compatibility.md | 1 + ebuild/build/ninja_backend.py | 23 +++++++++++- ebuild/cli/commands.py | 15 +++++--- tests/ebuild/test_build_dir_resolution.py | 34 +++++++++++++++++- tests/unit/test_golden_path_commands.py | 44 +++++++++++++++++++++++ 8 files changed, 130 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 536e83a0..46b650f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,18 @@ ## [Unreleased] ### Fixed +- **`ebuild test` now finds Windows test binaries.** Native `type: test` + targets are linked as `.exe` on Windows (gcc appends the suffix), + but `ebuild test` asked ninja to build `` and then looked for that + unsuffixed path. Ninja reported an unknown target and the runner reported + "built, but no binary". Both steps now use `executable_output_path()` + (`ebuild/build/ninja_backend.py`, `ebuild/cli/commands.py`). +- **`pytest` collection no longer aborts on Windows without gcc.** + `tests/ebuild/test_build_dir_resolution.py` evaluated + `subprocess.run(["gcc", "--version"])` in a `skipif`. When gcc is not + installed, Windows raises `FileNotFoundError` instead of a non-zero + returncode, which pytest treats as a collection ERROR and stops the + suite. The probe now uses `shutil.which` and catches `OSError`. - **A path containing a space produced a silently wrong `build.ninja`.** Paths were written into build statements unescaped, but Ninja ends the output list at the first unescaped `:` and splits on unescaped spaces. A build directory diff --git a/README.md b/README.md index 421a3ae7..a9034672 100644 --- a/README.md +++ b/README.md @@ -101,6 +101,11 @@ Additional commands: `configure`, `install`, `add`, `list-packages`, when it printed nothing recognisable — a number inferred from a zero exit status is a guess presented as a measurement. +Projects that declare `type: test` targets in `build.yaml` are built and run +directly (no ctest/cargo/meson). On Windows those binaries are named +`.exe`, matching the Ninja edge; `ebuild test` now asks ninja for that +path rather than the unsuffixed name. + It also treats a run that executed **no tests** as a failure. `ctest` exits `0` when it finds nothing to run, and a `CMakeLists.txt` with `enable_testing()` and no `add_test()` still produces a `CTestTestfile.cmake` — so the runner is found, diff --git a/TASKS.md b/TASKS.md index b7702ab6..48fe8690 100644 --- a/TASKS.md +++ b/TASKS.md @@ -11,7 +11,8 @@ Status is one of: `todo`, `in-progress`, `blocked`, `review`, `done`. | ID | Task | Owner | Mode | Status | Depends on | |----|------|-------|------|--------|------------| -| T-002 | Fix Windows Ninja test-target path parsing | backend | Maintenance | todo | none | +| T-002 | Fix Windows Ninja test-target path parsing | backend | Maintenance | review | none | +| T-003 | `ebuild package` looks for the unsuffixed binary on Windows (`_build/app` rather than `_build/app.exe`) | backend | Maintenance | todo | none | ## Completed diff --git a/docs/compatibility.md b/docs/compatibility.md index b9896589..f4851307 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -103,6 +103,7 @@ Download from [ARM Developer](https://developer.arm.com/downloads/-/gnu-rm) and | Limitation | Affected | Workaround | |-----------|----------|------------| | eOSuite excluded on Windows | `--with eosuite` | Use WSL2 or Linux VM | +| `ebuild package` looks for `_build/` rather than `_build/.exe` | Windows | Tracked as T-003 | | EIPC Go build separate from CMake | `layers/eipc/` | Build EIPC separately with `make` | | ESP32 requires ESP-IDF | `--target esp32` | Install ESP-IDF v5.x first | | MIPS toolchain not in most distros | `--target malta` | Build from source or use Docker | diff --git a/ebuild/build/ninja_backend.py b/ebuild/build/ninja_backend.py index 6557c5d0..abe67c17 100644 --- a/ebuild/build/ninja_backend.py +++ b/ebuild/build/ninja_backend.py @@ -41,6 +41,27 @@ def _exe_suffix() -> str: return ".exe" if sys.platform == "win32" else "" +def executable_output_path(build_dir: Path, target_name: str) -> Path: + """Return the linked binary path NinjaBackend emits for *target_name*. + + Args: + build_dir: Directory that contains ``build.ninja`` and the linked + outputs. + target_name: The ``name`` of an ``executable`` or ``test`` target. + + Returns: + ``build_dir / target_name`` on POSIX, or that path with ``.exe`` + appended on Windows, matching the compiler driver's output. + + Example: + >>> from pathlib import Path + >>> executable_output_path(Path("_build"), "hello").name in ( + ... "hello", "hello.exe") + True + """ + return Path(build_dir) / (target_name + _exe_suffix()) + + def _shared_flag() -> str: """The flag that makes the compiler driver emit a shared object. @@ -230,7 +251,7 @@ def _write_ninja(self) -> None: link_inputs = obj_files + dep_archives out = escape_ninja_path( - self.build_dir / (target.name + _exe_suffix())) + executable_output_path(self.build_dir, target.name)) lines.append( f"build {out}: link " f"{' '.join(link_inputs)}" ) diff --git a/ebuild/cli/commands.py b/ebuild/cli/commands.py index d3166e3e..c14b8921 100644 --- a/ebuild/cli/commands.py +++ b/ebuild/cli/commands.py @@ -26,7 +26,11 @@ import yaml from ebuild import __version__ -from ebuild.build.ninja_backend import NinjaBackend, PackagePaths +from ebuild.build.ninja_backend import ( + NinjaBackend, + PackagePaths, + executable_output_path, +) from ebuild.build.toolchain import resolve_toolchain from ebuild.cli.integration import register_commands as _register_integration_commands from ebuild.cli.logger import Logger @@ -2609,11 +2613,14 @@ def _run_native_tests( from ebuild.build.dispatch import ninja_command # Ninja addresses targets by their output path, and `ebuild build` drives - # it with -f from the project root, so the same form is used here. + # it with -f from the project root, so the same form is used here. The + # path must include the platform suffix: on Windows the edge is + # ``.exe``, and asking ninja to build ```` is an unknown + # target. argv = ( ninja_command() + ["-f", str(build_path / "build.ninja")] - + [str(build_path / t.name) for t in selected] + + [str(executable_output_path(build_path, t.name)) for t in selected] ) result = subprocess.run(argv) if result.returncode != 0: @@ -2622,7 +2629,7 @@ def _run_native_tests( failures: List[str] = [] for target in selected: - binary = build_path / target.name + binary = executable_output_path(build_path, target.name) if not binary.is_file(): log.error(f"{target.name}: built, but no binary at {binary}") failures.append(target.name) diff --git a/tests/ebuild/test_build_dir_resolution.py b/tests/ebuild/test_build_dir_resolution.py index 28d9eb32..fa2ab21e 100644 --- a/tests/ebuild/test_build_dir_resolution.py +++ b/tests/ebuild/test_build_dir_resolution.py @@ -26,6 +26,7 @@ from __future__ import annotations import os +import shutil import subprocess import textwrap from pathlib import Path @@ -242,8 +243,39 @@ def test_configure_and_build_from_outside_agree_on_the_build_dir( # ── end to end, with a real compiler ──────────────────────── +def _gcc_is_missing() -> bool: + """True when this host cannot run gcc. + + ``subprocess.run(['gcc', ...])`` raises FileNotFoundError on Windows + when gcc is not installed. Evaluating that in ``skipif`` is not a skip: + it aborts collection of this file and, with default pytest, the suite. + """ + gcc = shutil.which("gcc") + if gcc is None: + return True + try: + return subprocess.run( + [gcc, "--version"], capture_output=True + ).returncode != 0 + except OSError: + return True + + +def test_gcc_probe_does_not_raise_when_gcc_cannot_start(monkeypatch): + """Collection must stay a skip, not an ERROR, if gcc is absent.""" + monkeypatch.setattr( + shutil, "which", lambda name: r"C:\missing\gcc.exe" + ) + + def boom(*args, **kwargs): + raise FileNotFoundError(2, "The system cannot find the file specified") + + monkeypatch.setattr(subprocess, "run", boom) + assert _gcc_is_missing() is True + + @pytest.mark.skipif( - subprocess.run(["gcc", "--version"], capture_output=True).returncode != 0, + _gcc_is_missing(), reason="needs a working gcc to link the executable", ) def test_end_to_end_build_from_outside_produces_the_binary(tmp_path, monkeypatch): diff --git a/tests/unit/test_golden_path_commands.py b/tests/unit/test_golden_path_commands.py index a6a0ee22..4b2ba8fb 100644 --- a/tests/unit/test_golden_path_commands.py +++ b/tests/unit/test_golden_path_commands.py @@ -168,6 +168,50 @@ def outputs(line): assert ": link " in edge assert ": ar_rule" not in edge + def test_native_runner_asks_ninja_for_the_linked_binary(self, tmp_path, monkeypatch): + """`ebuild test` must name the same output NinjaBackend linked. + + On Windows the edge is ``t_smoke.exe`` because gcc appends ``.exe``. + Asking ninja to build ``t_smoke`` is an unknown target, and looking + for the unsuffixed path then reports "built, but no binary". + """ + import sys + from types import SimpleNamespace + + from ebuild.build.ninja_backend import NinjaBackend + from ebuild.cli import commands + from ebuild.core.config import ProjectConfig + + cfg = ProjectConfig( + name="p", version="1", source_dir=tmp_path, + targets=[TargetConfig(name="t_smoke", target_type="test", + sources=["t.c"])], + ) + build = tmp_path / "b" + NinjaBackend(cfg, build, + SimpleNamespace(cc="cc", cxx="c++", ar="ar")).generate() + + suffix = ".exe" if sys.platform == "win32" else "" + expected = build / f"t_smoke{suffix}" + expected.write_bytes(b"") + + monkeypatch.setattr(commands, "_configure_ninja_backend", + lambda *a, **k: None) + + runs = [] + + def fake_run(argv, *a, **k): + runs.append([str(x) for x in argv]) + return SimpleNamespace(returncode=0) + + monkeypatch.setattr(commands.subprocess, "run", fake_run) + + commands._run_native_tests( + cfg, list(cfg.targets), build, _SilentLog(), None) + + assert runs, "expected ninja to be invoked" + assert str(expected) in runs[0], runs[0] + @pytest.mark.ebuild class TestExternalTestRunners: From 64aa812bd78cbc84ee352c1bfaf13d5af4afd1e7 Mon Sep 17 00:00:00 2001 From: Thompson Opeyemi <57995305+thompsondev@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:44:33 +0100 Subject: [PATCH 2/7] fix(test_footprint): skip test if gcc is not available Added a conditional skip for the test measuring a real binary, ensuring it only runs when a working gcc is present. This prevents unnecessary test failures on systems without gcc installed. Co-authored-by: Cursor --- tests/unit/test_footprint.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_footprint.py b/tests/unit/test_footprint.py index 54508539..a73d9f27 100644 --- a/tests/unit/test_footprint.py +++ b/tests/unit/test_footprint.py @@ -15,6 +15,7 @@ """ import os +import shutil import subprocess from pathlib import Path @@ -57,13 +58,20 @@ def test_a_pure_bss_buffer_costs_ram_but_not_flash(self): class TestMeasure: + @pytest.mark.skipif( + shutil.which("gcc") is None, + reason="needs a working gcc to link the executable", + ) def test_measures_a_real_binary(self, tmp_path): src = tmp_path / "m.c" src.write_text("static char buf[4096];\nint main(void){return buf[0];}\n") # gcc on Windows appends .exe when -o names no extension, so the # path measured here has to carry it or there is nothing to measure. exe = tmp_path / ("m.exe" if os.name == "nt" else "m") - subprocess.run(["gcc", str(src), "-o", str(exe)], check=True) + try: + subprocess.run(["gcc", str(src), "-o", str(exe)], check=True) + except FileNotFoundError: + pytest.skip("needs a working gcc to link the executable") fp = measure(exe) assert fp.text > 0 From c7d83d86ef995cc5bbae481b197671a76e5cdf13 Mon Sep 17 00:00:00 2001 From: Thompson Opeyemi <57995305+thompsondev@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:01:20 +0100 Subject: [PATCH 3/7] fix: address PR #110 review findings 1, 2, and 4 Finding 1 (medium): test_native_runner_asks_ninja_for_the_linked_binary read sys.platform to pick the expected suffix, so on any POSIX runner it asserted the unsuffixed path -- exactly what the unfixed code produced -- and passed against the bug. Force _exe_suffix() to ".exe" instead, so the test exercises the Windows path on any host. Verified it now fails against the pre-fix argv construction and passes against the fix. Finding 2 (low): `ebuild package` still read `Path(build_dir) / binaries[0].name` instead of the executable_output_path() helper the ninja edge and `ebuild test` were just converted to, so it reported "No built artifact" after a successful Windows build. Took the one-line fix now that the helper exists, closed out T-003 in TASKS.md and docs/compatibility.md, and added a suffix-forcing regression test mirroring finding 1's. Doing so surfaced that the pre-existing package-command test fixtures wrote an unsuffixed stand-in binary; on this suite's real Windows host the fixed lookup could no longer find it, so those fixtures now build their artifact through executable_output_path() too. Finding 4 (low): _gcc_is_missing() only had a test for the OSError branch (gcc present but unable to start); the shutil.which() -> None branch, the common case on a bare Windows host, was untested. Added that case. Verified on this Windows host: full suite 559 passed, 6 skipped, 0 failed (previously 1 known pre-existing gcc-dependent failure). Finding 3 (no CI has run on this head) is a maintainer action (approve the queued workflow runs) and isn't addressed here. --- CHANGELOG.md | 5 ++++ TASKS.md | 2 +- docs/compatibility.md | 1 - ebuild/cli/commands.py | 2 +- tests/ebuild/test_build_dir_resolution.py | 6 +++++ tests/unit/test_golden_path_commands.py | 11 ++++++--- tests/unit/test_package_efw.py | 29 +++++++++++++++++++++-- 7 files changed, 48 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46b650f1..1bc37f26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ unsuffixed path. Ninja reported an unknown target and the runner reported "built, but no binary". Both steps now use `executable_output_path()` (`ebuild/build/ninja_backend.py`, `ebuild/cli/commands.py`). +- **`ebuild package` now finds the Windows binary too.** It looked up + `/` directly instead of through `executable_output_path()`, + so on Windows it reported "No built artifact" after a build that had + succeeded. Now uses the same helper as `ebuild test` + (`ebuild/cli/commands.py`). - **`pytest` collection no longer aborts on Windows without gcc.** `tests/ebuild/test_build_dir_resolution.py` evaluated `subprocess.run(["gcc", "--version"])` in a `skipif`. When gcc is not diff --git a/TASKS.md b/TASKS.md index 48fe8690..256b291e 100644 --- a/TASKS.md +++ b/TASKS.md @@ -12,13 +12,13 @@ Status is one of: `todo`, `in-progress`, `blocked`, `review`, `done`. | ID | Task | Owner | Mode | Status | Depends on | |----|------|-------|------|--------|------------| | T-002 | Fix Windows Ninja test-target path parsing | backend | Maintenance | review | none | -| T-003 | `ebuild package` looks for the unsuffixed binary on Windows (`_build/app` rather than `_build/app.exe`) | backend | Maintenance | todo | none | ## Completed | ID | Task | Owner | Verified by | Evidence | |----|------|-------|-------------|----------| | T-001 | Make initramfs creation portable and self-contained | backend | independent reviewer | Focused archive tests: **5 passed, 1 skipped** (symlink creation unavailable on this Windows host). Independent `bsdtar` extraction validated hard-link identity and payload. Full Python suite: **288 passed, 2 skipped, 1 unrelated failure** in the pre-existing Windows Ninja path assertion, recorded as T-002. QEMU boot was not run on Windows. | +| T-003 | `ebuild package` looks for the unsuffixed binary on Windows (`_build/app` rather than `_build/app.exe`) | backend | self (see PR #110 review, finding 2) | Was deferred out of T-002 for reviewability, then folded back in once `executable_output_path()` existed: `ebuild/cli/commands.py` now calls it at the `package` artifact lookup instead of `Path(build_dir) / name`. Covered by `tests/unit/test_package_efw.py::TestCommandPacks::test_it_finds_the_windows_suffixed_artifact`, which forces `_exe_suffix()` to `.exe` so it exercises the Windows path on any host, and also caught the fix's ripple effect on the suite's own real-Windows host: existing `test_package_efw.py` fixtures wrote an unsuffixed stand-in binary, which the fixed lookup could no longer find natively (`_exe_suffix()` returns `.exe` there unforced), so those fixtures now build the artifact through `executable_output_path()` too. Full suite run on this Windows host: **559 passed, 6 skipped, 0 failed**. | --- diff --git a/docs/compatibility.md b/docs/compatibility.md index f4851307..b9896589 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -103,7 +103,6 @@ Download from [ARM Developer](https://developer.arm.com/downloads/-/gnu-rm) and | Limitation | Affected | Workaround | |-----------|----------|------------| | eOSuite excluded on Windows | `--with eosuite` | Use WSL2 or Linux VM | -| `ebuild package` looks for `_build/` rather than `_build/.exe` | Windows | Tracked as T-003 | | EIPC Go build separate from CMake | `layers/eipc/` | Build EIPC separately with `make` | | ESP32 requires ESP-IDF | `--target esp32` | Install ESP-IDF v5.x first | | MIPS toolchain not in most distros | `--target malta` | Build from source or use Docker | diff --git a/ebuild/cli/commands.py b/ebuild/cli/commands.py index c14b8921..29d6b0ba 100644 --- a/ebuild/cli/commands.py +++ b/ebuild/cli/commands.py @@ -2530,7 +2530,7 @@ def package(log: Logger, config_path: str, build_dir: str, log.error("No executable target in build.yaml — nothing to package.") raise SystemExit(1) - artifact = Path(build_dir) / binaries[0].name + artifact = executable_output_path(Path(build_dir), binaries[0].name) if not artifact.is_file(): log.error(f"No built artifact at {artifact}. Run 'ebuild build' first.") raise SystemExit(1) diff --git a/tests/ebuild/test_build_dir_resolution.py b/tests/ebuild/test_build_dir_resolution.py index fa2ab21e..162c021f 100644 --- a/tests/ebuild/test_build_dir_resolution.py +++ b/tests/ebuild/test_build_dir_resolution.py @@ -274,6 +274,12 @@ def boom(*args, **kwargs): assert _gcc_is_missing() is True +def test_gcc_probe_reports_missing_when_which_finds_nothing(monkeypatch): + """The common case on a bare Windows host: no gcc on PATH at all.""" + monkeypatch.setattr(shutil, "which", lambda name: None) + assert _gcc_is_missing() is True + + @pytest.mark.skipif( _gcc_is_missing(), reason="needs a working gcc to link the executable", diff --git a/tests/unit/test_golden_path_commands.py b/tests/unit/test_golden_path_commands.py index 4b2ba8fb..ddad86dc 100644 --- a/tests/unit/test_golden_path_commands.py +++ b/tests/unit/test_golden_path_commands.py @@ -174,14 +174,20 @@ def test_native_runner_asks_ninja_for_the_linked_binary(self, tmp_path, monkeypa On Windows the edge is ``t_smoke.exe`` because gcc appends ``.exe``. Asking ninja to build ``t_smoke`` is an unknown target, and looking for the unsuffixed path then reports "built, but no binary". + + ``_exe_suffix()`` is forced to ``.exe`` rather than switching on + ``sys.platform``, so this exercises the Windows path — and fails + against the pre-fix code — on any host the suite runs on. """ - import sys from types import SimpleNamespace + from ebuild.build import ninja_backend from ebuild.build.ninja_backend import NinjaBackend from ebuild.cli import commands from ebuild.core.config import ProjectConfig + monkeypatch.setattr(ninja_backend, "_exe_suffix", lambda: ".exe") + cfg = ProjectConfig( name="p", version="1", source_dir=tmp_path, targets=[TargetConfig(name="t_smoke", target_type="test", @@ -191,8 +197,7 @@ def test_native_runner_asks_ninja_for_the_linked_binary(self, tmp_path, monkeypa NinjaBackend(cfg, build, SimpleNamespace(cc="cc", cxx="c++", ar="ar")).generate() - suffix = ".exe" if sys.platform == "win32" else "" - expected = build / f"t_smoke{suffix}" + expected = build / "t_smoke.exe" expected.write_bytes(b"") monkeypatch.setattr(commands, "_configure_ninja_backend", diff --git a/tests/unit/test_package_efw.py b/tests/unit/test_package_efw.py index 91d2be2c..479d0c61 100644 --- a/tests/unit/test_package_efw.py +++ b/tests/unit/test_package_efw.py @@ -31,6 +31,7 @@ pack, verify, ) +from ebuild.build.ninja_backend import executable_output_path from ebuild.cli.commands import cli @@ -164,7 +165,8 @@ def _project(self, tmp_path, built=True): })) if built: (tmp_path / "_build").mkdir() - (tmp_path / "_build" / "node").write_bytes(b"\x7fELF" + b"\x00" * 64) + executable_output_path(tmp_path / "_build", "node").write_bytes( + b"\x7fELF" + b"\x00" * 64) return tmp_path def test_it_refuses_before_a_build(self, tmp_path, monkeypatch): @@ -244,7 +246,8 @@ def _project(self, tmp_path): "sources": ["src/main.c"]}], })) (tmp_path / "_build").mkdir() - (tmp_path / "_build" / "node").write_bytes(b"\x7fELF" + b"\x00" * 64) + executable_output_path(tmp_path / "_build", "node").write_bytes( + b"\x7fELF" + b"\x00" * 64) return tmp_path def _packaged(self, tmp_path, monkeypatch, argv=(), **tool_kw): @@ -261,6 +264,28 @@ def test_it_writes_the_image_and_reports_its_size(self, tmp_path, monkeypatch): assert image.is_file() assert str(image.stat().st_size) in result.output + def test_it_finds_the_windows_suffixed_artifact(self, tmp_path, monkeypatch): + """On Windows the linked binary is `.exe`; `package` must look + for that path rather than the unsuffixed one NinjaBackend never + produces there. + + `_exe_suffix()` is forced to `.exe` rather than switching on + `sys.platform`, so this exercises the Windows path -- and fails + against the pre-fix code, which looked for the unsuffixed name -- + on any host the suite runs on. + """ + from ebuild.build import ninja_backend + monkeypatch.setattr(ninja_backend, "_exe_suffix", lambda: ".exe") + + tool = _efwtool_that_packs(tmp_path) + monkeypatch.chdir(self._project(tmp_path)) + assert (tmp_path / "_build" / "node.exe").is_file() + monkeypatch.setattr("ebuild.build.firmware_image.shutil.which", + lambda n: str(tool) if n == "efwtool" else None) + result = CliRunner().invoke(cli, ["package"]) + assert result.exit_code == 0, result.output + assert (tmp_path / "node.efw").is_file() + def test_the_default_name_comes_from_the_project(self, tmp_path, monkeypatch): """Not from the target or the build directory: `--output` is optional, so the fallback is the name a developer will look for.""" From 26a767f4043bb114f40d2e085145d7e5f42d055f Mon Sep 17 00:00:00 2001 From: Thompson Opeyemi <57995305+thompsondev@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:42:58 +0100 Subject: [PATCH 4/7] fix: address PR #110 review findings 1 and 4 (round two) Finding 1 (high): _report_footprint -- the flash/RAM summary `ebuild build` prints -- was the third of three call sites building an executable path from a target name, and the only one left unsuffixed. On Windows it silently returned with no diagnostic (the other two early exits in the function already log at debug level), so a developer had no way to tell the report was missing rather than not applicable. Uses executable_output_path() now, and the missing-artifact return logs at debug level to match the function's other guards. Added a suffix-forcing regression test mirroring the two this PR already has; confirmed it fails against the pre-fix lookup and passes against the fix. Finding 4 (low): _gcc_is_missing() existed twice -- the hardened version in tests/ebuild/test_build_dir_resolution.py and a bare shutil.which() check in tests/unit/test_footprint.py that the inner try/except only patched over. Hoisted the probe into tests/conftest.py as gcc_is_missing(), used from both files, and dropped the now-dead inner try/except in test_footprint.py. Findings 2 and 3 (PR body vs diff, and contradictory test-result numbers) are description-only issues on the open PR, not code -- not addressed in this commit. Full suite on this Windows host: 560 passed, 6 skipped, 0 failed. --- CHANGELOG.md | 7 +++ TASKS.md | 1 + ebuild/cli/commands.py | 3 +- tests/conftest.py | 28 +++++++++++ tests/ebuild/test_build_dir_resolution.py | 27 +++-------- tests/unit/test_footprint.py | 58 ++++++++++++++++++++--- 6 files changed, 97 insertions(+), 27 deletions(-) create mode 100644 tests/conftest.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bc37f26..7f013f6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,13 @@ so on Windows it reported "No built artifact" after a build that had succeeded. Now uses the same helper as `ebuild test` (`ebuild/cli/commands.py`). +- **`ebuild build`'s flash/RAM report went silent on Windows.** + `_report_footprint` also looked up `/` directly, so on + Windows the artifact was never found and the function returned with no + diagnostic — the report just never appeared, with no indication it was + skipped rather than not applicable. Now uses `executable_output_path()` + and logs at debug level when it has nothing to measure + (`ebuild/cli/commands.py`). - **`pytest` collection no longer aborts on Windows without gcc.** `tests/ebuild/test_build_dir_resolution.py` evaluated `subprocess.run(["gcc", "--version"])` in a `skipif`. When gcc is not diff --git a/TASKS.md b/TASKS.md index 256b291e..2f49d9f0 100644 --- a/TASKS.md +++ b/TASKS.md @@ -19,6 +19,7 @@ Status is one of: `todo`, `in-progress`, `blocked`, `review`, `done`. |----|------|-------|-------------|----------| | T-001 | Make initramfs creation portable and self-contained | backend | independent reviewer | Focused archive tests: **5 passed, 1 skipped** (symlink creation unavailable on this Windows host). Independent `bsdtar` extraction validated hard-link identity and payload. Full Python suite: **288 passed, 2 skipped, 1 unrelated failure** in the pre-existing Windows Ninja path assertion, recorded as T-002. QEMU boot was not run on Windows. | | T-003 | `ebuild package` looks for the unsuffixed binary on Windows (`_build/app` rather than `_build/app.exe`) | backend | self (see PR #110 review, finding 2) | Was deferred out of T-002 for reviewability, then folded back in once `executable_output_path()` existed: `ebuild/cli/commands.py` now calls it at the `package` artifact lookup instead of `Path(build_dir) / name`. Covered by `tests/unit/test_package_efw.py::TestCommandPacks::test_it_finds_the_windows_suffixed_artifact`, which forces `_exe_suffix()` to `.exe` so it exercises the Windows path on any host, and also caught the fix's ripple effect on the suite's own real-Windows host: existing `test_package_efw.py` fixtures wrote an unsuffixed stand-in binary, which the fixed lookup could no longer find natively (`_exe_suffix()` returns `.exe` there unforced), so those fixtures now build the artifact through `executable_output_path()` too. Full suite run on this Windows host: **559 passed, 6 skipped, 0 failed**. | +| T-004 | `_report_footprint` (the flash/RAM report `ebuild build` prints) looks for the unsuffixed binary on Windows, and fails silently rather than logging why | backend | self (see PR #110 review, finding 1) | Third of three `build_dir / name` call sites, and the only one with no diagnostic on the early-return path. `ebuild/cli/commands.py:516` now uses `executable_output_path()`, and the bare `return` on a missing artifact now logs at debug level, matching the function's other two early exits. Covered by `tests/unit/test_footprint.py::TestCLIFootprintReport::test_looks_up_the_windows_suffixed_artifact`, which forces `_exe_suffix()` to `.exe`; confirmed to fail against the pre-fix lookup (no report emitted) and pass against the fix. Full suite on this Windows host: **560 passed, 6 skipped, 0 failed**. | --- diff --git a/ebuild/cli/commands.py b/ebuild/cli/commands.py index 29d6b0ba..98201e06 100644 --- a/ebuild/cli/commands.py +++ b/ebuild/cli/commands.py @@ -513,8 +513,9 @@ def _report_footprint(cfg: "ProjectConfig", build_path: Path, log: Logger) -> No if not binaries: return - artifact = build_path / binaries[0].name + artifact = executable_output_path(build_path, binaries[0].name) if not artifact.is_file(): + log.debug(f"no artifact at {artifact}; skipping footprint") return prefix = getattr(cfg.toolchain, "target", None) or "host" diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 00000000..4f605ccf --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,28 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 EoS Project + +"""Shared pytest helpers for the ebuild test suite.""" + +import shutil +import subprocess + + +def gcc_is_missing() -> bool: + """True when this host cannot run gcc. + + ``subprocess.run(['gcc', ...])`` raises ``FileNotFoundError`` on Windows + when gcc is not installed. Evaluating that directly in ``skipif`` is not + a skip: it aborts collection of the file and, with default pytest, the + suite. Used by both ``tests/ebuild/test_build_dir_resolution.py`` and + ``tests/unit/test_footprint.py`` -- previously each carried its own copy + of this probe, one of them the version this existed to fix. + """ + gcc = shutil.which("gcc") + if gcc is None: + return True + try: + return subprocess.run( + [gcc, "--version"], capture_output=True + ).returncode != 0 + except OSError: + return True diff --git a/tests/ebuild/test_build_dir_resolution.py b/tests/ebuild/test_build_dir_resolution.py index 162c021f..428ff338 100644 --- a/tests/ebuild/test_build_dir_resolution.py +++ b/tests/ebuild/test_build_dir_resolution.py @@ -36,6 +36,7 @@ from click.testing import CliRunner from ebuild.cli import commands +from tests.conftest import gcc_is_missing pytestmark = pytest.mark.needs_yaml @@ -242,23 +243,9 @@ def test_configure_and_build_from_outside_agree_on_the_build_dir( # ── end to end, with a real compiler ──────────────────────── - -def _gcc_is_missing() -> bool: - """True when this host cannot run gcc. - - ``subprocess.run(['gcc', ...])`` raises FileNotFoundError on Windows - when gcc is not installed. Evaluating that in ``skipif`` is not a skip: - it aborts collection of this file and, with default pytest, the suite. - """ - gcc = shutil.which("gcc") - if gcc is None: - return True - try: - return subprocess.run( - [gcc, "--version"], capture_output=True - ).returncode != 0 - except OSError: - return True +# `gcc_is_missing()` lives in tests/conftest.py, shared with +# tests/unit/test_footprint.py -- both need to know whether the host can +# link a real binary before running a skipif against it. def test_gcc_probe_does_not_raise_when_gcc_cannot_start(monkeypatch): @@ -271,17 +258,17 @@ def boom(*args, **kwargs): raise FileNotFoundError(2, "The system cannot find the file specified") monkeypatch.setattr(subprocess, "run", boom) - assert _gcc_is_missing() is True + assert gcc_is_missing() is True def test_gcc_probe_reports_missing_when_which_finds_nothing(monkeypatch): """The common case on a bare Windows host: no gcc on PATH at all.""" monkeypatch.setattr(shutil, "which", lambda name: None) - assert _gcc_is_missing() is True + assert gcc_is_missing() is True @pytest.mark.skipif( - _gcc_is_missing(), + gcc_is_missing(), reason="needs a working gcc to link the executable", ) def test_end_to_end_build_from_outside_produces_the_binary(tmp_path, monkeypatch): diff --git a/tests/unit/test_footprint.py b/tests/unit/test_footprint.py index a73d9f27..d9146f68 100644 --- a/tests/unit/test_footprint.py +++ b/tests/unit/test_footprint.py @@ -15,7 +15,6 @@ """ import os -import shutil import subprocess from pathlib import Path @@ -33,6 +32,7 @@ measure, over_budget, ) +from tests.conftest import gcc_is_missing class TestAccounting: @@ -59,7 +59,7 @@ def test_a_pure_bss_buffer_costs_ram_but_not_flash(self): class TestMeasure: @pytest.mark.skipif( - shutil.which("gcc") is None, + gcc_is_missing(), reason="needs a working gcc to link the executable", ) def test_measures_a_real_binary(self, tmp_path): @@ -68,10 +68,7 @@ def test_measures_a_real_binary(self, tmp_path): # gcc on Windows appends .exe when -o names no extension, so the # path measured here has to carry it or there is nothing to measure. exe = tmp_path / ("m.exe" if os.name == "nt" else "m") - try: - subprocess.run(["gcc", str(src), "-o", str(exe)], check=True) - except FileNotFoundError: - pytest.skip("needs a working gcc to link the executable") + subprocess.run(["gcc", str(src), "-o", str(exe)], check=True) fp = measure(exe) assert fp.text > 0 @@ -211,6 +208,55 @@ def test_units(self, n, expected): assert format_size(n) == expected +class TestCLIFootprintReport: + """`_report_footprint` (`ebuild/cli/commands.py`) is the CLI-level + consumer wired into `ebuild build`. Like `ebuild test` and `ebuild + package`, it has to look up the same suffixed binary NinjaBackend + linked.""" + + def test_looks_up_the_windows_suffixed_artifact(self, tmp_path, monkeypatch): + """On Windows the linked binary is `.exe`; the report must + look there, not at the unsuffixed name NinjaBackend never produces + on that platform -- the third of three call sites this rule applies + to, and the only one that failed silently. + + `_exe_suffix()` is forced to `.exe` rather than switching on + `sys.platform`, so this exercises the Windows path -- and fails + against the pre-fix code -- on any host the suite runs on. + """ + from types import SimpleNamespace + + from ebuild.build import ninja_backend + from ebuild.cli import commands + from ebuild.core.config import ProjectConfig, TargetConfig + + monkeypatch.setattr(ninja_backend, "_exe_suffix", lambda: ".exe") + monkeypatch.setattr( + "ebuild.build.footprint.find_size_tool", + lambda prefix: "/usr/bin/size") + monkeypatch.setattr( + "ebuild.build.footprint.measure", + lambda artifact, tool=None: Footprint(text=1000, data=200, bss=500)) + + cfg = ProjectConfig( + name="app", version="1", source_dir=tmp_path, + targets=[TargetConfig(name="app", target_type="executable", + sources=["a.c"])], + ) + build = tmp_path / "b" + build.mkdir() + (build / "app.exe").write_bytes(b"\x7fELF") + + logs = [] + log = SimpleNamespace( + info=logs.append, debug=lambda *a, **k: None, + warning=lambda *a, **k: None) + + commands._report_footprint(cfg, build, log) + + assert any("Flash" in line for line in logs), logs + + def _completed(returncode=0, stdout="", stderr=""): return subprocess.CompletedProcess( args=["size"], returncode=returncode, stdout=stdout, stderr=stderr) From 93fdebdd6fcc1889a2019379154d574d3ee4885c Mon Sep 17 00:00:00 2001 From: Thompson Opeyemi <57995305+thompsondev@users.noreply.github.com> Date: Fri, 4 Sep 2026 14:39:31 +0100 Subject: [PATCH 5/7] fix: address PR #110 review findings 2, 3, 4, 5 (round three) Finding 2 (medium): the PR/TASKS.md carried three different pytest result counts across the same fix's history, none of them tied to a specific head commit. Reconciled to a single run taken at this state (560 passed, 6 skipped, exit code 0), replacing the two superseded counts in TASKS.md. Finding 3 (medium): T-003 and T-004 were moved to Completed with "Verified by: self" by the same PR that implements them, which .ai/reviewer.md's role boundary ("if you implemented it, you do not approve it") rules out. Moved both back to the Active table at status review, alongside T-002; the evidence prose is preserved verbatim under a new Evidence section, explicitly marked self-reported and pending independent review. Finding 4 (low): the new footprint regression test called _report_footprint, which reads eos.yaml/board.yaml relative to the process cwd via _selected_board()/_board_config() -- unmonkeypatched, so the test only measured what it claimed to because neither file happens to exist at the repo root. Added monkeypatch.chdir(tmp_path). Finding 5 (low): tests/conftest.py held a plain helper function with no fixtures or hooks, imported by name from two test modules -- the one thing conftest.py's contract asks you not to do. Renamed to tests/support.py and updated both importers. Finding 1 (PR body vs. diff) and finding 6 (helper's module placement, explicitly non-blocking) are not addressed here: finding 1 needs edit access to the PR description, which this environment does not have (gh CLI installed but unauthenticated); finding 6 is filed as backlog per the reviewer's own recommendation, not this PR's scope. Full suite on this Windows host: 560 passed, 6 skipped, 0 failed. --- TASKS.md | 31 +++++++++++++++++++++-- tests/ebuild/test_build_dir_resolution.py | 4 +-- tests/{conftest.py => support.py} | 0 tests/unit/test_footprint.py | 8 +++++- 4 files changed, 38 insertions(+), 5 deletions(-) rename tests/{conftest.py => support.py} (100%) diff --git a/TASKS.md b/TASKS.md index 2f49d9f0..54ec4dd5 100644 --- a/TASKS.md +++ b/TASKS.md @@ -12,14 +12,41 @@ Status is one of: `todo`, `in-progress`, `blocked`, `review`, `done`. | ID | Task | Owner | Mode | Status | Depends on | |----|------|-------|------|--------|------------| | T-002 | Fix Windows Ninja test-target path parsing | backend | Maintenance | review | none | +| T-003 | `ebuild package` looks for the unsuffixed binary on Windows (`_build/app` rather than `_build/app.exe`) | backend | Maintenance | review | none | +| T-004 | `_report_footprint` (the flash/RAM report `ebuild build` prints) looks for the unsuffixed binary on Windows, and fails silently rather than logging why | backend | Maintenance | review | none | + +### Evidence (self-reported by implementer; pending independent review per `.ai/reviewer.md` — "if you implemented it, you do not approve it") + +- **T-003**: Was deferred out of T-002 for reviewability, then folded back in + once `executable_output_path()` existed: `ebuild/cli/commands.py` now calls + it at the `package` artifact lookup instead of `Path(build_dir) / name`. + Covered by + `tests/unit/test_package_efw.py::TestCommandPacks::test_it_finds_the_windows_suffixed_artifact`, + which forces `_exe_suffix()` to `.exe` so it exercises the Windows path on + any host, and also caught the fix's ripple effect on the suite's own + real-Windows host: existing `test_package_efw.py` fixtures wrote an + unsuffixed stand-in binary, which the fixed lookup could no longer find + natively (`_exe_suffix()` returns `.exe` there unforced), so those + fixtures now build the artifact through `executable_output_path()` too. +- **T-004**: Third of three `build_dir / name` call sites, and the only one + with no diagnostic on the early-return path. `ebuild/cli/commands.py:516` + now uses `executable_output_path()`, and the bare `return` on a missing + artifact now logs at debug level, matching the function's other two early + exits. Covered by + `tests/unit/test_footprint.py::TestCLIFootprintReport::test_looks_up_the_windows_suffixed_artifact`, + which forces `_exe_suffix()` to `.exe` and chdirs into `tmp_path` so the + process cwd's own `eos.yaml`/`board.yaml`, if any, cannot change what it + measures; confirmed to fail against the pre-fix lookup (no report + emitted) and pass against the fix. +- **Suite result** (single run, both changes present, this Windows host): + **560 passed, 6 skipped, exit code 0**. Supersedes any other count quoted + for T-003 or T-004 elsewhere in this repo or in PR #110's description. ## Completed | ID | Task | Owner | Verified by | Evidence | |----|------|-------|-------------|----------| | T-001 | Make initramfs creation portable and self-contained | backend | independent reviewer | Focused archive tests: **5 passed, 1 skipped** (symlink creation unavailable on this Windows host). Independent `bsdtar` extraction validated hard-link identity and payload. Full Python suite: **288 passed, 2 skipped, 1 unrelated failure** in the pre-existing Windows Ninja path assertion, recorded as T-002. QEMU boot was not run on Windows. | -| T-003 | `ebuild package` looks for the unsuffixed binary on Windows (`_build/app` rather than `_build/app.exe`) | backend | self (see PR #110 review, finding 2) | Was deferred out of T-002 for reviewability, then folded back in once `executable_output_path()` existed: `ebuild/cli/commands.py` now calls it at the `package` artifact lookup instead of `Path(build_dir) / name`. Covered by `tests/unit/test_package_efw.py::TestCommandPacks::test_it_finds_the_windows_suffixed_artifact`, which forces `_exe_suffix()` to `.exe` so it exercises the Windows path on any host, and also caught the fix's ripple effect on the suite's own real-Windows host: existing `test_package_efw.py` fixtures wrote an unsuffixed stand-in binary, which the fixed lookup could no longer find natively (`_exe_suffix()` returns `.exe` there unforced), so those fixtures now build the artifact through `executable_output_path()` too. Full suite run on this Windows host: **559 passed, 6 skipped, 0 failed**. | -| T-004 | `_report_footprint` (the flash/RAM report `ebuild build` prints) looks for the unsuffixed binary on Windows, and fails silently rather than logging why | backend | self (see PR #110 review, finding 1) | Third of three `build_dir / name` call sites, and the only one with no diagnostic on the early-return path. `ebuild/cli/commands.py:516` now uses `executable_output_path()`, and the bare `return` on a missing artifact now logs at debug level, matching the function's other two early exits. Covered by `tests/unit/test_footprint.py::TestCLIFootprintReport::test_looks_up_the_windows_suffixed_artifact`, which forces `_exe_suffix()` to `.exe`; confirmed to fail against the pre-fix lookup (no report emitted) and pass against the fix. Full suite on this Windows host: **560 passed, 6 skipped, 0 failed**. | --- diff --git a/tests/ebuild/test_build_dir_resolution.py b/tests/ebuild/test_build_dir_resolution.py index 428ff338..e08444f3 100644 --- a/tests/ebuild/test_build_dir_resolution.py +++ b/tests/ebuild/test_build_dir_resolution.py @@ -36,7 +36,7 @@ from click.testing import CliRunner from ebuild.cli import commands -from tests.conftest import gcc_is_missing +from tests.support import gcc_is_missing pytestmark = pytest.mark.needs_yaml @@ -243,7 +243,7 @@ def test_configure_and_build_from_outside_agree_on_the_build_dir( # ── end to end, with a real compiler ──────────────────────── -# `gcc_is_missing()` lives in tests/conftest.py, shared with +# `gcc_is_missing()` lives in tests/support.py, shared with # tests/unit/test_footprint.py -- both need to know whether the host can # link a real binary before running a skipif against it. diff --git a/tests/conftest.py b/tests/support.py similarity index 100% rename from tests/conftest.py rename to tests/support.py diff --git a/tests/unit/test_footprint.py b/tests/unit/test_footprint.py index d9146f68..e17aecaa 100644 --- a/tests/unit/test_footprint.py +++ b/tests/unit/test_footprint.py @@ -32,7 +32,7 @@ measure, over_budget, ) -from tests.conftest import gcc_is_missing +from tests.support import gcc_is_missing class TestAccounting: @@ -223,6 +223,11 @@ def test_looks_up_the_windows_suffixed_artifact(self, tmp_path, monkeypatch): `_exe_suffix()` is forced to `.exe` rather than switching on `sys.platform`, so this exercises the Windows path -- and fails against the pre-fix code -- on any host the suite runs on. + + `_report_footprint` also reads `eos.yaml`/`board.yaml` relative to + the process cwd (via `_selected_board`/`_board_config`), which this + test does not exercise -- chdir into `tmp_path` so a real cwd + carrying either file cannot change what this test measures. """ from types import SimpleNamespace @@ -230,6 +235,7 @@ def test_looks_up_the_windows_suffixed_artifact(self, tmp_path, monkeypatch): from ebuild.cli import commands from ebuild.core.config import ProjectConfig, TargetConfig + monkeypatch.chdir(tmp_path) monkeypatch.setattr(ninja_backend, "_exe_suffix", lambda: ".exe") monkeypatch.setattr( "ebuild.build.footprint.find_size_tool", From 0f8c1b717cc2df2b2345a4aaf7e5a4b4a7a7090f Mon Sep 17 00:00:00 2001 From: Thompson Opeyemi <57995305+thompsondev@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:27:29 +0100 Subject: [PATCH 6/7] docs: address PR #110 review findings 3, 4, 5 (round four) Finding 3 (low): executable_output_path()'s placement in the Ninja-specific backend was rated non-blocking last round, but no backlog entry was filed as recommended. Added T-005 to TASKS.md's Active table at status todo. Finding 4 (low): the CHANGELOG entry and the helper's own docstring attributed the original bug to "gcc appends the suffix" / "matching the compiler driver's output". That's not the mechanism: the Ninja edge already carried the suffix via _exe_suffix() inside _write_ninja; the CLI consumers rebuilt the path independently and dropped it. Reworded both to the actual internal cause. Finding 5 (low): T-002 -- the PR's original subject -- had no entry in the new Evidence section while T-003/T-004 each got one, an asymmetry the same review flagged in the opposite direction last round. Added a T-002 evidence bullet naming commands.py:2624/:2633 and the regression test that covers them. Finding 1 (PR body vs. diff) and finding 2 (no CI has run) are not addressed in this commit: finding 1 was already fixed via a browser edit to the PR description before this review posted -- the review appears to have analyzed a snapshot taken before that edit landed, since the specific stale phrases it quotes no longer appear on the live PR body (verified via the GitHub API). Finding 2 is a maintainer action. Full suite on this Windows host: 560 passed, 6 skipped, 0 failed. --- CHANGELOG.md | 14 ++++++++------ TASKS.md | 8 ++++++++ ebuild/build/ninja_backend.py | 6 +++++- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f013f6e..7dc169d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,12 +3,14 @@ ## [Unreleased] ### Fixed -- **`ebuild test` now finds Windows test binaries.** Native `type: test` - targets are linked as `.exe` on Windows (gcc appends the suffix), - but `ebuild test` asked ninja to build `` and then looked for that - unsuffixed path. Ninja reported an unknown target and the runner reported - "built, but no binary". Both steps now use `executable_output_path()` - (`ebuild/build/ninja_backend.py`, `ebuild/cli/commands.py`). +- **`ebuild test` now finds Windows test binaries.** The Ninja edge for a + native `type: test` target already carried the platform suffix + (`_exe_suffix()` names it `.exe` on Windows), but `ebuild test` + rebuilt the path itself without that suffix, asked ninja to build + ``, and then looked for that unsuffixed path. Ninja reported an + unknown target and the runner reported "built, but no binary". Both + steps now go through the same `executable_output_path()` the edge itself + uses (`ebuild/build/ninja_backend.py`, `ebuild/cli/commands.py`). - **`ebuild package` now finds the Windows binary too.** It looked up `/` directly instead of through `executable_output_path()`, so on Windows it reported "No built artifact" after a build that had diff --git a/TASKS.md b/TASKS.md index 54ec4dd5..74e97fe7 100644 --- a/TASKS.md +++ b/TASKS.md @@ -14,9 +14,17 @@ Status is one of: `todo`, `in-progress`, `blocked`, `review`, `done`. | T-002 | Fix Windows Ninja test-target path parsing | backend | Maintenance | review | none | | T-003 | `ebuild package` looks for the unsuffixed binary on Windows (`_build/app` rather than `_build/app.exe`) | backend | Maintenance | review | none | | T-004 | `_report_footprint` (the flash/RAM report `ebuild build` prints) looks for the unsuffixed binary on Windows, and fails silently rather than logging why | backend | Maintenance | review | none | +| T-005 | Move `executable_output_path()` out of the Ninja-specific backend into a backend-neutral module (`ebuild/build/layout.py`), re-exported from `ninja_backend` for compatibility | backend | Maintenance | todo | none | ### Evidence (self-reported by implementer; pending independent review per `.ai/reviewer.md` — "if you implemented it, you do not approve it") +- **T-002**: `ebuild/cli/commands.py:2624` and `:2633` (the Ninja target-list + argv and the binary that gets executed in `_run_native_tests`) both use + `executable_output_path()` instead of rebuilding the path themselves. + Covered by + `tests/unit/test_golden_path_commands.py::TestTestTargetType::test_native_runner_asks_ninja_for_the_linked_binary`, + which forces `_exe_suffix()` to `.exe`; confirmed to fail against the + pre-fix argv construction and pass against the fix. - **T-003**: Was deferred out of T-002 for reviewability, then folded back in once `executable_output_path()` existed: `ebuild/cli/commands.py` now calls it at the `package` artifact lookup instead of `Path(build_dir) / name`. diff --git a/ebuild/build/ninja_backend.py b/ebuild/build/ninja_backend.py index abe67c17..5ca29bb1 100644 --- a/ebuild/build/ninja_backend.py +++ b/ebuild/build/ninja_backend.py @@ -51,7 +51,11 @@ def executable_output_path(build_dir: Path, target_name: str) -> Path: Returns: ``build_dir / target_name`` on POSIX, or that path with ``.exe`` - appended on Windows, matching the compiler driver's output. + appended on Windows -- the same path the Ninja edge in + ``_write_ninja`` already names via ``_exe_suffix()``. A consumer + that rebuilds this path independently instead of calling this + function can silently drop the suffix and go looking for a binary + the edge never produced. Example: >>> from pathlib import Path From eef1cde3735758a5762fb86da764ad93b8447488 Mon Sep 17 00:00:00 2001 From: Thompson Opeyemi <57995305+thompsondev@users.noreply.github.com> Date: Fri, 4 Sep 2026 18:09:55 +0100 Subject: [PATCH 7/7] docs: address PR #110 review findings 2, 3 (round five) Finding 2 (low): the last surviving copy of the wrong root cause. test_native_runner_asks_ninja_for_the_linked_binary's docstring still said "gcc appends .exe" -- round four fixed this same sentence in CHANGELOG.md and the helper's own docstring but missed this one. Reworded to the actual mechanism: _exe_suffix() names the edge that way, and a consumer that rebuilds the path without it asks ninja for an unknown target. Finding 3 (low): the ebuild-test paragraph documenting the .exe fix was inserted between a sentence and its own continuation in README.md -- "...is a guess presented as a measurement." and "It also treats a run that executed no tests as a failure." are one thought about ebuild test's result reporting, and the new paragraph split them. Moved it after that paragraph instead. Finding 1 (no CI has run, fifth consecutive review) is a maintainer action, not addressed here. Full suite on this Windows host: 560 passed, 6 skipped, 0 failed. --- README.md | 10 +++++----- tests/unit/test_golden_path_commands.py | 7 ++++--- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index a9034672..d862ffee 100644 --- a/README.md +++ b/README.md @@ -101,17 +101,17 @@ Additional commands: `configure`, `install`, `add`, `list-packages`, when it printed nothing recognisable — a number inferred from a zero exit status is a guess presented as a measurement. -Projects that declare `type: test` targets in `build.yaml` are built and run -directly (no ctest/cargo/meson). On Windows those binaries are named -`.exe`, matching the Ninja edge; `ebuild test` now asks ninja for that -path rather than the unsuffixed name. - It also treats a run that executed **no tests** as a failure. `ctest` exits `0` when it finds nothing to run, and a `CMakeLists.txt` with `enable_testing()` and no `add_test()` still produces a `CTestTestfile.cmake` — so the runner is found, ctest prints `No tests were found!!!`, and trusting the exit status would report a green suite for a project with no tests at all. +Projects that declare `type: test` targets in `build.yaml` are built and run +directly (no ctest/cargo/meson). On Windows those binaries are named +`.exe`, matching the Ninja edge; `ebuild test` now asks ninja for that +path rather than the unsuffixed name. + ## Test ```bash diff --git a/tests/unit/test_golden_path_commands.py b/tests/unit/test_golden_path_commands.py index ddad86dc..6231196f 100644 --- a/tests/unit/test_golden_path_commands.py +++ b/tests/unit/test_golden_path_commands.py @@ -171,9 +171,10 @@ def outputs(line): def test_native_runner_asks_ninja_for_the_linked_binary(self, tmp_path, monkeypatch): """`ebuild test` must name the same output NinjaBackend linked. - On Windows the edge is ``t_smoke.exe`` because gcc appends ``.exe``. - Asking ninja to build ``t_smoke`` is an unknown target, and looking - for the unsuffixed path then reports "built, but no binary". + On Windows the edge is ``t_smoke.exe`` because ``_exe_suffix()`` + names it that way; a consumer that rebuilds the path without the + suffix asks ninja for an unknown target, and looking for the + unsuffixed path then reports "built, but no binary". ``_exe_suffix()`` is forced to ``.exe`` rather than switching on ``sys.platform``, so this exercises the Windows path — and fails