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()