diff --git a/.claude/commands/apply.md b/.claude/commands/apply.md index 127f3c8e..61ab9830 100644 --- a/.claude/commands/apply.md +++ b/.claude/commands/apply.md @@ -268,7 +268,7 @@ The layout script reports, per page, where the text starts and stops, bottom whi The hole check is the one a visual read misses. A moderncv `\cventry` renders as a `tabular`, so it is an **unbreakable block**: when it does not fit in the space left, the whole entry jumps to the next page and leaves a hole behind, while the document still compiles and still reports the right page count. Fix it by shortening the entry that follows the hole, not by stretching the page. -If Poppler is missing, or the `pdftotext` first in PATH is the xpdf build Git for Windows ships (no `-bbox`), the script exits 2 with `skipped:` — note the degraded mode in the Step 6 report and rely on the visual inspection alone. Exit 2 is never a layout verdict. +If Poppler is missing, or no `pdftotext` on PATH supports `-bbox` (Git for Windows ships an xpdf build without it), the script exits 2 with `skipped:` - note the degraded mode in the Step 6 report and rely on the visual inspection alone. Exit 2 is never a layout verdict. The thresholds are calibrated for the stock moderncv and `cover.cls` geometry; a template registered via `/add-template` may report a phantom hole above a footer the 90pt band does not cover. diff --git a/CHANGELOG.md b/CHANGELOG.md index d2765614..059199e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -172,6 +172,13 @@ per-file diff commands. requires every shipped heading to appear in order, so an added section passes and a renamed, dropped or reordered one still fails; upstream keeps the exact comparison. +- **`verify_layout.py` uses a `pdftotext` that supports `-bbox` wherever it sits on PATH** (#668, + fixes #655, `tools/verify_layout.py`, `tests/test_verify_layout.py`, `.claude/commands/apply.md`) - + under Git Bash, which Claude Code uses on Windows, Git's xpdf `pdftotext` 4.00 (no `-bbox`) sits + ahead of MiKTeX's Poppler, so the layout check skipped on the standard Windows setup even with + Poppler installed. It now probes each distinct `pdftotext` on PATH with `-h`, runs the first that + lists `-bbox` by its full path, and keeps the `skipped:` exit 2 for when none does. + ## [1.7.3] - 2026-10-19 ### Added diff --git a/tests/test_verify_layout.py b/tests/test_verify_layout.py index 07f9977b..3e3cc803 100644 --- a/tests/test_verify_layout.py +++ b/tests/test_verify_layout.py @@ -274,6 +274,14 @@ class TestExtractorFailure(unittest.TestCase): send /apply chasing a phantom hole, so it has to land on the skip path. """ + def setUp(self): + super().setUp() + verify_layout.find_pdftotext.cache_clear() + + def tearDown(self): + super().tearDown() + verify_layout.find_pdftotext.cache_clear() + def test_pdftotext_without_bbox_raises_a_skippable_error(self): failure = subprocess.CalledProcessError(99, "pdftotext", stderr="Error: unknown flag") with patch("tools.verify_layout.shutil.which", return_value="/usr/bin/pdftotext"), patch( @@ -282,6 +290,127 @@ class TestExtractorFailure(unittest.TestCase): with self.assertRaisesRegex(RuntimeError, "bounding boxes"): parse_pdf(Path("cv/main_example.pdf")) + def test_first_candidate_lacks_bbox_and_second_has_it(self): + """When the first pdftotext candidate lacks -bbox (e.g. Git for Windows's xpdf) + and a subsequent candidate has it (e.g. Poppler), the tool walks PATH, probes + with -h, and runs the working candidate with its full path (#655). + """ + def fake_which(cmd, path=None): + if path == "/git/bin": + return "/git/bin/pdftotext" + if path == "/miktex/bin": + return "/miktex/bin/pdftotext" + return None + + def fake_run(cmd, *args, **kwargs): + binary = cmd[0] + if "-h" in cmd: + if binary == "/git/bin/pdftotext": + return subprocess.CompletedProcess(cmd, 99, stdout="", stderr="Usage: pdftotext ...") + if binary == "/miktex/bin/pdftotext": + return subprocess.CompletedProcess( + cmd, 99, stdout="", stderr="Usage: pdftotext ...\n -bbox : output bounding box" + ) + if "-bbox" in cmd: + return subprocess.CompletedProcess( + cmd, 0, stdout='', stderr="" + ) + raise AssertionError(f"unexpected command: {cmd}") + + with patch("tools.verify_layout.os.get_exec_path", return_value=["/git/bin", "/miktex/bin"]), patch( + "tools.verify_layout.shutil.which", side_effect=fake_which + ), patch("tools.verify_layout.subprocess.run", side_effect=fake_run) as mock_run: + pages = parse_pdf(Path("cv/main_example.pdf")) + self.assertEqual(len(pages), 1) + bbox_calls = [call for call in mock_run.call_args_list if "-bbox" in call[0][0]] + self.assertEqual(len(bbox_calls), 1) + self.assertEqual(bbox_calls[0][0][0][0], "/miktex/bin/pdftotext") + + def test_no_candidate_has_bbox_keeps_skip_path(self): + """When no candidate on PATH supports -bbox, the check skips with today's message (#655).""" + def fake_which(cmd, path=None): + if path == "/git/bin": + return "/git/bin/pdftotext" + if path == "/other/bin": + return "/other/bin/pdftotext" + return None + + failure = subprocess.CalledProcessError(99, "/git/bin/pdftotext", stderr="Error: unknown flag") + + def fake_run(cmd, *args, **kwargs): + if "-h" in cmd: + return subprocess.CompletedProcess(cmd, 99, stdout="", stderr="Usage: pdftotext (xpdf, no bbox)") + if "-bbox" in cmd: + raise failure + raise AssertionError(f"unexpected command: {cmd}") + + with patch("tools.verify_layout.os.get_exec_path", return_value=["/git/bin", "/other/bin"]), patch( + "tools.verify_layout.shutil.which", side_effect=fake_which + ), patch("tools.verify_layout.subprocess.run", side_effect=fake_run): + with self.assertRaisesRegex(RuntimeError, "bounding boxes") as ctx: + parse_pdf(Path("cv/main_example.pdf")) + message = str(ctx.exception) + self.assertIn("xpdf", message) + self.assertIn("Poppler aborted", message) + + def test_find_pdftotext_deduplicates_path_candidates(self): + """Duplicate entries or multiple PATH directories resolving to the same binary + are probed only once (#655). + """ + def fake_which(cmd, path=None): + return "/same/pdftotext" + + h_calls = [] + + def fake_run(cmd, *args, **kwargs): + if "-h" in cmd: + h_calls.append(cmd[0]) + return subprocess.CompletedProcess( + cmd, 99, stdout="", stderr="Usage: pdftotext ...\n -bbox : output bounding box" + ) + raise AssertionError(f"unexpected command: {cmd}") + + with patch("tools.verify_layout.os.get_exec_path", return_value=["/bin1", "/bin2", "/bin3"]), patch( + "tools.verify_layout.shutil.which", side_effect=fake_which + ), patch("tools.verify_layout.subprocess.run", side_effect=fake_run): + selected = verify_layout.find_pdftotext() + self.assertEqual(selected, "/same/pdftotext") + self.assertEqual(h_calls, ["/same/pdftotext"]) + + def test_find_pdftotext_caches_resolution(self): + """Resolved binary is cached at module level so repeated parse_pdf calls + avoid re-probing PATH candidates with subprocesses (#655). + """ + def fake_which(cmd, path=None): + return "/miktex/bin/pdftotext" + + h_calls = [] + bbox_calls = [] + + def fake_run(cmd, *args, **kwargs): + binary = cmd[0] + if "-h" in cmd: + h_calls.append(binary) + return subprocess.CompletedProcess( + cmd, 0, stdout="Usage: pdftotext ...\n -bbox : output bounding box", stderr="" + ) + if "-bbox" in cmd: + bbox_calls.append(binary) + return subprocess.CompletedProcess( + cmd, 0, stdout='', stderr="" + ) + raise AssertionError(f"unexpected command: {cmd}") + + with patch("tools.verify_layout.os.get_exec_path", return_value=["/miktex/bin"]), patch( + "tools.verify_layout.shutil.which", side_effect=fake_which + ), patch("tools.verify_layout.subprocess.run", side_effect=fake_run): + pages1 = parse_pdf(Path("cv/main_example.pdf")) + pages2 = parse_pdf(Path("cv/main_example.pdf")) + self.assertEqual(len(pages1), 1) + self.assertEqual(len(pages2), 1) + self.assertEqual(h_calls, ["/miktex/bin/pdftotext"]) + self.assertEqual(bbox_calls, ["/miktex/bin/pdftotext", "/miktex/bin/pdftotext"]) + def test_poppler_abort_on_empty_info_string_names_that_cause_too(self): """Poppler 26.0x before 26.05 aborts -bbox on an empty Info-dict string, e.g. the empty /Title hyperref writes when pdftitle is unset (#451). That crash is diff --git a/tools/verify_layout.py b/tools/verify_layout.py index 7bd0eb4c..728a7744 100644 --- a/tools/verify_layout.py +++ b/tools/verify_layout.py @@ -27,9 +27,9 @@ Geometry comes from Poppler word bounding boxes (`pdftotext -bbox`). Poppler is repo-wide - since #369 `verify_pdf.py` prefers pypdf and falls back to Poppler - but word bounding boxes have no pypdf equivalent, so this is the one step that still wants it. Without it, or with a broken extractor, the check reports `skipped:` and exits 2 rather -than inventing a layout failure. A broken extractor has two distinct causes: an xpdf-based -`pdftotext` (Git for Windows ships one ahead of Poppler in PATH) has no `-bbox` flag at -all, and Poppler 26.0x before 26.05 aborts `-bbox`/`-bbox-layout`/`-htmlmeta` on a PDF +than inventing a layout failure. A broken extractor has two distinct causes: no `pdftotext` +on PATH has the `-bbox` flag (Git for Windows ships an xpdf build without it, and the check +skips past it to any Poppler further down PATH), and Poppler 26.0x before 26.05 aborts `-bbox`/`-bbox-layout`/`-htmlmeta` on a PDF whose Info dictionary carries an empty string in any field - which `hyperref` writes for every field it does not set, so any `lualatex`/`pdflatex` document built with `hyperref` and no `\\hypersetup{pdftitle=...}` triggers a real Poppler crashing on a legal PDF (#451). @@ -59,6 +59,8 @@ suite needs neither Poppler nor a compiled PDF. from __future__ import annotations import argparse +import functools +import os import re import shutil import subprocess @@ -183,12 +185,53 @@ class Page: return max((tops[i + 1] - tops[i], tops[i]) for i in range(len(tops) - 1)) +@functools.cache +def find_pdftotext() -> str | None: + """Find a pdftotext binary that supports Poppler's -bbox flag. + + Git for Windows prepends Git's mingw64\\bin to PATH under Git Bash (used by + Claude Code), shipping an xpdf pdftotext without -bbox. If a working Poppler + is installed further down PATH (e.g. MiKTeX, Chocolatey, Scoop), probe each + candidate with -h and pick the first that advertises -bbox (#655). Falls back + to the first candidate if none advertises -bbox, so the caller keeps today's + skip message on failure. Returns None if no pdftotext is found at all. + + Cached at module level to avoid re-probing candidates with subprocesses on + every parse_pdf call within a process. + """ + candidates: list[str] = [] + seen: set[str] = set() + for entry in os.get_exec_path(): + candidate = shutil.which("pdftotext", path=entry) + if candidate and candidate not in seen: + seen.add(candidate) + candidates.append(candidate) + if not candidates: + return None + for candidate in candidates: + try: + res = subprocess.run( + [candidate, "-h"], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + ) + output = (res.stdout or "") + (res.stderr or "") + if "-bbox" in output: + return candidate + except (subprocess.SubprocessError, OSError): + continue + return candidates[0] + + def parse_pdf(path: Path) -> list[Page]: - if not shutil.which("pdftotext"): + binary = find_pdftotext() + if not binary: raise RuntimeError("pdftotext (Poppler) not found; install poppler-utils") try: out = subprocess.run( - ["pdftotext", "-bbox", "-enc", "UTF-8", str(path), "-"], + [binary, "-bbox", "-enc", "UTF-8", str(path), "-"], capture_output=True, text=True, # pdftotext emits UTF-8; without this Windows decodes it as cp1252 and @@ -210,8 +253,8 @@ def parse_pdf(path: Path) -> list[Page]: detail = stderr_lines[0] if stderr_lines else f"exit {exc.returncode}" raise RuntimeError( f"pdftotext could not produce bounding boxes for {path} ({detail}); " - "either the pdftotext first in PATH is an xpdf build with no -bbox flag " - "(Git for Windows ships one ahead of Poppler), or Poppler aborted on this " + "either no pdftotext on PATH has the -bbox flag (Git for Windows ships an " + "xpdf build without it), or Poppler aborted on this " "document - Poppler 26.0x before 26.05 aborts on a PDF whose Info " "dictionary carries an empty string, as hyperref writes when pdftitle is unset" ) from exc