From 892832f0855476b32721de2a90f5871d4b80673c Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 7 Oct 2026 17:52:32 -0400 Subject: [PATCH] fix(generators): make_samples builds fixtures from the shared checkout, not the tree it runs in Run as a script, a generator's sys.path[0] is examples/generators/, so pcapkit resolved to the editable install's pinned checkout instead of the tree being run. make_samples.py, options.py, dispatch.py and pcapng.py now put their own tree root first on sys.path before any pcapkit import. options.generate() also raises once its captures are written if any captured case was left out, so a stale library fails make samples instead of silently shrinking the fixture set. Closes #1343 --- examples/generators/dispatch.py | 10 ++ examples/generators/make_samples.py | 9 ++ examples/generators/options.py | 33 +++- examples/generators/pcapng.py | 6 + .../test_generators_import_their_own_tree.py | 144 ++++++++++++++++++ 5 files changed, 195 insertions(+), 7 deletions(-) create mode 100644 tests/project/test_generators_import_their_own_tree.py diff --git a/examples/generators/dispatch.py b/examples/generators/dispatch.py index 584222d0a4..06f78794c3 100644 --- a/examples/generators/dispatch.py +++ b/examples/generators/dispatch.py @@ -75,7 +75,9 @@ from __future__ import annotations import os +import pathlib import struct +import sys import tempfile from typing import TYPE_CHECKING, NamedTuple @@ -85,6 +87,14 @@ __all__ = ['cases', 'probe', 'outcomes', 'Case', 'Outcome', 'Family', 'FAMILIES', 'KNOWN_DEGRADED', 'code_name'] +#: Repository root, i.e. the grandparent of the directory holding this file. +ROOT = pathlib.Path(__file__).resolve().parents[2] + +# Resolve pcapkit from this tree rather than from the install -- see +# make_samples.py (#1343). +if str(ROOT) not in sys.path: + sys.path.insert(0, str(ROOT)) + #: Fixed capture start time, kept for parity with :mod:`examples.generators.options` #: even though nothing here is written to disk. EPOCH = 1500000000 diff --git a/examples/generators/make_samples.py b/examples/generators/make_samples.py index b74aad545a..0d2785c1cb 100644 --- a/examples/generators/make_samples.py +++ b/examples/generators/make_samples.py @@ -43,6 +43,15 @@ #: Repository root, i.e. the grandparent of the directory holding this script. ROOT = pathlib.Path(__file__).resolve().parents[2] + +# Run as a script, sys.path[0] is this directory, not the tree root, so pcapkit +# would resolve to whatever is installed -- an editable install pins one +# checkout -- rather than to this tree (#1343). PathFinder precedes the +# editable finder in sys.meta_path, so putting the root first wins. Every +# sibling that imports pcapkit repeats this, since each may run on its own. +if str(ROOT) not in sys.path: + sys.path.insert(0, str(ROOT)) + #: Directory holding this script and its sibling generator modules. HERE = pathlib.Path(__file__).resolve().parent #: Destination directory for every generated capture. diff --git a/examples/generators/options.py b/examples/generators/options.py index f19ae91beb..2fceb7751f 100644 --- a/examples/generators/options.py +++ b/examples/generators/options.py @@ -30,10 +30,11 @@ ``_read_*`` against ``_make_*``. Only the codes that construct *successfully* can appear in a capture -- there -are no octets for the ones that do not. Roughly a fifth of the space does not, -and that shortfall is not swept up: it is recorded case by case in -:file:`tests/protocols/test_option_roundtrip_unit.py`, against the defect that -causes it. +are no octets for the ones that do not. A failing case in a captured family +makes :func:`generate` raise once the rest are written, so a broken or foreign +library cannot pass for a smaller fixture set. Every other failure is recorded +case by case in :file:`tests/protocols/test_option_roundtrip_unit.py`, against +the defect that causes it. Why the case table is not a list of options ------------------------------------------- @@ -80,7 +81,7 @@ Two deliberate deviations from the sibling generators ---------------------------------------------------- -Almost every import of :mod:`pcapkit` and of :mod:`scapy` in this module is +Every import of :mod:`pcapkit` and of :mod:`scapy` in this module is inside the function that needs it, which is why :mod:`pylint`'s ``import-outside-toplevel`` is switched off for the file rather than argued with forty times. Both are load-bearing. :mod:`scapy` is needed only by @@ -111,11 +112,10 @@ import datetime import pathlib import signal +import sys import warnings from typing import TYPE_CHECKING, NamedTuple -from pcapkit.protocols.protocol import ProtocolBase - if TYPE_CHECKING: from typing import Any, Callable, Iterator, Optional @@ -123,6 +123,12 @@ #: Repository root, i.e. the grandparent of the directory holding this file. ROOT = pathlib.Path(__file__).resolve().parents[2] + +# Resolve pcapkit from this tree rather than from the install -- see +# make_samples.py (#1343). +if str(ROOT) not in sys.path: + sys.path.insert(0, str(ROOT)) + #: Default destination directory for the generated captures. SAMPLE = ROOT / 'examples' / 'captures' @@ -303,6 +309,7 @@ def handler(registry: 'Any', code: 'Any') -> 'Any': The handler registered for ``code``, or the registry's fallback. """ + from pcapkit.protocols.protocol import ProtocolBase return ProtocolBase._lookup_registry(registry, code) # pylint: disable=protected-access @@ -1859,6 +1866,10 @@ def generate(dest: 'pathlib.Path | None' = None) -> 'list[pathlib.Path]': Returns: The paths written, in the order they were written. + Raises: + RuntimeError: If any captured case was left out, after the captures + that could be built have been written. + """ from scapy.all import wrpcap # pylint: disable=no-name-in-module @@ -1903,6 +1914,14 @@ def generate(dest: 'pathlib.Path | None' = None) -> 'list[pathlib.Path]': print(f' [left out] {label}: {unreachable[label]}') for label in sorted(warned): print(f' [warned] {label}: {"; ".join(warned[label])}') + + # On a correct tree every captured case constructs and parses, so a case + # left out means the library is broken or is not this tree's (#1343). Fail + # rather than hand back a silently smaller fixture set. + if unreachable: + import pcapkit + raise RuntimeError(f'options: {len(unreachable)} case(s) left out of the captures, ' + f'built with pcapkit from {pathlib.Path(pcapkit.__file__).parent}') return written diff --git a/examples/generators/pcapng.py b/examples/generators/pcapng.py index 6860acfc5a..7ca0b44e4e 100644 --- a/examples/generators/pcapng.py +++ b/examples/generators/pcapng.py @@ -167,6 +167,12 @@ #: Repository root, i.e. the grandparent of the directory holding this script. ROOT = pathlib.Path(__file__).resolve().parents[2] + +# Resolve pcapkit from this tree rather than from the install -- see +# make_samples.py (#1343). +if str(ROOT) not in sys.path: + sys.path.insert(0, str(ROOT)) + #: Default destination directory for the fixtures. SAMPLE_DIR = ROOT / 'examples' / 'captures' #: Committed fixture the synthesised DHCP payloads are lifted from. diff --git a/tests/project/test_generators_import_their_own_tree.py b/tests/project/test_generators_import_their_own_tree.py new file mode 100644 index 0000000000..0db9364843 --- /dev/null +++ b/tests/project/test_generators_import_their_own_tree.py @@ -0,0 +1,144 @@ +# -*- coding: utf-8 -*- +"""The sample generators build from their own tree's :mod:`pcapkit`. + +GitHub issue #1343. Run as a script, a generator's ``sys.path[0]`` is +:file:`examples/generators/`, not the tree root, so ``import pcapkit`` fell +through to whatever was installed. An editable install pins one checkout, so a +worktree's generators silently built their fixtures from *that* checkout's +library: 257 captured cases instead of 266, with nothing failing. + +Each generator that imports :mod:`pcapkit` now puts its own tree root first on +:data:`sys.path`. The probe below proves it from a scratch worktree, whose path +no install points at, and with ``PYTHONPATH`` and ``PYTHONSAFEPATH`` stripped. +The probe script lives outside the tree, so -- exactly as when a generator is run +as a script -- nothing but the generator itself can put the tree on the path. + +The second half of the issue is that the shortfall was silent: +``options.generate()`` recorded each failing case and carried on. It now raises +once the captures it could build are written, which +:meth:`OptionsGenerateTests.test_left_out_case_raises` pins. + +This module is unit-tier and reads no capture. + +""" +from __future__ import annotations + +import importlib.util +import os +import pathlib +import re +import shutil +import subprocess +import sys +import tempfile +import unittest +from unittest import mock + +#: Repository root. +ROOT = pathlib.Path(__file__).resolve().parents[2] +#: Directory holding the generators. +GENERATORS = ROOT / 'examples' / 'generators' +#: A top-level or function-local import of :mod:`pcapkit`. +IMPORTS_PCAPKIT = re.compile(r'^\s*(?:from|import)\s+pcapkit\b', re.MULTILINE) + +#: Executes a generator's module body without its ``__main__`` block, then says +#: where :mod:`pcapkit` resolves from. Kept outside the tree under test. +PROBE = '''\ +import runpy, sys +runpy.run_path(sys.argv[1], run_name='_probe_1343') +import pcapkit +print(pcapkit.__file__) +''' + + +def _git(*args: 'str') -> 'subprocess.CompletedProcess[str]': + return subprocess.run(['git', '-C', str(ROOT), *args], capture_output=True, + text=True, check=False, timeout=120) + + +def _generators() -> 'list[pathlib.Path]': + """:file:`make_samples.py`, plus every sibling that imports :mod:`pcapkit`.""" + return sorted(path for path in GENERATORS.glob('*.py') + if path.name == 'make_samples.py' + or IMPORTS_PCAPKIT.search(path.read_text(encoding='utf-8'))) + + +class OwnTreeImportTests(unittest.TestCase): + """Run from a scratch worktree, every generator imports that worktree.""" + + scratch: 'pathlib.Path' + tree: 'pathlib.Path' + + @classmethod + def setUpClass(cls) -> None: + if shutil.which('git') is None: + raise unittest.SkipTest('git is not installed') + if _git('rev-parse', '--is-inside-work-tree').stdout.strip() != 'true': + raise unittest.SkipTest('not a git work tree') + + cls.scratch = pathlib.Path(tempfile.mkdtemp(prefix='pcapkit-1343-')) + cls.tree = cls.scratch / 'tree' + added = _git('worktree', 'add', '--detach', str(cls.tree), 'HEAD') + if added.returncode != 0: + shutil.rmtree(cls.scratch, ignore_errors=True) + raise unittest.SkipTest(f'git worktree add failed: {added.stderr.strip()}') + + # Test the generators as they stand in this tree, committed or not. + for path in GENERATORS.glob('*.py'): + shutil.copy2(path, cls.tree / 'examples' / 'generators' / path.name) + (cls.scratch / 'probe.py').write_text(PROBE, encoding='utf-8') + + @classmethod + def tearDownClass(cls) -> None: + # ``remove`` drops this worktree's own admin entry. No ``prune``: that + # would touch every other worktree of the repository too. + _git('worktree', 'remove', '--force', str(cls.tree)) + shutil.rmtree(cls.scratch, ignore_errors=True) + + def test_generators_resolve_pcapkit_inside_their_tree(self) -> None: + """``pcapkit.__file__`` lies in the scratch worktree, from any cwd.""" + env = {key: value for key, value in os.environ.items() + if key not in ('PYTHONPATH', 'PYTHONSAFEPATH')} + expected = (self.tree / 'pcapkit').resolve() + + generators = _generators() + self.assertIn('options.py', [path.name for path in generators]) + for path in generators: + for cwd in (self.scratch, pathlib.Path('/')): + with self.subTest(generator=path.name, cwd=str(cwd)): + result = subprocess.run( + [sys.executable, str(self.scratch / 'probe.py'), + str(self.tree / 'examples' / 'generators' / path.name)], + cwd=cwd, env=env, capture_output=True, text=True, + check=False, timeout=300) + self.assertEqual(result.returncode, 0, result.stderr) + resolved = pathlib.Path(result.stdout.strip().splitlines()[-1]).resolve() + self.assertEqual(resolved.parent, expected, + f'{path.name} imported pcapkit from {resolved}') + + +class OptionsGenerateTests(unittest.TestCase): + """``options.generate()`` fails rather than writing a smaller fixture set.""" + + def test_left_out_case_raises(self) -> None: + if importlib.util.find_spec('scapy') is None: + self.skipTest('options.generate() needs scapy') + + spec = importlib.util.spec_from_file_location( + 'pcapkit_samples_options_1343', GENERATORS / 'options.py') + assert spec is not None and spec.loader is not None + options = importlib.util.module_from_spec(spec) + spec.loader.exec_module(options) + + family = next(family for family in options.FAMILIES if family.capture is not None) + case = options.Case(family.label, 0, 'Probe', {}) + left_out = options.Outcome(case, 'CONSTRUCT', 'ValueError: probe', None, ()) + + with mock.patch.object(options, 'outcomes', lambda: iter([left_out])), \ + tempfile.TemporaryDirectory(prefix='pcapkit-1343-') as dest: + with self.assertRaisesRegex(RuntimeError, r'1 case\(s\) left out'): + options.generate(pathlib.Path(dest)) + + +if __name__ == '__main__': + unittest.main()