diff --git a/.github/FUNDING.yml b/.github/FUNDING.yml index 1e0784758e..c1bc40627a 100644 --- a/.github/FUNDING.yml +++ b/.github/FUNDING.yml @@ -1,9 +1,9 @@ # Platforms behind the repository's sponsor button. # # Only the platforms actually configured are listed. GitHub's commented-out -# roster of every other platform used to sit below, but those lines were -# instructions to whoever adopted the file rather than configuration, and one of -# them (`otechie`) is no longer a platform GitHub documents. +# roster of every other platform is left out: those lines are instructions to +# whoever adopts the file rather than configuration, and one of them +# (`otechie`) is no longer a platform GitHub documents. github: jarryshaw patreon: jarryshaw diff --git a/.github/coverage.toml b/.github/coverage.toml index 1018463b82..9427f7a67e 100644 --- a/.github/coverage.toml +++ b/.github/coverage.toml @@ -15,8 +15,8 @@ # thread method on a hung test (#1387). # sigterm -- installs a SIGTERM handler that saves the data. `patch = # ["_exit"]` covers os._exit and atexit covers a clean exit, but -# nothing covered a signal, so a worker SIGTERMed during -# shutdown would leave no file -- one candidate for #1423, whose +# neither covers a signal, so without this a worker SIGTERMed +# during shutdown leaves no file -- one candidate for #1423, whose # actual cause is not established. Does not help against # SIGKILL -- execnet's post-timeout kill is `Popen.kill()` -- # which is why `tests/conftest.py` also saves at diff --git a/.github/release.yml b/.github/release.yml index 14fa2fcb04..3d45e10fd5 100644 --- a/.github/release.yml +++ b/.github/release.yml @@ -50,18 +50,18 @@ # against). # * `dependencies` sits second-to-last rather than first -- see below. # -# `dependencies` used to be the first category, and its pull requests are -# almost all bot noise, from pyup and Dependabot. Moving it down is a +# `dependencies` ranks second-to-last, and its pull requests are almost all bot +# noise, from pyup and Dependabot. Ranking it low rather than first is a # judgement call, not a data requirement: nothing in the overlaps above forces # its position relative to anything except `release` and `ci`, both of which -# already rank ahead of it regardless of where it sits. The case for moving it -# here: a future pull request that mixes a real change with a `dependencies` +# already rank ahead of it regardless of where it sits. The case for ranking it +# low: a future pull request that mixes a real change with a `dependencies` # label (say, a fix prompted by a dependency bump) surfaces under that real # category instead of being swallowed by bot noise -- which is also, # concretely, what keeps #22 ("New distribution [0.13.3.post2]", # ci+dependencies+release) filed under Release rather than Dependencies. The -# case for leaving it first, as before: it keeps a high-volume, low-interest -# category out of the way of everything else on a first read. This file takes +# case for ranking it first: it keeps a high-volume, low-interest category out +# of the way of everything else on a first read. This file takes # the former; flip its position in the list below if that judgement is wrong. # # `*` catches whatever matches none of the categories above -- nothing in the diff --git a/.github/workflows/create-release.yml b/.github/workflows/create-release.yml index 90cea497b6..d02c4a0b9f 100644 --- a/.github/workflows/create-release.yml +++ b/.github/workflows/create-release.yml @@ -44,8 +44,8 @@ concurrency: # all need ``github``: rejecting it leaves nothing tagged and nothing published. # ``TestGatedJobsDependOnGithub`` in tests/project/test_release_gates.py checks # the ``needs:`` closure. Do not add a reviewer elsewhere without re-examining -# it: a reviewer on ``conda-tag`` with ``needs: [ version_check ]`` is the bug -# #887 fixed, which tagged and committed to ``main`` before the Release was +# it: a reviewer on ``conda-tag`` with ``needs: [ version_check ]`` is #887's +# defect, which tags and commits to ``main`` before the Release is # approved. One shared ``release`` would publish PyPI and Anaconda, different # credentials and indexes, together; and PyPI cannot afford an unintended # publish, since an accepted version number cannot be reused (``skip-existing: @@ -69,8 +69,8 @@ concurrency: # publishes without naming an environment. # # #888: each publisher reads its own evidence, never "the ``v*`` tag exists" -- -# the two diverge on a half-completed release, and the next run then skipped -# every job and reported green. ``github`` still reads ``PCAPKIT_TAG_EXISTS``, +# the two diverge on a half-completed release, and a run reading the tag would +# skip every job and report green. ``github`` still reads ``PCAPKIT_TAG_EXISTS``, # since that tag is its own artefact; ``tag`` reads whether ``conda-+0`` # exists, ``pypi`` PyPI's file count, and ``conda`` Anaconda's aggregate count # (a cost saver) plus, per leg, its own distribution -- the correctness fix, @@ -133,7 +133,7 @@ jobs: # ``filter`` defaults to ``latest``: # https://docs.github.com/en/rest/actions/workflow-jobs#list-jobs-for-a-workflow-run # ``updated`` is ``'false'`` only for a ``skipped`` job; anything else it cannot - # read leaves it unset, and ``version_check`` runs as before. + # read leaves it unset, and ``version_check`` runs as it otherwise would. # # The concurrency group reads only the event, so such a run still joins # ``main``'s group and can displace a run pending there. It releases nothing @@ -602,8 +602,8 @@ jobs: # its own ``ktlo_*`` output above (test_python_matrix_tiers_1633.py). matrix: ${{ fromJSON(needs.version_check.outputs.PYTHON_PYPI) }} steps: - # #1634: this run's commit, not ``ref: v``, which was whatever - # ``main`` had become by approval; the next step checks the tag names it. + # #1634: this run's commit, not ``ref: v``, which is whatever + # ``main`` has become by approval; the next step checks the tag names it. - name: Checkout code uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -680,7 +680,7 @@ jobs: # alone, as a rebuild is not byte-identical; after a 502 an asset "may # leave an empty asset with a state of starter", which ``--clobber`` # replaces. A missing release fails ("release not found") where the action - # created one on the default branch. ``conda`` runs the same script. + # creates one on the default branch. ``conda`` runs the same script. - name: Upload package to GitHub Release id: upload if: ${{ steps.publish.outcome == 'success' }} diff --git a/.github/workflows/cron-conda.yml b/.github/workflows/cron-conda.yml index 84ce9d1345..79f920fd14 100644 --- a/.github/workflows/cron-conda.yml +++ b/.github/workflows/cron-conda.yml @@ -779,7 +779,7 @@ jobs: # ``draft``, which unset inputs make the tag, "" and false (the # ``CoreInputs`` getters ``name``, ``body``, ``createdPrerelease`` and # ``createdDraft`` in src/Inputs.ts, read through its ``updated*`` - # getters), so each bump wiped the release's name and notes. Each + # getters), so unguarded each bump wipes the release's name and notes. Each # ``omit*DuringUpdate`` leaves its field undefined (the same ``updated*`` # getters), and Octokit drops it (@octokit/endpoint's # ``removeUndefinedProperties``, called from its ``merge``, bundled in diff --git a/.github/workflows/cron-vendor.yml b/.github/workflows/cron-vendor.yml index dcc7f8d8ae..cbe96fe08a 100644 --- a/.github/workflows/cron-vendor.yml +++ b/.github/workflows/cron-vendor.yml @@ -9,7 +9,7 @@ on: schedule: - cron: '0 10 * * 6' # Saturdays at 10:00 UTC # No `push` trigger: Unit Tests' own push run already reaches a verdict, and - # calling it again with `gate-only: true` re-ran the gate on the same tree + # calling it again with `gate-only: true` would re-run the gate on the same tree # (#715). `workflow_run` also fires for a pull request's run; the # `vendor-update` job's `if` refuses those, not a `branches:` filter here. workflow_run: diff --git a/.github/workflows/experimental.yml b/.github/workflows/experimental.yml index 9ecbb4d435..505e6695b8 100644 --- a/.github/workflows/experimental.yml +++ b/.github/workflows/experimental.yml @@ -8,8 +8,8 @@ name: Experimental Tests # up nothing (#2030); it has no `workflow_call` trigger, and a job cannot # `needs:` one in another workflow. # -# Its check names, `Python (experimental)`, are the ones the legs had -# in unit-tests.yml, and `windows`'s is `Python on Windows +# Its check names, `Python (experimental)`, follow unit-tests.yml's +# `Python ` form, and `windows`'s is `Python on Windows # (experimental)` (#2060); no required context of ruleset 23497679 names one. # # The same events and `changes` job as unit-tests.yml, so a docs-only pull @@ -613,7 +613,7 @@ jobs: retention-days: 7 # #2060: the unit tier on Windows, the platform of the win-64 conda package, - # added so that its first release had a test run. One leg, on `latest`, the + # so that its releases have a test run. One leg, on `latest`, the # newest CPython the conda builds target. Non-blocking by `experimental`'s # mechanics: every step that can fail is `continue-on-error`, the long ones # have caps under the job's, `Experimental summary` reports each failure as a diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 533e8c3e7a..0adc9aa769 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -14,11 +14,11 @@ name: "Lint" # one tree disagree on it; a change in the R class alone is not evidence. # # Promotion order, cheapest first: -# * bandit -- clean since #2145: each finding was a false positive and carries +# * bandit -- clean (#2145): each finding was a false positive and carries # a justified `# nosec`. Promotable. -# * vermin -- clean since #2145: meets vermin.ini's 3.8 target. Promotable. -# * mypy -- #2145 cleared the `unused-ignore`s and the pyshark link-type -# tables; the rest is real typing work. +# * vermin -- clean (#2145): meets vermin.ini's 3.8 target. Promotable. +# * mypy -- the `unused-ignore`s and the pyshark link-type tables are clean +# (#2145); the rest is real typing work. # * pylint -- not realistically promotable: `unused-argument`, # `redefined-builtin` and `super-init-not-called` are inherent to the # schema DSL, and its metaprogramming yields `unsubscriptable-object` and @@ -73,7 +73,7 @@ jobs: python-version: ${{ steps.python-versions.outputs.latest }} # `.[all,dev]`: pylint and mypy resolve imports, and `dev` carries the - # engines #910 took out of `all`. pylint's `import-error` for `pypcap` and + # engines `all` leaves out (#910). pylint's `import-error` for `pypcap` and # `pypcapfile` (`python_version < '3.12'`) and `pcap-ct` (in no extra # installed here) is expected. - name: Install package and lint tools diff --git a/.github/workflows/python-compatibility.yml b/.github/workflows/python-compatibility.yml index 0621ee517e..64d361a528 100644 --- a/.github/workflows/python-compatibility.yml +++ b/.github/workflows/python-compatibility.yml @@ -50,7 +50,7 @@ jobs: # 23497679's required contexts. A KTLO leg is suffixed ` (KTLO)`, so no # required context can match it, and its steps carry `continue-on-error`, # as in unit-tests.yml's `test` job: job-level, a failed leg's check run - # concluded `failure`, so every commit showed red (#1672). Nothing below + # concludes `failure`, so every commit shows red (#1672). Nothing below # `requires-python`'s 3.8 runs (#2051). name: Compat Python ${{ matrix.python-version }}${{ matrix.ktlo && ' (KTLO)' || '' }} needs: [python-versions] diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 33ce04454e..6045b7150c 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -558,8 +558,8 @@ jobs: # # Scoped out (#845): per-engine integration coverage, which needs regenerated # captures and stays in aggregate in `integration` and `pypcap-parity`, and - # the multi-engine HAS_RUNTIME gate. #1538 dropped the DPKT cell, which ran - # only what `test` runs. + # the multi-engine HAS_RUNTIME gate. There is no DPKT cell (#1538): it would + # run only what `test` runs. engine-tests: name: Engines (${{ matrix.engine }}) if: ${{ inputs.gate-only != true && needs.changes.outputs.code != 'false' }} @@ -605,7 +605,7 @@ jobs: # `not-installable` ones too, so their failure is a genuine compile # failure rather than no compiler. # - # #974: an apt stall used to eat the whole job, whose cap reports + # #974: an unbounded apt stall eats the whole job, whose cap reports # `cancelled` and so fails `Required checks passed`. Not `-o # Acquire::Retries=3 -o Acquire::http::Timeout=30`, which #974 proposed: # both are noble's defaults already, and that per-`select()` timeout never @@ -613,7 +613,7 @@ jobs: # command: 60s update, 180s install (#1306), two attempts, inside the # 10-minute step cap, which stays the hang detector. # - # #1432: a slow mirror, not a dead one, then killed both attempts. So: + # #1432: a slow mirror, not a dead one, can kill both attempts. So: # # * **The .deb files are cached**, so a warm run downloads nothing. The # key is a digest of the `Inst` lines of `apt-get install -s` -- every @@ -704,7 +704,7 @@ jobs: # its extra, the others as the raw distribution, bypassing the marker # that would otherwise skip it silently (#845 finding 2). `set +e` covers # only that install; the pip upgrade and the `test` extra run under - # `set -eu`, as a broken base install once read as "expected" (#845). + # `set -eu`, as a broken base install would otherwise read as "expected" (#845). - name: Write the per-cell script run: | cat > "$RUNNER_TEMP/engine-cell.sh" <<'CELL' @@ -939,7 +939,7 @@ jobs: # only place tests/integration/test_engine_runtime.py's real # ``engine='pyshark'`` extraction is reachable. Without tshark the engine # declines, the extractor silently falls back to the default parser, and - # the test used to pass on that fallback's output. + # the test passes on that fallback's output. # # The apt steps are `engine-tests`'s, for the reasons in the note above # its plan step, with APT_SET keeping this job's cache entries apart; this @@ -1120,8 +1120,8 @@ jobs: # #1029: the protocols cell took 30.4 minutes of the 45-minute cap, so it is # split into `protocols/internet` and the rest (`protocols` with `--exclude # protocols/internet`), measured on CI at 8.2m and 7.6m. The cost: a pair of - # modules split between the two no longer shares a process, so pollution - # between them is invisible again; every pair of a protocols module followed + # modules split between the two does not share a process, so pollution + # between them is invisible; every pair of a protocols module followed # by a root-level module survives. The modules directly in tests/protocols # stay in the rest cell, as moving them would split more pairs than it joins. # @@ -1133,10 +1133,6 @@ jobs: # files, so the leg would run the root-level modules alone -- green, # while covering none of it. # - # tests/corekit was once left out as permanently red; #2115 found its - # failures were test modules mixing two pcapkit imports, fixed them, and - # added the leg. - # # The `project` cell keeps the project pins (#1954): whether a module passes # in one interpreter, in file order, with no xdist, is a question about the # module, so a pin can fail it as readily as any other. @@ -1154,7 +1150,7 @@ jobs: runs-on: ubuntu-latest # 45, as `test`'s: this is the only test job with no xdist, so a cell # carries its whole serial cost, and a contended run once passed 20 - # minutes. The slowest cell since #1029 is 8.2m; even #1029's slowdown + # minutes. The slowest cell, with #1029's split, is 8.2m; even #1029's slowdown # window, at least 1.6x, leaves it far inside the cap. timeout-minutes: 45 strategy: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3fc6621cbd..de373b6f3d 100755 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -178,16 +178,15 @@ review round. We follow a rough convention designed to answer two questions: what changed and why. The subject line carries the what, and the body of the commit describes the why. A real one from the history, -with its body abridged: +with its body abridged and its line anchors replaced by names: ``` fix(tcp): resolve the connection flags before building the options (#587) (#597) Building any MP_JOIN option raised `AttributeError: 'TCP' object has no -attribute '_flags'`. `TCP.make` built the options at tcp.py:547 and assigned -`self._flags` only at tcp.py:567, but `_make_mptcp_join` (tcp.py:2780-2786) -branches on that attribute to choose between RFC 8684 s3.2's three MP_JOIN -layouts [...] +attribute '_flags'`. `TCP.make` built the options before it assigned +`self._flags`, but `_make_mptcp_join` branches on that attribute to choose +between RFC 8684 s3.2's three MP_JOIN layouts [...] ``` The format is: diff --git a/MANIFEST.in b/MANIFEST.in index 9097b15585..7b34463669 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -17,11 +17,8 @@ prune temp prune typings # Belt-and-braces, both of them: setuptools ships these two whether or not this -# file mentions them, so neither line is load-bearing. An earlier version of this -# comment claimed the opposite -- that ``include README.md`` was "the only thing -# that puts it in an sdist" and that an sdist without it "cannot be installed at -# all" -- and both halves are false. Measured by deleting the two ``include`` -# lines below and rebuilding from a clean tree: the ``tar tzf`` listing is +# file mentions them, so neither line is load-bearing. Measured by deleting the +# two ``include`` lines below and rebuilding from a clean tree: the ``tar tzf`` listing is # identical either way, with an empty ``diff``, and the resulting sdist # installs with exit 0. ``README.md``, ``LICENSE``, ``CHANGELOG.md`` and # ``CITATION.cff`` are all still in it. @@ -34,7 +31,7 @@ prune typings # (``setuptools/_distutils/command/sdist.py``) appends the first of those # that exists to the file list unconditionally, before ``MANIFEST.in`` is read. # The ``.md`` entry is setuptools' own addition over the distutils default, so -# the README becoming Markdown is exactly what this line stopped mattering for. +# it is what ships a Markdown README without this line. # * ``Distribution._finalize_license_files`` (``setuptools/dist.py``) supplies # the default ``license_files`` glob # ``['LICEN[CS]E*', 'COPYING*', 'NOTICE*', 'AUTHORS*']``, used because @@ -48,9 +45,9 @@ prune typings # So the failure mode is real in principle; it just cannot be reached by removing # this line, because the README is in the sdist regardless. # -# Of the ``include`` lines this file originally carried, only -# ``include CHANGELOG.md`` is load-bearing: deleting it alone drops just that -# file from the listing, and nothing else changes. +# Of these two and ``include CHANGELOG.md`` below, only the last is +# load-bearing: deleting it alone drops just that file from the listing, and +# nothing else changes. # # These two are kept anyway, as an explicit statement of intent that does not # depend on a setuptools default staying where it is -- a much weaker reason than @@ -96,5 +93,5 @@ recursive-include docs/source/changelog *.rst # straight from the tree, so the conda recipe builds no sdist; its modules # reach that wheel through package discovery, and every other file under it # through the ``package-data`` entry in ``pyproject.toml`` (#1814). A line naming -# it would warn in every other build, and ``include`` of a directory matched +# it would warn in every other build, and ``include`` of a directory matches # nothing even in the conda one (#1794). diff --git a/Makefile b/Makefile index c2b2ccff3b..38d4c24260 100644 --- a/Makefile +++ b/Makefile @@ -5,12 +5,12 @@ export PIPENV_CACHE_DIR ?= $(CURDIR)/.pipenv-cache export PIP_CACHE_DIR ?= $(CURDIR)/.pip-cache export all_proxy= -# No recipe below needs a bash-only feature -- the last one, brace -# expansion in the `isort:` target, went with #765's `find`-based fix (see -# that target and tests/project/test_isort_clean.py for why: a later revision -# of that same fix leaned on brace expansion plus a bash new enough for -# `shopt -s globstar`, which is a second, version-specific way bash-only -# reliance can go wrong beyond just needing bash at all). The pin stays +# No recipe below needs a bash-only feature: the `isort:` target uses `find` +# where a recursive glob would need brace expansion (#765; see that target and +# tests/project/test_isort_clean.py for why: brace expansion plus +# `shopt -s globstar` needs a bash new enough for it, which is a second, +# version-specific way bash-only reliance can go wrong beyond just needing +# bash at all). The pin stays # anyway: one known shell to write recipes against, rather than whatever # `/bin/sh` happens to be on a given machine, is worth keeping even with # nothing bash-specific relying on it, and it costs nothing to take @@ -149,25 +149,23 @@ RUN ?= pipenv run # flags in each. Note the `pcapkit` before the flags as well as after: that is how this # recipe has always read, and vermin de-duplicates the paths (it reports the # same number of files analyzed either way), so it is preserved verbatim rather -# than tidied. `--no-parse-comments` is gone (#2145): it came with the first -# vermin recipe, with no recorded reason, and made vermin ignore the -# `# novermin` markers on compat.py's version-gated imports. The cost of parsing +# than tidied. There is no `--no-parse-comments` (#2145): it makes vermin ignore +# the `# novermin` markers on compat.py's version-gated imports. The cost of parsing # them is that a marker silences its whole line, so keep them on guarded lines. VERMIN_FLAGS = --backport argparse --backport enum --backport importlib --backport ipaddress --backport typing --backport typing_extensions --eval-annotations -vv -# `pylint.extensions.emptystring` no longer exists (#767): the check it provided, -# `compare-to-empty-string`, was folded into the core `refactoring` checker at -# pylint 3.0 as `use-implicit-booleaness-not-comparison-to-string` (C1804), with -# `compare-to-empty-string` kept only as a message alias -- no plugin load needed -# any more (confirmed: it fires the same with or without the dead plugin -# reference, so it was never actually gated by it), so the entry is dropped -# rather than the check. `old-division`, `no-absolute-import` and -# `input-builtin` are dropped because pylint has genuinely removed them. -# `eq-without-hash` is dropped for a different reason: the extension module -# that still ships that check (`pylint.extensions.eq_without_hash`, `W1641`) -# was simply never named in `--load-plugins=`, so `--disable=eq-without-hash` -# was as unrecognised as if it had been removed -- same spurious Command-line -# message, not the same cause. See .github/workflows/lint.yml's header for -# the full account. +# `--load-plugins=` names no `pylint.extensions.emptystring`, which pylint no +# longer ships (#767): the check it provided, `compare-to-empty-string`, was +# folded into the core `refactoring` checker at pylint 3.0 as +# `use-implicit-booleaness-not-comparison-to-string` (C1804), with +# `compare-to-empty-string` kept only as a message alias, so `--enable=` names +# the core check and no plugin load is needed (it fires the same with or +# without the plugin reference). `--disable=` names no `old-division`, +# `no-absolute-import` or `input-builtin`, because pylint has removed them. +# Nor does it name `eq-without-hash`, for a different reason: the extension +# module that ships that check (`pylint.extensions.eq_without_hash`, `W1641`) +# is not in `--load-plugins=`, so `--disable=eq-without-hash` would be as +# unrecognised as a removed check -- the same spurious Command-line message, +# not the same cause. PYLINT_FLAGS = --load-plugins=pylint.extensions.check_elif,pylint.extensions.docstyle,pylint.extensions.overlapping_exceptions --disable=all --enable=F,E,W,R,basic,classes,format,imports,refactoring,else_if_used,docstyle,use-implicit-booleaness-not-comparison-to-string,overlapping-except --disable=blacklisted-name,invalid-name,missing-class-docstring,missing-function-docstring,missing-module-docstring,design,too-many-lines,too-many-nested-blocks,broad-except,singleton-comparison,ungrouped-imports --max-line-length=120 --init-import=yes MYPY_FLAGS = --follow-imports=silent --ignore-missing-imports --show-column-numbers --show-error-codes BANDIT_FLAGS = -r diff --git a/conda/pypcapkit/bld.bat b/conda/pypcapkit/bld.bat index 5033d027b4..e7a478fa8e 100644 --- a/conda/pypcapkit/bld.bat +++ b/conda/pypcapkit/bld.bat @@ -4,9 +4,8 @@ SETLOCAL EnableDelayedExpansion REM build.sh's two calls, for Windows (#2060). cmd does not expand the glob in REM build.sh's `pip install conda/wheels/*`, so collect what Setup Conda REM Distribution downloaded and install it in one call, letting pip resolve the -REM files together. In a batch file a FOR variable takes two percent signs; the -REM loop this replaces had one, a syntax error, and likely why no win-64 package -REM was ever built. +REM files together. In a batch file a FOR variable takes two percent signs: with +REM one, the loop is a syntax error. SET WHEELS= FOR %%W IN (conda\wheels\*) DO SET WHEELS=!WHEELS! "%%W" IF NOT DEFINED WHEELS ( diff --git a/examples/benchmark/README.md b/examples/benchmark/README.md index dd08517283..f00f40135e 100644 --- a/examples/benchmark/README.md +++ b/examples/benchmark/README.md @@ -9,10 +9,10 @@ reStructuredText snippets: examples/benchmark/run.sh ``` -It exists because that table could not previously be completed on **any** single -machine, and because the numbers in it were not reproducible. Both problems are the -same problem -- the environment was never pinned and could not hold every engine at -once -- so the fix is to define the environment precisely and ship it. Once the +It exists because no **single** environment can complete that table, and because +numbers measured in an unpinned one are not reproducible. Both problems are the +same problem -- an environment that is not pinned, and that cannot hold every engine +at once -- so the fix is to define the environment precisely and ship it. Once the environment is a container, the number of them is a loop, which is what makes a complete table a single command rather than one hand-run measurement per version pasted together. @@ -78,7 +78,7 @@ file. **The ratio table is normalised to the default engine measured in the same virtualenv on the same pass.** An absolute timing describes the machine it was taken on at least as much as it describes the engine, so a table of absolute numbers cannot -be reproduced across machines -- which is exactly the state the old table was in. A +be reproduced across machines. A ratio divides the machine out, and pooling it across every interpreter is what makes it an answer about engines rather than about one interpreter. The observed range then includes any disagreement *between* interpreters, which is real uncertainty about the @@ -153,7 +153,7 @@ So failure is contained, and counted, at each level below: what would not be honest is discarding it silently, so the count and the first reason are reported. - **A failed pass** costs that engine that pass, not the run. The row is built from - the passes that did work, its `Passes` count shows fewer, and it carries a footnote + the passes that did work, its `Samples` count shows fewer, and it carries a footnote naming what was lost. If *every* pass failed, the engine becomes `*not measured*` with the exception as its reason. @@ -235,11 +235,11 @@ The shape comes from `examples/legacy_smoke/test_time.py` so the figures stay comparable in kind with what the project has already published: 1,000 timed extractions of `examples/captures/in.pcap` per engine, timed with `time.perf_counter_ns`, first sample discarded as a warm-up, reported as -milliseconds per packet. What changed, and why: +milliseconds per packet. Where this departs from it, and why: -| Change | Why | +| Departure | Why | | --- | --- | -| The whole set is measured `--repeats` times, not once | one pass gives no way to tell a real gap from noise, which is most of what was wrong with the old table | +| The whole set is measured `--repeats` times, not once | one pass gives no way to tell a real gap from noise | | Every Python version is measured in one run, not one per sitting | the legacy script measures whichever interpreter invokes it, so a complete table was one sitting per version, hand-assembled; a column measured last week is not comparable in absolute terms with one measured today, which is the property the per-version table depends on | | A second table reports ratios, with one absolute footnote | absolute times are not comparable across machines, and "which engine is faster" is a question about engines rather than about this host | | The driver is asserted on every extraction | the legacy script preflights once; an engine that stops being itself mid-run would go unnoticed | diff --git a/examples/benchmark/benchmark.py b/examples/benchmark/benchmark.py index d9ddbc6e81..18a1dbc70a 100644 --- a/examples/benchmark/benchmark.py +++ b/examples/benchmark/benchmark.py @@ -700,8 +700,8 @@ def run(capture: 'str', engines: 'tuple[str, ...]', rounds: 'int', repeats: 'int raise except Exception as exc: # pylint: disable=broad-except # A third-party engine gave up partway through this pass. Measured: - # `tshark` crashed on the third pass of a real run and, before this - # branch existed, took the working engines down with it. The + # `tshark` crashed on the third pass of a real run, which without this + # branch takes the working engines down with it. The # pass is lost; the run is not, and the loss is recorded rather than # quietly rounded away. failures[engine].append(f'pass {repeat + 1}: {type(exc).__name__}: {exc}') diff --git a/examples/benchmark/entrypoint.sh b/examples/benchmark/entrypoint.sh index 13724cf073..40c69e7a71 100755 --- a/examples/benchmark/entrypoint.sh +++ b/examples/benchmark/entrypoint.sh @@ -17,7 +17,7 @@ # `BENCH_MODE=report` reports on documents produced by *other* containers, handed in # through `/in`. The matrix spans several interpreters and no single one of them # measured all of it, so the reporting pass cannot be the tail of a measuring run -# the way it was when there was one image. It runs here rather than on the host +# the way it could be with one image. It runs here rather than on the host # because the host is not required to have a Python at all -- docker is the only # thing `run.sh` insists on, and report.py is standard library only, so the # interpreter it happens to run under does not affect what it says. diff --git a/examples/benchmark/report.py b/examples/benchmark/report.py index 1b403926db..fe599dd6b2 100644 --- a/examples/benchmark/report.py +++ b/examples/benchmark/report.py @@ -52,8 +52,7 @@ The snippets land in :file:`docs/source/engines.rst`, which Sphinx renders, so a Sphinx-only role would in fact survive there. Staying docutils-clean is kept deliberately anyway: it is what lets a table be pasted into any reStructuredText -a plain docutils reader will see, the project README having been one such -destination until it became Markdown. +a plain docutils reader will see. """ @@ -84,12 +83,10 @@ #: mark a row or a column that carries a note: one reference per noted row, matched to #: the ``.. [*]`` bodies emitted directly after the table, in the same order. #: -#: These used to be literal symbols with a hand-written legend, on the theory that a -#: footnote reference could collide with footnote numbers already on the page the -#: snippet is pasted into. It cannot: docutils assigns auto-symbols and auto-numbers -#: page-wide, in document order, so a snippet's footnotes simply take the next free -#: symbols and numbers wherever they land -- and :file:`docs/source/engines.rst` used -#: footnotes for these tables before this module did. +#: A footnote reference cannot collide with footnote numbers already on the page the +#: snippet is pasted into: docutils assigns auto-symbols and auto-numbers page-wide, +#: in document order, so a snippet's footnotes simply take the next free symbols and +#: numbers wherever they land. FOOTNOTE = '[*]_' #: Label of the one footnote every overlapping row shares. Auto-numbered and labelled @@ -762,8 +759,8 @@ def line(cells: 'Sequence[str]', align_numeric: 'bool') -> 'str': return ' '.join(padded).rstrip() rule = ' '.join('=' * width for width in widths) - # The header stays left-aligned even in a numeric table, matching the hand-written - # table this one replaces: a right-aligned ``3.10`` sits over the tail of its + # The header stays left-aligned even in a numeric table, matching the table in + # docs/source/engines.rst: a right-aligned ``3.10`` sits over the tail of its # column and reads as though it belonged to the column before it. lines = [rule, line(headers, False), rule] lines.extend(line(row, numeric) for row in body) @@ -848,10 +845,8 @@ def _provenance(documents: 'Sequence[dict[str, Any]]', image: 'Optional[str]', f"SHA-256 ``{capture['sha256'][:16]}...``")) # Every engine on every interpreter should see the same number of frames in the same # capture, so this row is normally one number. A disagreement is reported rather than - # dropped: with one interpreter it was barely possible and omitting it cost nothing, - # but two CPython versions extracting different counts from one file would be the - # most interesting thing in the report, and the previous behaviour was to hide - # exactly that by printing no row at all. + # dropped: two CPython versions extracting different counts from one file would be the + # most interesting thing in the report, and printing no row would hide exactly that. packets = {entry['packets'] for document in ordered for entry in document['results'] if entry['packets']} if len(packets) == 1: @@ -875,9 +870,8 @@ def _provenance(documents: 'Sequence[dict[str, Any]]', image: 'Optional[str]', # column are counting the same unit. They are not the same *number*, and cannot be: # a pass happens once per environment, so the table's count is this figure times the # number of environments. - # The column was called "Passes" while there were two environments and one - # interpreter, where the difference was easy to overlook; across a matrix it reads as - # a contradiction, hence the rename. + # Hence "Samples" for the column rather than "Passes": across a matrix, one name for + # two different numbers reads as a contradiction. environments = ', '.join(f"``{document['environment']}``" for document in ordered) noun = 'environment' if len(ordered) == 1 else 'environments' pairs.append(('Passes', f"{first['repeats']} over the whole engine set, " @@ -1089,7 +1083,7 @@ def render_versions_rst(rows: 'Sequence[Row]', documents: 'Sequence[dict[str, An lines.extend(_simple_table(headers, body, numeric=True)) lines.append('') - # The owner's own sentence about the old hand-maintained table, kept word for word: + # The owner's own sentence about the hand-maintained table, kept word for word: # it is the right warning, and a regenerated table that rewords it would show up as # a diff in the prose every time the numbers changed. It sits between the table and # the footnotes, where docs/source/engines.rst has it. @@ -1337,9 +1331,9 @@ def render_text(rows: 'Sequence[Row]', documents: 'Sequence[dict[str, Any]]', lines.append('') # No per-version failure lines here. The provenance block above already names # every version the run could not measure and why, in the same output a few - # lines up, and a second rendering of the same fact managed to disagree with the - # first: it called a partly measured version "not measured" while its figures - # were printed in the grid directly above. + # lines up, and a second rendering of the same fact can disagree with the first, + # calling a partly measured version "not measured" while its figures are printed + # in the grid directly above. lines.append(f'{"engine":14} {"relative":>10} {"range":>19} {"ms/packet":>12} samples') lines.append('-' * 78) diff --git a/examples/benchmark/run.sh b/examples/benchmark/run.sh index 9fd02b9742..b7e62728d6 100755 --- a/examples/benchmark/run.sh +++ b/examples/benchmark/run.sh @@ -451,7 +451,7 @@ docker start --attach "${REPORTER}" || status=$? docker cp "${REPORTER}:/out/." "${OUT_DIR}/" >/dev/null 2>&1 || status=1 # What actually landed is checked rather than announced. `docker cp` of an empty -# directory succeeds, so an earlier version of this reported "wrote ... table.rst" +# directory succeeds, so an announcement would report "wrote ... table.rst" # after a run that crashed before writing anything -- which is worse than saying # nothing, because it sends the reader looking for a file that is not there. echo >&2 diff --git a/examples/benchmark/test_harness.py b/examples/benchmark/test_harness.py index 590371bab1..c03f218900 100644 --- a/examples/benchmark/test_harness.py +++ b/examples/benchmark/test_harness.py @@ -1170,10 +1170,9 @@ def test_pooling_across_interpreters_is_declared(self): def test_a_packet_count_disagreement_is_reported_not_dropped(self): """One capture must yield one frame count on every interpreter. - If it does not, that is the most important thing in the report -- and the - previous behaviour was to print no row at all, which hid precisely the case the - row exists to establish. With one interpreter the disagreement was barely - possible; across a matrix it is a real failure mode. + If it does not, that is the most important thing in the report -- and printing + no row at all would hide precisely the case the row exists to establish, which + across a matrix is a real failure mode. """ docs = matrix(versions=('3.11', '3.12')) @@ -1196,8 +1195,8 @@ def test_the_sample_column_is_not_called_passes(self): """A pass happens once per environment, so samples outnumber passes. The provenance block says "3 passes ... in 7 environments" and the table's own - count is the product of the two. Calling both "Passes" read as a contradiction - as soon as there was more than one interpreter. + count is the product of the two. Calling both "Passes" would read as a + contradiction once there is more than one interpreter. """ docs = matrix(versions=('3.11', '3.12')) @@ -1305,12 +1304,12 @@ class TestHostileReasons: or the tail of a pip or gcc log that the Dockerfile recorded because a `pypcap` build was allowed to fail. All of it is interpolated into emitted markup as prose, and the suite's other fixtures are all well-behaved English -- which is exactly why - this went unnoticed until a hostile string was tried. + it takes a hostile string to show the failure. """ #: Strings a compiler or a Python traceback produces without trying, each of which - #: broke the emitted snippet under plain docutils before the escaping went in. + #: breaks the emitted snippet under plain docutils without the escaping. HOSTILE = ( # `**kwargs` in a gcc diagnostic: inline strong start-string without end-string. 'gcc: error: **kwargs handling broke the build', @@ -1394,8 +1393,8 @@ def test_the_plain_text_report_is_not_escaped(self): def test_the_emulation_note_is_escaped_too(self): """`--emulated` is outside text as much as any reason is. - `run.sh` composes that sentence itself, which makes it safe in practice and is - exactly why it was the last channel left unescaped. It still arrives on the + `run.sh` composes that sentence itself, which makes it safe in practice and + exactly the channel easiest to leave unescaped. It still arrives on the command line, and it reaches markup in two places -- a provenance row and a bold paragraph -- so a value with a stray ``*`` in it would break the snippet from a direction nobody was watching. @@ -1429,8 +1428,8 @@ def test_the_emulation_note_is_escaped_too(self): class TestFootnotes: """Row and column notes are reStructuredText footnotes, as docs/source/engines.rst has them. - The tables used to mark rows with literal symbols and explain them in a hand-written - legend, which had to be converted by hand before the snippet could go on the page. + Literal symbols explained in a hand-written legend would have to be converted by + hand before the snippet could go on the page. Auto-symbol footnotes pair with their references by order alone, so the order the bodies come out in is checked against what docutils actually resolves, not against a count. @@ -1506,7 +1505,7 @@ def test_every_reference_resolves_to_its_own_note(self): def test_they_paste_beside_the_page_footnotes(self): """docutils numbers footnotes page-wide, so nothing on the page collides. - This is the case the literal markers were chosen to avoid, and it does not + This is the collision literal markers would be chosen to avoid, and it does not arise: the page's own notes take the first symbol and number, the snippets' take the next, and every reference still reaches its own body. @@ -1566,7 +1565,7 @@ def test_the_bodies_follow_their_table(self): def test_a_gap_nothing_explained_still_has_a_body(self): """An engine missing from a version's documents altogether is still a gap. - A literal marker with no legend line was merely unhelpful; a footnote reference + A literal marker with no legend line would be merely unhelpful; a footnote reference with no body is a docutils error on the page. """ @@ -1635,8 +1634,8 @@ def test_four_decimal_places_whatever_the_magnitude(self, value, expected): Significant figures -- which every other number in the report uses -- would give ``0.01700`` and ``14.74`` different numbers of decimal places in the same - column. Four places is also what the hand-maintained table in the docs has - always used, so a regenerated table diffs its numbers rather than its layout. + column. Four places is also what the hand-maintained table in the docs uses, + so a regenerated table diffs its numbers rather than its layout. """ assert report._fixed(value) == expected # pylint: disable=protected-access @@ -1797,8 +1796,9 @@ class TestLoaderFailure: """A ``pcap`` module that will not load is a reason, not a fatal error (#2129). On macOS and Windows, ``pcap-ct``'s libpcap loader raises :exc:`OSError` with a - message that does not mention libpcap. ``pypcap``'s preflight let it escape, and - the whole ``pcap_ct`` environment exited before measuring anything. + message that does not mention libpcap. Unless the harness asks the import itself, + ``pypcap``'s preflight lets it escape, and the whole ``pcap_ct`` environment exits + before measuring anything. """ @@ -1848,8 +1848,8 @@ def test_an_oserror_reading_the_capture_is_still_fatal(self, harness, monkeypatc tmp_path): """Only the import is in scope: a ``pcap`` module that loads, then fails, re-raises. - The message mentions libpcap on purpose. Matching on that word is what the - harness did before, and it would have swallowed this real failure. + The message mentions libpcap on purpose: matching on that word would swallow + this real failure. """ monkeypatch.syspath_prepend(fake_pcap(tmp_path / 'site', diff --git a/examples/generators/dispatch.py b/examples/generators/dispatch.py index a1ecae3db7..281860f4a4 100644 --- a/examples/generators/dispatch.py +++ b/examples/generators/dispatch.py @@ -3,7 +3,7 @@ :file:`pcapkit/foundation/registry/protocols.py` documents the registries that decide *which* :class:`~pcapkit.protocols.protocol.Protocol` subclass parses the -next layer. Nothing enumerated them: several +next layer. No other module enumerates them: several test modules reference ``__proto__``, and :file:`tests/protocols/test_registry_runtime.py` checks that a table entry *resolves* to the right class object, but that is a weaker property than the one @@ -37,15 +37,14 @@ :class:`~pcapkit.protocols.misc.raw.Raw` itself is not a defect. The expected class is deliberately **not** read out of the registry under -test. An earlier version of this module did exactly that -(``target=_resolve(entry)``), which made the check self-referential: dispatch -always "reached" whatever the table currently said, because that was the very -value being checked against. Deleting an entry was still caught, by the exact -per-family counts in +test. Read from there (``target=_resolve(entry)``), the check would be +self-referential: dispatch would always "reach" whatever the table said, +because that is the very value being checked against. Deleting an entry would +still be caught, by the exact per-family counts in :meth:`~tests.protocols.test_dispatch_registry_unit.DispatchRegistryTests .test_cases_cover_every_table_named_in_the_issue`, but *retargeting* one -- -entry present, registry size unchanged, class wrong -- passed silently, since -nothing independent of the table said what the table should have held. +entry present, registry size unchanged, class wrong -- would pass silently, +since nothing independent of the table would say what the table should hold. :data:`PINNED_TARGETS` is that independent record, and :attr:`Case.registered` keeps the registry's own current answer alongside it purely as a diagnostic, so a mismatch between the two is reported as what it is rather than folded @@ -100,14 +99,13 @@ #: Only ``dstport``/``payload`` vary. #: #: Every key is a parameter :meth:`TCP.make ` -#: declares, which is not a style point: this mapping used to read ``seq=1``, -#: ``ack=0``, ``ack_flag=False`` and ``urgent_pointer=0``, of which only ``ack`` -#: was a parameter at all -- and it is the acknowledgement *flag*, so the -#: acknowledgement number was never set while the flag was. The other three were -#: absorbed by ``make``'s trailing ``**kwargs`` and discarded, so every segment -#: built here carried sequence number ``0`` however plainly this said ``1``. That -#: is the same defect as #602, and since #617 an undeclared keyword raises -#: :exc:`~pcapkit.utilities.exceptions.UnsupportedCall` rather than going quiet. +#: declares, which is not a style point: ``ack`` is the acknowledgement *flag*, +#: not the number, and ``seq``, ``ack_flag`` and ``urgent_pointer`` are not +#: parameters at all. An undeclared keyword raises +#: :exc:`~pcapkit.utilities.exceptions.UnsupportedCall` (#617); without that +#: check, ``make``'s trailing ``**kwargs`` absorbs and discards it, and a segment +#: built from such a mapping carries sequence number ``0`` whatever it says +#: (#602). _TCP_BASE = dict(srcport=50000, seq_no=1, ack_no=0, ns=False, cwr=False, ece=False, urg=False, ack=False, psh=False, rst=False, syn=True, fin=False, window=8192, checksum=b'\x00\x00', urgent=0) @@ -313,11 +311,9 @@ def _link_payload(code: 'int') -> 'bytes': return _ip4(6, _tcp(9999)) if code == EtherType.Internet_Protocol_version_6: return _ip6(6, _tcp(9999)) - if code == 0x8137: # Novell IPX -- see KNOWN_DEGRADED. - # ``IPX()`` cannot even be constructed with no arguments (#492: - # ``Socket(0)`` is not a valid member), so there is no well-formed IPX - # payload to probe with; this is a deliberately IPX-shaped run of zero - # octets, enough to dispatch to the class and watch it fail to parse. + if code == 0x8137: # Novell IPX. + # A run of zero octets the length of an IPX header: dispatch into + # IPX is all this probes. return b'\x00' * 30 raise LookupError(f'link: no payload builder for {code!r}') # pragma: no cover @@ -378,7 +374,7 @@ def _internet_payload(code: 'int') -> 'bytes': if code == TransType.IPv6_Opts: from pcapkit.protocols.internet.ipv6_opts import IPv6_Opts return bytes(IPv6_Opts(options=[], next=6, payload=_tcp(9999))) - if code == TransType.IPX_in_IP: # See KNOWN_DEGRADED -- same #492 as Link's IPX. + if code == TransType.IPX_in_IP: # As for Link's IPX. return b'\x00' * 30 if code == TransType.SCTP: from pcapkit.const.sctp.chunk import Chunk @@ -394,12 +390,10 @@ def _internet_payload(code: 'int') -> 'bytes': from pcapkit.const.hip.parameter import Parameter from pcapkit.protocols.internet.hip import HIP - # Two copies of one parameter, not one -- HIP.make's header length - # arithmetic loses four octets for a lone parameter and the packet is - # then rejected on the way back in; see HIP_COPIES in - # examples/generators/options.py for the full explanation. Only the - # dispatch into HIP is under test here, so which parameter travels - # inside it does not matter. + # Two copies of one parameter, though one round-trips as well (see + # HIP_COPIES in examples/generators/options.py). Only the dispatch + # into HIP is under test here, so which parameter travels inside it, + # and how many, does not matter. return bytes(HIP(parameters=[(Parameter.SEQ, {})] * 2, extension=True, next=6, packet=1, version=2, checksum=b'\x00\x00', controls_anonymous=False, shit=0, rhit=0, payload=_tcp(9999))) @@ -615,19 +609,18 @@ def _internet_enum() -> 'Any': #: The class each case is *supposed* to dispatch to, pinned independently of #: whatever the live registry currently holds. #: -#: This is the fix for a gap in the first version of this harness: with the -#: expected target read straight out of the table under test -#: (``target=_resolve(entry)``), a *mis-pointed* entry passed silently -- -#: dispatch always "reached" whatever the corrupted table said, because that -#: was the very value being checked against. Deleting an entry was still -#: caught, by the count guard in +#: With the expected target read straight out of the table under test +#: (``target=_resolve(entry)``), a *mis-pointed* entry would pass silently -- +#: dispatch would always "reach" whatever the corrupted table said, because +#: that is the very value being checked against. Deleting an entry would still +#: be caught, by the count guard in #: :meth:`~tests.protocols.test_dispatch_registry_unit.DispatchRegistryTests #: .test_cases_cover_every_table_named_in_the_issue`, but retargeting one -- -#: entry present, registry size unchanged, class wrong -- was not caught by -#: anything. Recording the expected ``(module, name)`` here, by hand, the same -#: way :data:`KNOWN_DEGRADED` records defects by hand, means a table entry -#: that no longer says what this module expects is a mismatch against a fixed -#: point rather than against itself. +#: entry present, registry size unchanged, class wrong -- would not be caught +#: by anything. Recording the expected ``(module, name)`` here, by hand, the +#: same way :data:`KNOWN_DEGRADED` records degraded cases by hand, means a +#: table entry that does not say what this module expects is a mismatch +#: against a fixed point rather than against itself. #: #: Keyed by :attr:`Case.label`. A code whose label is missing here builds a #: :class:`Case` with ``target=None`` -- see @@ -663,18 +656,16 @@ def _internet_enum() -> 'Any': 'internet/HIP': ('pcapkit.protocols.internet.hip', 'HIP'), 'internet/SCTP': ('pcapkit.protocols.transport.sctp', 'SCTP'), 'internet/OSPFIGP': ('pcapkit.protocols.application.ospf', 'OSPF'), - # #904: Shim6 (140) previously had no entry at all, and the default - # factory made it resolve to Raw. It is registered directly at - # IPv6_Ext, which parses the RFC 6564 §4 generic layout it has - # never had a dedicated dissector for -- a deliberate addition, not a - # regression. + # #904: Shim6 (140) is registered directly at IPv6_Ext, which parses + # the RFC 6564 §4 generic layout, as Shim6 has no dedicated dissector. + # Without the entry the default factory would resolve it to Raw. 'internet/Shim6': ('pcapkit.protocols.internet.ipv6_ext', 'IPv6_Ext'), # -- TCP.__proto__ (port) -------------------------------------------------- 'tcp/20': ('pcapkit.protocols.application.ftp', 'FTP_DATA'), 'tcp/21': ('pcapkit.protocols.application.ftp', 'FTP'), - # Repointed from ``httpv1`` to the version-identifying proxy by #682, which - # is what makes these two agree with ``udp/80`` and ``udp/8080`` below. + # The version-identifying proxy (#682), as for ``udp/80`` and ``udp/8080`` + # below. 'tcp/80': ('pcapkit.protocols.application.http', 'HTTP'), 'tcp/8080': ('pcapkit.protocols.application.http', 'HTTP'), @@ -849,7 +840,7 @@ def probe(case: 'Case') -> 'Outcome': The check is against :attr:`Case.target` -- :data:`PINNED_TARGETS`, recorded independently of the registry -- not against :attr:`Case.registered`, which is only ever what the table under test - currently claims. Checking a table against itself is what let a + currently claims. Checking a table against itself would let a mis-pointed entry pass silently; see :data:`PINNED_TARGETS`. Args: @@ -896,15 +887,18 @@ def outcomes(families: 'Optional[tuple[Family, ...]]' = None) -> 'list[Outcome]' return [probe(case) for case in cases(families)] -#: Codes known to degrade to :class:`~pcapkit.protocols.misc.raw.Raw`, -#: with the defect that causes it. Recorded, not fixed -- GitHub issue #496 is -#: a test-harness change and none of these classes is this module's to touch. +#: Codes known to degrade to :class:`~pcapkit.protocols.misc.raw.Raw`, each +#: with the ``'error'`` fragments it may report and the reason it degrades. +#: Recorded, not fixed -- GitHub issue #496 is a test-harness change and none +#: of these classes is this module's to touch. #: -#: Each entry is checked both ways, the same as -#: :data:`examples.generators.options.EXPECTED_FAILURES`: a case listed here -#: must still fail to reach its target, or the entry is stale and the defect it -#: names has been fixed; a case *not* listed here must reach its target, or a -#: fresh regression has gone unnoticed. +#: This copy is what the ``__main__`` summary counts. The checks run against +#: the copy :class:`~tests.protocols.test_dispatch_registry_unit.DispatchRegistryTests` +#: keeps of it, both ways, as for +#: :data:`examples.generators.options.EXPECTED_FAILURES`: a case listed there +#: must still fail to reach its target, or the entry is stale; a case *not* +#: listed there must reach its target, or a fresh regression has gone +#: unnoticed. Nothing holds the two copies to each other. KNOWN_DEGRADED = { # NGAP is registered for both of its PPIDs. Building a well-formed # PER-encoded NGAP-PDU is out of scope for a minimal dispatch probe -- @@ -916,7 +910,7 @@ def outcomes(families: 'Optional[tuple[Family, ...]]' = None) -> 'list[Outcome]' # a malformed PDU, and where it is not, NGAP reports the missing # dependency instead. Both are the same placeholder-payload limitation, so # the entry accepts either -- CI installs ``.[test]``, which deliberately - # excludes ``[NGAP]``, and pinning only the first reason failed there. + # excludes ``[NGAP]``, so pinning only the first reason would fail there. 'sctp/PayloadProtocolIdentifier_3GPP_NG_Application_Protocol': ( ('malformed NGAP-PDU', 'needs the optional "pycrate" dependency'), 'pcapkit/protocols/application/ngap.py -- b\'ngap-pdu\' is not an ' diff --git a/examples/generators/endian.py b/examples/generators/endian.py index 3196d70f9a..4529a41775 100644 --- a/examples/generators/endian.py +++ b/examples/generators/endian.py @@ -4,24 +4,22 @@ Classic PCAP declares its own byte order in the magic number of the global header, and every 32-bit field after that -- the global header's own, and the four in each record header -- is written in *that* order rather than in the -host's. A reader therefore carries a branch for it, and until this module there -was no ``.pcap`` fixture in the repository to take the big-endian side of that -branch: every capture under ``examples/captures/`` was little-endian, so on a -little-endian runner the branch was never taken and reading the host's order -instead of the file's produced the right answer anyway. - -That is how GitHub issue #605 survived: ``Frame.unpack`` seeded the byte order -under the key ``bytesorder`` where ``byteorder_callback`` -- then in -:file:`pcapkit/protocols/schema/misc/pcap/frame.py`, since moved to -:file:`pcapkit/corekit/fields/byteorder.py` -- reads ``byteorder``, so the -lookup always missed and always fell back to :data:`sys.byteorder`. Reading a -big-endian capture then byte-swapped ``ts_sec``, ``ts_usec``, ``incl_len`` and +host's. A reader therefore carries a branch for it, and only a big-endian +fixture takes the big-endian side of that branch: with little-endian captures +alone, a little-endian runner never takes it, and reading the host's order +instead of the file's produces the right answer anyway. + +That is how a defect such as GitHub issue #605 goes unseen: with ``Frame.unpack`` seeding the +byte order under the key ``bytesorder`` where ``byteorder_callback`` +(:file:`pcapkit/corekit/fields/byteorder.py`) reads ``byteorder``, the lookup +always misses and always falls back to :data:`sys.byteorder`. Reading a +big-endian capture then byte-swaps ``ts_sec``, ``ts_usec``, ``incl_len`` and ``orig_len`` -- and because ``incl_len`` is the payload length, the first record -swallowed the rest of the file and the read that followed was handed a negative +swallows the rest of the file and the read that follows is handed a negative payload length, raising ``ValueError: read length must be non-negative or -1`` out of -:file:`pcapkit/protocols/schema/schema.py`. A one-character fix, with nothing in -the suite able to tell whether it worked. +:file:`pcapkit/protocols/schema/schema.py`. The fix is one character, and +nothing but these fixtures can tell whether it works. =============================== ========================================== Fixture Exercises diff --git a/examples/generators/legacy.py b/examples/generators/legacy.py index 3a5b940f70..03560f7eb1 100644 --- a/examples/generators/legacy.py +++ b/examples/generators/legacy.py @@ -72,22 +72,21 @@ oversight and is not: neither fixture contains a datagram that reassembles *incompletely*, even though ``test_reassembly.py`` and ``test_analyse.py`` both have a branch for one -- a payload that is a tuple of received fragments, and a -``packet`` of :data:`None`. Nothing prevents that branch any more; it is simply +``packet`` of :data:`None`. Nothing prevents that branch; it is simply that every stream in these two captures arrives whole. Each direction here is sent in full and every segment eventually delivered, some of them out of order and one retransmitted, so the holes that open all close again before the FIN or RST that submits the buffer. -It used to be that no capture could reach that branch at all, which is how the -absence started. ``submit`` sliced the payload buffer with the bounds of each -hole, but those bounds were absolute TCP sequence numbers -(``first=tcp_info.seq`` in ``pcapkit/toolkit/pcap.py``) while the buffer is -indexed from the start of the direction's data, so with any real initial -sequence number every slice started far beyond the end of the buffer, every -fragment came out empty, and ``if data:`` discarded the datagram without a word. -That is fixed -- ``submit`` converts each hole's absolute bounds into -offsets into the buffer it is reading, and decides completeness from that -payload buffer's own gap list -- and GitHub issue #349 records the whole of it. +``submit`` converts each hole's absolute bounds -- TCP sequence numbers, +``first=tcp_info.seq`` in ``pcapkit/toolkit/pcap.py`` -- into offsets into the +buffer it is reading, which is indexed from the start of the direction's data, +and decides completeness from that payload buffer's own gap list (GitHub issue +#349). Were it to slice with the absolute bounds instead, every slice would +start far beyond the end of the buffer for any real initial sequence number, +every fragment would come out empty, and ``if data:`` would discard the +datagram without a word, so no capture could reach the incomplete branch at +all. Regenerating these two fixtures to carry a permanently lost segment as well was considered and rejected: they are pinned byte-for-byte by the test suite, and a stream with a hole in it is cheaper to build segment by segment than to read out diff --git a/examples/generators/options.py b/examples/generators/options.py index 2c31b186c8..768172f8f8 100644 --- a/examples/generators/options.py +++ b/examples/generators/options.py @@ -106,8 +106,8 @@ every second until :func:`roundtrip` has the :exc:`TimeoutError`, because one raise can be lost: a finaliser the cyclic collector runs reports it to :func:`sys.unraisablehook` and drops it, and an ``except OSError`` drops it too, -:exc:`TimeoutError` being one. A single alarm then left the case running for good -(#1889). +:exc:`TimeoutError` being one. A single alarm would then leave the case running +for good (#1889). """ @@ -163,7 +163,7 @@ #: (:rfc:`2675#section-2`). See :func:`_jumbogram_datagram`. JUMBO_LEN = 0x10000 #: Snapshot length of a capture whose frames all fit in it: scapy's default, -#: which keeps every capture but a jumbogram's as it always was. +#: which every capture but a jumbogram's carries. SNAPLEN = 0xFFFF #: Snapshot length of a capture holding a longer frame: libpcap's #: ``MAXIMUM_SNAPLEN``. A libpcap reader cuts a record to the file's snaplen, so @@ -371,14 +371,13 @@ def _named_registry(owner: 'type', enum: 'Any', prefix: 'str', attribute: 'str') -> 'Any': """A protocol's dispatch registry, or a stand-in derived from its handlers. - ``IPv4`` and ``HIP`` were the last two protocols to dispatch their options - and parameters by attribute name rather than through a registry, and for - those there was nothing to enumerate. The equivalent source of truth is the - enumeration crossed with the presence of the handler, which is what this - builds when the registry is absent. + A protocol that dispatches its options or parameters by attribute name + rather than through a registry has nothing to enumerate. The equivalent + source of truth is the enumeration crossed with the presence of the + handler, which is what this builds when the registry is absent. - Both of them have a real registry (#429), so the fallback is not taken - for either -- it is kept because it costs nothing and is + ``IPv4`` and ``HIP``, read through here, both have a real registry (#429), + so the fallback is not taken for either -- it is kept because it costs nothing and is the only thing that would notice a protocol added tomorrow without one. Both shapes are a :class:`collections.defaultdict`, so :func:`cases` and :func:`handler` cannot tell which one they were given -- and in particular @@ -418,24 +417,23 @@ def _named_registry(owner: 'type', enum: 'Any', prefix: 'str', #: option and nothing else. #: #: Every key here has to be spelled the way :meth:`TCP.make -#: ` declares it, because ``make`` -#: takes ``**kwargs`` and *silently drops* whatever it does not declare -- no -#: ``UnknownFieldWarning``, no ``TypeError``, nothing. GitHub issue #602 is -#: the keys below, spelled wrong and therefore ignored: +#: ` declares it: an undeclared +#: keyword raises :exc:`~pcapkit.utilities.exceptions.UnsupportedCall` (#617). +#: Without that check, ``make``'s ``**kwargs`` drops it silently, and GitHub +#: issue #602 is this table's keys spelled wrong and so ignored: #: -#: * ``'seq'`` is ``seq_no``. The mapping read as sequence number 1 and every -#: generated frame carried 0, which is what made the defect worth an issue: -#: the captures were not the packets this table describes. -#: * ``'urgent_pointer'`` is ``urgent``. Harmless in effect, since the value -#: asked for and the default that was used are both 0. -#: * ``'ack_flag'`` is ``ack``, and ``ack`` is the *acknowledgement flag* -#: rather than the acknowledgement number, which is ``ack_no``. So the two -#: were the wrong way round: ``'ack': 0`` set the flag (to a falsy 0) and -#: ``'ack_flag': False`` set nothing at all. +#: * ``'seq'`` for ``seq_no``. The mapping reads as sequence number 1 and +#: every generated frame carries 0, so the captures are not the packets this +#: table describes. +#: * ``'urgent_pointer'`` for ``urgent``. Harmless in effect, since the value +#: asked for and the default are both 0. +#: * ``'ack_flag'`` for ``ack``, where ``ack`` is the *acknowledgement flag* +#: rather than the acknowledgement number, which is ``ack_no``: ``'ack': 0`` +#: sets the flag (to a falsy 0) and ``'ack_flag': False`` sets nothing at +#: all. #: -#: The values are unchanged from what the mapping always claimed to mean: a -#: SYN-only segment with sequence number 1, every other flag clear, and the -#: acknowledgement number, checksum and urgent pointer at zero. +#: The mapping is a SYN-only segment with sequence number 1, every other flag +#: clear, and the acknowledgement number, checksum and urgent pointer at zero. TCP_BASE = { 'srcport': 50000, 'dstport': 80, 'seq_no': 1, 'ack_no': 0, 'ns': False, 'cwr': False, 'ece': False, 'urg': False, 'ack': False, @@ -531,11 +529,10 @@ def _ipv4_overrides() -> 'dict[Any, dict[str, Any]]': from pcapkit.const.ipv4.protection_authority import ProtectionAuthority return { # Two authorities rather than one because two bits set in the bitmap say - # more than one does, not because one is unrepresentable: it used to be, - # a single ``GENSER`` (value 0) making ``_make_opt_sec`` size a - # zero-octet bitmap and then index into it for a bare ``IndexError``, and - # #537 fixed that arithmetic. So this is a coverage choice and routes - # around nothing. + # more than one does, not because one is unrepresentable. Without + # #537's fix, a single ``GENSER`` (value 0) makes ``_make_opt_sec`` size + # a zero-octet bitmap and then index into it for a bare ``IndexError``. + # So this is a coverage choice and routes around nothing. OptionNumber.SEC: {'authorities': [ProtectionAuthority.GENSER, ProtectionAuthority.NSA]}, # ``counts=10``, the default, needs 43 option octets, which overflows @@ -1007,9 +1004,10 @@ def _mh_extension_build(code: 'Any', kwargs: 'dict[str, Any]') -> 'Any': #: How many copies of the parameter under test go in one packet. #: -#: One, since #689. What follows is why it was two, and why it is not. +#: One (#689). What follows records why a pair is not needed, and what a pair +#: would hide. #: -#: It was two because of the defect #651 fixed. ``HIP.make`` computes the +#: A pair hides the defect #651 fixed. ``HIP.make`` computes the #: header's ``len`` field as ``total_length // 8 + 4``, which is lossless only #: when the parameter octets are a multiple of eight; and every padding site in #: the two HIP modules aligned the *contents* to eight, ignoring the four-octet @@ -1026,7 +1024,7 @@ def _mh_extension_build(code: 'Any', kwargs: 'dict[str, Any]') -> 'Any': #: What kept the pair afterwards was a short list of codes whose own packed #: length disagreed with the ``len`` they declared, so their record was not #: 8-aligned however the padding was computed, and a pair cancelled that -#: misalignment the same way the pre-#651 padding rule once did. #672 and #679 +#: misalignment the same way it cancelled #651's padding defect. #672 and #679 #: closed two of those -- ``R1_COUNTER`` (129), whose 8-octet :rfc:`7401` #: Section 5.2.3 counter had packed as four, and ``LOCATOR_SET`` (193), whose #: ``Length`` had been counted in 4-octet units where the RFC counts bytes -- @@ -1042,7 +1040,7 @@ def _mh_extension_build(code: 'Any', kwargs: 'dict[str, Any]') -> 'Any': #: made the field two octets. #: * ``HIP_TRANSFORM`` is HIPv1-only -- ``_read_param_hip_transform`` raises for #: any other version -- and is built at version 1 through -#: :data:`HIP_VERSION`, so it round-trips and is no longer recorded. +#: :data:`HIP_VERSION`, so it round-trips and needs no entry. #: #: A third gap here, ``R1_Counter`` (128), was closed separately by #690 and was #: never one of the pair's cancellations above: it parsed as an @@ -1050,8 +1048,8 @@ def _mh_extension_build(code: 'Any', kwargs: 'dict[str, Any]') -> 'Any': #: ``code=`` of the class statement and ``R1CounterParameter`` declared only #: 129 -- even though ``__parameter__``'s ``_read_param_*``/``_make_param_*`` #: entries already existed for both codes, as two hand-written dict entries, -#: not because of any name-normalisation rule. Not recorded since #690, -#: which registered ``R1CounterParameter`` for 128 too. +#: not because of any name-normalisation rule. #690 registered +#: ``R1CounterParameter`` for 128 too, so it needs no entry. #: #: So the pair was routing around nothing by the time #672, #679 and #690 had #: all landed, and dropping to one copy does not change what round-trips. @@ -1100,8 +1098,8 @@ def _hip_overrides() -> 'dict[Any, dict[str, Any]]': # These two are not here because the default fails -- ``counter=0`` # constructs and parses perfectly well. They are here because a # zero-valued field cannot discriminate a width defect from a correct - # one, and this parameter's width defect (#672) hid behind exactly that - # for as long as it did. The RFC-only walk over + # one, which is how this parameter's width defect (#672) stayed + # hidden. The RFC-only walk over # ``options-internet.pcap`` -- which takes its stride solely from # :rfc:`7401` Section 5.2.1's ``Total Length = 11 + Length - (Length + # 3) % 8`` -- read ``Length = 12``, advanced 16 over a 12-octet record, @@ -1110,8 +1108,8 @@ def _hip_overrides() -> 'dict[Any, dict[str, Any]]': # octets. Every octet it mis-read was zero, so its zero-padding check # passed and it reported no violation. Measured: patching one counter to # ``aabbccdd`` turned that silence into ``type 0 padding not zeroed: - # aabbccdd``. With a non-zero counter the fixture can no longer conceal - # a mis-stride here, whether or not the width is right -- which is the + # aabbccdd``. With a non-zero counter the fixture cannot conceal a + # mis-stride here, whether or not the width is right -- which is the # reason to keep this override, #672 having corrected the width. Parameter.R1_COUNTER: {'counter': 0xaabbccdd}, Parameter.R1_Counter: {'counter': 0xaabbccdd}, @@ -1784,7 +1782,7 @@ def seen() -> 'tuple[str, ...]': # Broad on purpose: the point is to find out *what* a # constructor does when it is wrong, and narrowing this to # ProtocolError would let an AttributeError or a KeyError -- - # which is what most of the current defects raise -- abort the + # which is what most defects here raise -- abort the # sweep rather than be recorded as this case's result. return Outcome(case, 'CONSTRUCT', f'{type(exc).__name__}: {exc}', None, seen()) @@ -1868,7 +1866,7 @@ class _deadline: # pylint: disable=invalid-name Each raise re-arms the alarm for one more second, so the raise repeats until :attr:`settled` is set (#1889). One raise is not enough: one that lands in a finaliser, or in an ``except OSError``, never reaches the body, and a single - alarm then left a hung case hung. The body must set :attr:`settled` before + alarm would leave a hung case hung. The body must set :attr:`settled` before it leaves: :meth:`__exit__` sets it too, but a firing handled as :meth:`__exit__` is entered runs before it can. diff --git a/examples/generators/pcapng.py b/examples/generators/pcapng.py index 5e003954af..d36aa9f6e5 100644 --- a/examples/generators/pcapng.py +++ b/examples/generators/pcapng.py @@ -51,91 +51,86 @@ Block types deliberately left out --------------------------------- -Some block types once raised on spec-conformant input and parse since their -fixes, so they are absent from these fixtures only because the fixtures have not -been extended to carry them -- not because pcapkit cannot read them: - -* **Custom Block** (``0x00000BAD``/``0x40000BAD``) -- **parses.** Its - padding was computed as ``(4 - pkt['data'] % 4) % 4``, i.e. on the - :class:`bytes` object rather than on its length, so any custom block raised - ``TypeError: not all arguments converted during bytes formatting``. The - padding field is gone rather than repaired: the block carries no length for - its custom data, so ``data`` already spans the custom data, its padding *and* - the block's options, which is as much as a reader that does not own the - private enterprise number can tell apart (GitHub issue #341). +These block types parse, and are absent from these fixtures only because the +fixtures do not carry them -- not because pcapkit cannot read them: + +* **Custom Block** (``0x00000BAD``/``0x40000BAD``) -- **parses.** It has no + padding field: the block carries no length for its custom data, so ``data`` + spans the custom data, its padding *and* the block's options, which is as + much as a reader that does not own the private enterprise number can tell + apart. A padding computed as ``(4 - pkt['data'] % 4) % 4``, on the + :class:`bytes` object rather than on its length, makes any custom block + raise ``TypeError: not all arguments converted during bytes formatting`` + (GitHub issue #341). * **Packet Block** (obsolete, ``0x00000002``) -- **parses.** Its - ``interface_id`` and ``drop_count`` were declared ``UInt32Field`` although - Appendix A of draft-ietf-opsawg-pcapng (Figure 19) packs both into one 32-bit - word -- which is the layout the ``options`` length's 32-octet overhead already - assumed. Both are ``UInt16Field``, and the reader resolves the link type - from the block's own ``interface_id`` rather than through the ``linktype`` - property, which needs a ``self._info`` that does not exist yet while the block - is still being read and so failed with ``AttributeError: 'PCAPNG' object has - no attribute '_info'`` (GitHub issue #345). The block MUST NOT appear in new - files, so a fixture carrying one would be documenting the past. - -Further constructs were once avoided rather than covered; they are -**fixed**, so the fixtures carry them as regression cover: + ``interface_id`` and ``drop_count`` are ``UInt16Field``, as Appendix A of + draft-ietf-opsawg-pcapng (Figure 19) packs both into one 32-bit word, the + layout the ``options`` length's 32-octet overhead assumes. The reader + resolves the link type from the block's own ``interface_id`` rather than + through the ``linktype`` property, which needs a ``self._info`` that does + not exist yet while the block is still being read, and so fails with + ``AttributeError: 'PCAPNG' object has no attribute '_info'`` (GitHub issue + #345). The block MUST NOT appear in new files, so a fixture carrying one + would be documenting the past. + +The fixtures carry these constructs as regression cover: * **``if_IPv6addr``** -- ``IPv6InterfaceField.post_process`` in - ``pcapkit/corekit/fields/ipaddress.py`` read the trailing prefix-length octet - as ``int(value[16:])``, i.e. it parsed a raw byte as an ASCII decimal string, - so a ``/64`` prefix raised ``ValueError`` and only the ten lengths 48-57 - parsed at all -- each of them to the wrong value (GitHub issue #346). Every - interface built by :func:`_interface_profile` carries the option, using - the ``/64`` address from section 4.2 of the format specification. -* **Simple Packet Block** in a multi-interface section -- the engine raised - ``FormatError: PCAP-NG: [SPB] invalid section with 2 interfaces`` for any - simple packet block in a section declaring more than one interface, which the - format specification (section 4.4 of draft-ietf-opsawg-pcapng) permits: it - says only that such a block then refers to the interface described by the - first Interface Description Block, not that it is invalid. The check is for - *zero* interfaces (GitHub issue #347), so ``test.pcapng`` carries its - simple packet block in the two-interface first section, which is the case - that used to be rejected. - -Further defects were exercised on purpose, because they warned rather than -raised, and a fixture that covers the code path is what will catch a future -crash there. Each was an option-area sizing error in -``pcapkit/protocols/schema/misc/pcapng.py``, each was confirmed by -bisecting one block type at a time, and each is **fixed** -- so the -fixtures that reached them are regression cover rather than known-bad input: - -* **Interface Statistics Block** -- ``options`` was sized ``length - 20`` though - the block's fixed fields occupy 24 octets, so the option area over-ran into - the trailing block length. Unconditional: an explicit ``opt_endofopt`` did not - save it, because the field still consumed its declared width. Symptom, on - every capture containing one including the downloaded - ``many_interfaces.pcapng``: ``packet length < 0: -8`` and ``[Block 5] block - length mismatch: N != 0``. It is ``length - 24`` (GitHub issue #342). -* **Name Resolution Block options** -- ``options`` was sized - ``__option_padding__ - 4``, which over-ran whenever ``ns_*`` options were - present. Symptom: ``[Block 4] block length mismatch: 60 != 314``. Not a wrong - constant: ``Schema.unpack`` advances the file by each field's *declared* - length, so the octets the option field was trying to size had already been - consumed by ``records``. An ``OptionField`` hands the remainder it did not - parse back to the file -- the rewind ``ForwardMatchField`` already did -- which - is what makes ``__option_padding__`` mean the same thing to the field that - reads it as to the field that reported it, and the NRB's ``options`` is sized - ``__option_padding__`` (GitHub issue #344). ``ns_dnsname`` also gained the - ``PaddingField`` its ``if_name`` and ``if_description`` siblings have, without - which an unaligned DNS name under-read by its own padding. -* **Name Resolution Block IPv6 records** -- ``resol`` was sized ``length - 4``, - copied from the IPv4 record where the address is four octets wide; an IPv6 - address is sixteen, so the name field over-read by twelve and swallowed the - record terminator and the block's options. Symptom: ``packet length < - 0: -29273``, and the block's ``ns_*`` options vanished. It is ``length - 16`` - (GitHub issue #343). + ``pcapkit/corekit/fields/ipaddress.py`` reads the trailing prefix-length + octet as a byte. Read as ``int(value[16:])``, i.e. parsed as an ASCII + decimal string, a ``/64`` prefix raises ``ValueError`` and only the ten + lengths 48-57 parse at all -- each of them to the wrong value (GitHub issue + #346). Every interface built by :func:`_interface_profile` carries the + option, using the ``/64`` address from section 4.2 of the format + specification. +* **Simple Packet Block** in a multi-interface section -- the format + specification (section 4.4 of draft-ietf-opsawg-pcapng) permits it: it says + only that such a block then refers to the interface described by the first + Interface Description Block, not that it is invalid. The engine's check is + for *zero* interfaces (GitHub issue #347); one for more than one raises + ``FormatError: PCAP-NG: [SPB] invalid section with 2 interfaces``. So + ``test.pcapng`` carries its simple packet block in the two-interface first + section. + +The fixtures also cover option-area sizing errors in +``pcapkit/protocols/schema/misc/pcapng.py``, each confirmed by bisecting one +block type at a time. They warn rather than raise, and a fixture that covers +the code path is what will catch a future crash there: + +* **Interface Statistics Block** -- the block's fixed fields occupy 24 + octets, so ``options`` is sized ``length - 24`` (GitHub issue #342). Sized + ``length - 20``, the option area over-runs into the trailing block length, + unconditionally: an explicit ``opt_endofopt`` does not save it, because the + field still consumes its declared width. Symptom, on every capture + containing one including the downloaded ``many_interfaces.pcapng``: + ``packet length < 0: -8`` and ``[Block 5] block length mismatch: N != 0``. +* **Name Resolution Block options** -- ``options`` is sized + ``__option_padding__`` (GitHub issue #344). Sized ``__option_padding__ - 4``, + it over-runs whenever ``ns_*`` options are present: ``[Block 4] block length + mismatch: 60 != 314``. Not a wrong constant: ``Schema.unpack`` advances the + file by each field's *declared* length, so the octets the option field is + trying to size have already been consumed by ``records``. An + ``OptionField`` hands the remainder it did not parse back to the file, as + ``ForwardMatchField`` rewinds, which is what makes ``__option_padding__`` + mean the same thing to the field that reads it as to the field that + reports it. ``ns_dnsname`` has the ``PaddingField`` its ``if_name`` and + ``if_description`` siblings have, without which an unaligned DNS name + under-reads by its own padding. +* **Name Resolution Block IPv6 records** -- ``resol`` is sized ``length - 16`` + (GitHub issue #343). Sized ``length - 4``, as for the IPv4 record where the + address is four octets wide, the name field over-reads by twelve and + swallows the record terminator and the block's options. Symptom: ``packet + length < 0: -29273``, and the block's ``ns_*`` options vanish. ``many_interfaces.pcapng`` and ``profile.pcapng`` extract with neither a -block length mismatch nor a negative packet length; before these fixes each -reported one of each. +block length mismatch nor a negative packet length; without these fixes each +reports one of each. Every fixture here was checked against an independent PCAP-NG implementation (scapy's ``PcapNgReader``) as well as against pcapkit, and against a structural walk asserting that each block's total length is 4-octet aligned, repeated identically at both ends, and that the block chain covers the file exactly. So -the complaints above were pcapkit's readings, not malformed fixtures. +the complaints above are pcapkit's readings, not malformed fixtures. One complaint is expected rather than a defect: ``test.pcapng`` carries a deliberately truncated packet (``captured_len`` below ``original_len``, to @@ -360,8 +355,8 @@ def journal(self, entry: 'bytes') -> 'bytes': """:manpage:`systemd(1)` Journal Export Block. ``entry`` is padded out to a 4-octet boundary by :meth:`_Blocks.block`, - not here -- see :func:`_journal_entry` for why that distinction used to - matter. + not here -- see :func:`_journal_entry` for why that distinction does + not matter to a reader. """ return self.block(BLOCK_JOURNAL, entry) @@ -422,10 +417,10 @@ def _journal_entry(message: 'str', realtime_us: 'int') -> 'bytes': The block body is padded to a 4-octet boundary by :meth:`_Blocks.block`, so a message whose entry is already aligned gets no padding at all. That - used to be load-bearing: an unaligned entry's own NUL padding was read as - a binary field name and raised a bare ``struct.error`` (fixed in `#699 - `__, which made a - NUL-only line end the entry). Measured on ``ef859f776``: a misaligned + is not load-bearing: a NUL-only line ends the entry (`#699 + `__), without which an + unaligned entry's own NUL padding is read as a binary field name and + raises a bare ``struct.error``. Measured on ``ef859f776``: a misaligned entry parses with no ``SchemaWarning``. The loop stays only because removing it regenerates ``test.pcapng`` -- an untracked fixture change, not a prose one; see `#791 @@ -483,9 +478,8 @@ def _interface_profile(writer: '_Blocks', name: 'str', description: 'str', resolution: 'int' = 6) -> 'list[tuple[int, bytes]]': """The full set of ``if_*`` options pcapkit can parse for one interface. - ``if_IPv6addr`` was once the one omission, because pcapkit raised on it - rather than because it did not belong here; it is included since the - prefix-length octet is read correctly -- see the module docstring. + ``if_IPv6addr`` among them: pcapkit reads its prefix-length octet as a + byte (GitHub issue #346) -- see the module docstring. Args: writer: Writer whose byte order the option values are packed in. @@ -503,7 +497,7 @@ def _interface_profile(writer: '_Blocks', name: 'str', description: 'str', (IF_DESCRIPTION, description.encode('utf-8')), (IF_IPV4ADDR, octets + bytes([255, 255, 255, 0])), # The address from section 4.2 of draft-ietf-opsawg-pcapng, whose /64 - # prefix is the case that used to raise: the trailing octet is 0x40, + # prefix is the case #346 guards: the trailing octet is 0x40, # which read as an ASCII decimal string is the character ``@``. (IF_IPV6ADDR, bytes.fromhex('20010db885a308d313198a2e03707344') + bytes([64])), (IF_MACADDR, mac), @@ -522,10 +516,9 @@ def _statistics(writer: '_Blocks', interface: 'int', received: 'int', dropped: ' start_us: 'int', end_us: 'int') -> 'list[tuple[int, bytes]]': """The full set of ``isb_*`` options for one interface statistics block. - The list is terminated by :meth:`_Blocks.options`, which matters here: - pcapkit sizes an interface statistics block's option area four octets too - generously, and only the explicit ``opt_endofopt`` stops it reading the - trailing block length as an option. + The list is terminated by :meth:`_Blocks.options`. The block's option area + is sized ``length - 24`` (GitHub issue #342), which an ``opt_endofopt`` + alone cannot stand in for -- see the module docstring. """ def split(value: 'int') -> 'bytes': @@ -629,13 +622,13 @@ def build_test() -> 'bytes': original_len=len(packets[2]))) # The simple packet block sits in this section, which declares two - # interfaces, because that is the shape the engine used to reject: the + # interfaces, because that is the shape #347 guards: the # format spec says such a block refers to the first interface description # block, not that it is invalid (GitHub issue #347). blocks.append(writer.spb(packets[3])) - # Name resolution. The IPv6 record is included knowing pcapkit mis-sizes - # it -- covering the path is what catches a future crash there. + # Name resolution. The IPv6 record is included for its sizing (GitHub + # issue #343) -- covering the path is what catches a future crash there. blocks.append(writer.nrb([ (NRB_RECORD_IPV4, bytes([10, 0, 0, 1]) + b'gateway.example\x00'), (NRB_RECORD_IPV4, bytes([10, 0, 0, 2]) + b'client.example\x00alias.example\x00'), @@ -1047,7 +1040,8 @@ def generate(dest: 'pathlib.Path | None' = None) -> 'list[pathlib.Path]': than silent. Args: - dest: Destination directory; defaults to ``/sample``. + dest: Destination directory; defaults to ``examples/captures/`` under + the repository root. Returns: The paths written, in fixture order. diff --git a/examples/legacy_smoke/README.md b/examples/legacy_smoke/README.md index 044ef20c8e..6c3dea15e2 100644 --- a/examples/legacy_smoke/README.md +++ b/examples/legacy_smoke/README.md @@ -106,14 +106,13 @@ Both *inputs* are committed, so this works on a fresh clone with no `make sample first. Use `make fixtures PYTHON=../../.venv/bin/python` to pick a specific interpreter. -These reports were tracked once, and #685 is why they are not any more. Nothing reads a -report back -- no test asserts on one, and the docs only quote them as prose -- so a -parser change that alters how a frame renders leaves the committed copy quietly wrong, -with no failure anywhere to say so. `pcapng.txt` sat recording `packet -> NIL` for -blocks that had started carrying their captured octets, and the staleness was -found by reading the file rather than by anything breaking. Generating them on demand -means the copy on your disk describes the tree you are standing in, which was the only -thing that made them worth looking at. +They are untracked by design (#685). Nothing reads a report back -- no test asserts on +one, and the docs only quote them as prose -- so a parser change that alters how a frame +renders would leave a committed copy quietly wrong, with no failure anywhere to say so: +a committed `pcapng.txt` recorded `packet -> NIL` for blocks that carry their captured +octets, and the staleness was found by reading the file rather than by anything +breaking. Generated on demand, the copy on your disk describes the tree you are +standing in, which is the only thing that makes them worth looking at. `TZ=UTC` is a belt-and-braces pin, not a requirement. `pcapkit` renders a frame timestamp in UTC, or in the zone a PCAP-NG interface declares in `if_tzone`, and diff --git a/examples/legacy_smoke/test_profile.py b/examples/legacy_smoke/test_profile.py index f341d03b32..4b141810fb 100644 --- a/examples/legacy_smoke/test_profile.py +++ b/examples/legacy_smoke/test_profile.py @@ -16,10 +16,10 @@ def test() -> 'None': if __name__ == '__main__': - # cProfile will not create the directory it dumps into, so `python - # test_profile.py` on a fresh checkout used to profile the whole run and then - # die with FileNotFoundError. `make profile` does mkdir first; the script - # should not need it to. + # cProfile will not create the directory it dumps into, so without this + # `python test_profile.py` on a fresh checkout profiles the whole run and + # then dies with FileNotFoundError. `make profile` does mkdir first; the + # script should not need it to. os.makedirs(os.path.dirname(STATS), exist_ok=True) cProfile.run( diff --git a/pyproject.toml b/pyproject.toml index ef42118089..b7f60a9e0e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -3,10 +3,10 @@ requires = [ # 69.0.0 is the first release that ships ``pcapkit/py.typed`` unasked # (pypa/setuptools#3136, in its NEWS.rst): nothing here declares the file, so # a wheel built with 68.2.2 or older leaves it out and type checkers read the - # package as untyped (#2062). Measured on both sides of the bump. It also - # keeps what the earlier floor of 62.3.0 was for: ``package-data`` globs that - # read ``**`` as any depth, which the ``pcapkit._extern`` entry below needs - # (#1814) -- 61.0.0 and 62.2.0 read it as ``*``, measured. 69.0.0 still + # package as untyped (#2062). Measured on both sides of the bump. It is also + # above 62.3.0, the floor for ``package-data`` globs that read ``**`` as any + # depth, which the ``pcapkit._extern`` entry below needs (#1814) -- 61.0.0 + # and 62.2.0 read it as ``*``, measured. 69.0.0 still # supports Python 3.8, the ``requires-python`` floor. "setuptools>=69.0.0", ] @@ -28,8 +28,8 @@ maintainers = [ ] license = { text="BSD 3-Clause License" } # 3.8 is the floor (#2051): the sources use 3.8 syntax (assignment expressions, -# positional-only parameters) and nothing converts them any more, since the -# ``bpc-*`` backport tools are no longer maintained. No upper bound: there is no +# positional-only parameters) and nothing converts them for an older Python, as +# the ``bpc-*`` backport tools are no longer maintained. No upper bound: there is no # Python 4 for one to exclude, and a cap here is copied into the metadata of # every project that depends on this one. # @@ -124,9 +124,9 @@ crypto = [ # carries obligations on redistribution that a BSD-licensed dependent may # not want. # -# #910's owner accepted that trade deliberately rather than leaving ``all`` -# contradict the reasoning that used to keep ``pycrate`` out of it -- see the -# ``all`` extra's own comment below for the rest of that ruling. This extra +# #910's owner accepted that trade deliberately, over the size and licence +# costs above -- see the ``all`` extra's own comment below for the rest of that +# ruling. This extra # still exists on its own, for installing NGAP support without the ``cli`` and # ``crypto`` core addons ``all`` bundles it with. # @@ -216,27 +216,25 @@ PyPCAPFile = [ "pypcapfile; python_version < '3.12'" ] # `pcapkit-vendor` to regenerate pcapkit.const. Includes ``pycrate`` so # pcapkit.vendor.ngap's crawlers (#880) can run, and ``requests``/ # ``beautifulsoup4`` for the crawlers that import them at module scope -# (c.f. the ``test`` extra's own comment below for which, and #507). #910 -# narrowed ``all`` to core addons only, so this extra names each of them -# itself rather than leaning on ``all`` to also carry them -- ``pycrate`` -# happens to be a core addon in its own right post-#910 (see the ``all`` -# extra's comment below), but ``requests``/``beautifulsoup4`` are not, having -# moved to ``dev`` instead. Either way, this extra's own requirements are what +# (c.f. the ``test`` extra's own comment below for which, and #507). ``all`` +# carries core addons only (#910), so this extra names each of them itself +# rather than leaning on ``all`` to also carry them -- ``pycrate`` happens to be +# a core addon in its own right (see the ``all`` extra's comment below), but +# ``requests``/``beautifulsoup4`` are not, and ``dev`` carries them +# instead. Either way, this extra's own requirements are what # it is correct against, not whatever ``all`` happens to carry. vendor = [ "requests[socks]", "beautifulsoup4[html5lib]", "pycrate" ] -# #910: narrowed to *core addons* only -- the owner's ruling scopes this to -# everything needed for the library itself to work at full functionality, rather -# than everything an end user might conceivably want. An earlier ruling on the -# same issue read the latter and added ``pycrate`` on that basis; the ruling that -# stuck narrowed it again, this time by kind: ``cli``, ``crypto`` and ``pycrate`` -# (NGAP) are core addons and stay, and the 3rd-party capture engines that -# used to be here -- ``dpkt``, ``scapy``, ``pyshark``, ``pypcapfile`` -- are -# installed one at a time on demand, as ``PyPCAP`` and ``PCAP_CT`` always were, -# and move out to their own extras below. Install one explicitly: ``pip install pypcapkit[DPKT]`` / +# #910: *core addons* only -- the owner's ruling scopes this to everything +# needed for the library itself to work at full functionality, rather than +# everything an end user might conceivably want, and draws the line by kind: +# ``cli``, ``crypto`` and ``pycrate`` (NGAP) are core addons, and the 3rd-party +# capture engines -- ``dpkt``, ``scapy``, ``pyshark``, ``pypcapfile`` -- are +# installed one at a time on demand, as ``PyPCAP`` and ``PCAP_CT`` are, from +# their own extras below. Install one explicitly: ``pip install pypcapkit[DPKT]`` / # ``[Scapy]`` / ``[PyShark]`` / ``[PyPCAPFile]``. # -# ``pypcap`` and ``pcap-ct``/``libpcap`` stay out for the reason they always -# have, which #910 leaves untouched: installability, not desirability. +# ``pypcap`` and ``pcap-ct``/``libpcap`` stay out for a reason of their own, +# apart from #910's: installability, not desirability. # ``pypcap`` is an sdist-only C extension that needs a compiler and the libpcap # development files, and fails where only the header is present -- as on the # macOS runners. ``pcap-ct`` and ``libpcap`` need no compiler, but are published @@ -253,10 +251,10 @@ all = [ ] # Not for an end user -- for CI, which has a different need entirely: pylint, # mypy and Sphinx's autodoc all resolve imports against whatever is installed, -# so narrowing ``all`` above would otherwise have made every one of -# ``dpkt``/``scapy``/``pyshark``/``pypcapfile`` newly unresolvable to those +# so with ``all`` limited to core addons, every one of +# ``dpkt``/``scapy``/``pyshark``/``pypcapfile`` would be unresolvable to those # tools. This extra is what a workflow installs *alongside* ``all`` -- -# ``.[all,dev]`` -- to see everything ``all`` used to carry, for that purpose +# ``.[all,dev]`` -- to see those engines too, for that purpose # only. ``.github/workflows/lint.yml``'s own comment traces pylint's # ``import-error`` messages, from ``pypcap``/``pcap-ct`` (via ``PyPCAP`` and # ``PCAP_CT`` above, both absent on the Python it lints) and from @@ -267,8 +265,8 @@ all = [ dev = [ # scapy: the same floor as the Scapy extra above, #1572 "dpkt", "scapy>=2.6.0", "pyshark", - # same marker and reasoning as the PyPCAPFile extra above and the old - # ``all``: on 3.12+ the package installs cleanly and is then unusable, so + # same marker and reasoning as the PyPCAPFile extra above: on 3.12+ the + # package installs cleanly and is then unusable, so # the marker keeps the resolver from fetching something that could never # run. It is not guarding against a hard error -- ``Extractor.run`` already # degrades to the default engine with a warning, installed or not @@ -290,7 +288,7 @@ test = [ # >=3.6.1, not >=3: the `shouldfail` forwarding tests/conftest.py's xdist fix # depends on (`WorkerInteractor.pytest_sessionfinish` in xdist/remote.py) # lands in 3.6.0, which PyPI has yanked, so 3.6.1 is the actual floor. - # Below it the fix silently reverts and + # Below it the fix silently does nothing and # test_xdist_run_reports_the_diagnostic_and_fails goes red rather than # skipped, since `importlib.util.find_spec('xdist')` still succeeds. "pytest-xdist>=3.6.1", @@ -329,10 +327,10 @@ test = [ "requests", "beautifulsoup4", # Same reason as the entries above, for a different test: without it - # ``tests/project/test_isort_clean.py`` skipped on every CI leg, so the only + # ``tests/project/test_isort_clean.py`` skips on every CI leg, so the only # guard on ``make isort``'s recipe lines -- and on the target-resolving - # globs behind them, wrong more than once (#757, #765, #767) -- ran - # nowhere. #766. + # globs behind them, wrong more than once (#757, #765, #767) -- runs + # nowhere (#766). # # A lint tool in the ``test`` extra rather than in a ``lint`` extra of its # own, deliberately. ``isort`` is what *this suite* needs in order not to @@ -350,8 +348,8 @@ test = [ # Unpinned, matching ``requests`` and ``beautifulsoup4`` above rather than # inventing an unmeasured floor (verified against 9.0.1). A future isort # whose verdict moves therefore turns that test red, which is the intended - # failure: it names the file and prints the diff, where the skip it replaced - # said nothing at all. + # failure: it names the file and prints the diff, where a skip would say + # nothing at all. "isort", # The second opinion on the hand-rolled YAML scanners: the ``yaml.safe_load`` # cross-checks under ``tests/project/``, and the tests there that parse a diff --git a/util/_actions.py b/util/_actions.py index 2dcc11c9f2..c4fae881e9 100644 --- a/util/_actions.py +++ b/util/_actions.py @@ -190,7 +190,7 @@ def open_summary(*, environ: 'Optional[Mapping[str, str]]' = None) -> 'IO[str]': """ environ = os.environ if environ is None else environ - # Both are the caller's to close, as the file this returned before #2030 was. + # Both are the caller's to close, as a plain summary file would be (#2030). page = open(environ['GITHUB_STEP_SUMMARY'], 'a', encoding='utf-8') # pylint: disable=consider-using-with path = environ.get(SUMMARY_COPY) if not path: @@ -268,5 +268,5 @@ def getenvb(name: 'str') -> 'bytes': if os.supports_bytes_environ: return os.environb[name.encode()] # #2120: Windows keeps its environment as text and has no os.environb, so - # reading it there raised AttributeError on the win-64 conda leg. + # reading it there raises AttributeError, as on the win-64 conda leg. return os.fsencode(os.environ[name]) diff --git a/util/apt_packages.py b/util/apt_packages.py index 5909fbd3c2..3366364379 100644 --- a/util/apt_packages.py +++ b/util/apt_packages.py @@ -121,7 +121,7 @@ def plan() -> 'int': for attempt in range(1, ATTEMPTS + 1): if call(['sudo', 'timeout', str(UPDATE_BOUND), 'apt-get', 'update']).returncode == 0: break - # #2047: unescaped, as before the helper, and through `say`, which writes a lone surrogate as its byte. + # #2047: unescaped, and through `say`, which writes a lone surrogate as its byte. _actions.say(_actions.annotation( 'warning', f'apt-get update failed or exceeded its {UPDATE_BOUND}s bound.', title=f'apt-get update attempt {attempt} of {ATTEMPTS} failed ({os.environ["APT_SET"]})', escape=False)) diff --git a/util/bump_version.py b/util/bump_version.py index fab7842ddb..685634039f 100644 --- a/util/bump_version.py +++ b/util/bump_version.py @@ -177,7 +177,7 @@ class CitationFieldError(RuntimeError): """Raised when :file:`CITATION.cff` exists but has no ``version`` field. Subclasses :exc:`RuntimeError` rather than defining a new hierarchy, to match - the ``cannot find version`` failure this script already raised for the same + the ``cannot find version`` failure this script already raises for the same class of problem in :file:`pcapkit/__init__.py`. """ @@ -190,13 +190,13 @@ def current_version() -> str: has, and falls back to scraping :file:`pcapkit/__init__.py` out of the working directory when :mod:`pcapkit` cannot be imported. - The fallback strips whitespace before quotes rather than both at once. The - previous spelling, ``strip(" '")``, stopped at the trailing newline and so - returned ``"1.5.0b4'\\n"`` -- quote and newline included -- which + The fallback strips whitespace before quotes rather than both at once: + ``strip(" '")`` stops at the trailing newline and so returns + ``"1.5.0b4'\\n"`` -- quote and newline included -- which :class:`~packaging.version.Version` rejects outright with - :exc:`~packaging.version.InvalidVersion`. The path had therefore never worked; - it went unnoticed because the only caller installs the package first, so - :mod:`pcapkit` always imports and the fallback is never reached in CI. + :exc:`~packaging.version.InvalidVersion`. CI never reaches the fallback, + because the only caller installs the package first, so :mod:`pcapkit` + always imports. Returns: The current version string. diff --git a/util/check_suite_report.py b/util/check_suite_report.py index e009df28f7..3670991e3a 100644 --- a/util/check_suite_report.py +++ b/util/check_suite_report.py @@ -89,9 +89,9 @@ class Workflow(NamedTuple): expected: 'dict[str, tuple[tuple[str, str], ...]]' -#: The workflows reported, by name. No list holds an annotation today: the 3.7 -#: legs that stopped at install went with #2051, and pypy3.10 installs since -#: #2040's cryptography cap. +#: The workflows reported, by name. No ``expected`` list holds an annotation: no +#: leg stops at install by design, since the KTLO floor is 3.8 (#2051) and +#: pypy3.10 installs under #2040's cryptography cap. WORKFLOWS = { 'Unit Tests': Workflow( 'unit-tests', re.compile(r'(?:Python |Integration Python |Engines \(|PyPCAP/PyPCAPFile parity Python )'), diff --git a/util/ci_timing.py b/util/ci_timing.py index e28a383fbb..5129bc4ad3 100644 --- a/util/ci_timing.py +++ b/util/ci_timing.py @@ -116,9 +116,8 @@ #: The suffix every KTLO leg's job name carries (#1633); such a leg's steps cannot #: fail it (#1672). KTLO_SUFFIX = ' (KTLO)' -#: The suffix an experimental leg's job name carried in ``unit-tests.yml`` before -#: #1732 moved it out; such a leg's steps could not fail it either, and its runs -#: stay in the window until they age out. +#: The suffix every experimental leg's job name carries (#1732); such a leg's +#: steps cannot fail it either. EXPERIMENTAL_SUFFIX = ' (experimental)' #: The aggregate job the ruleset requires; its completion is "mergeable". @@ -364,7 +363,7 @@ def _parse_steps(section: 'str') -> 'list[dict[str, Any]]': A cap written as an expression differs by leg, and the job's spec does not say which leg a name ran, so it reads as none, as an uncapped step's does. No - workflow writes one since #1732 moved the experimental legs out of ``test``. + workflow writes one (#1732). """ found = [] # type: list[dict[str, Any]] diff --git a/util/classify_changes.py b/util/classify_changes.py index d986a22591..ce6548ec79 100644 --- a/util/classify_changes.py +++ b/util/classify_changes.py @@ -62,7 +62,7 @@ def main() -> 'int': capture_output=True, text=True, check=False) files = [path for path in diff.stdout.split('\0') if path] if diff.returncode != 0 or not files: - # #2047: unescaped, as before the helper, so git's stderr keeps its lines. + # #2047: unescaped, so git's stderr keeps its lines. _actions.annotate('warning', f'git diff exited {diff.returncode} with {len(files)} files: ' f'{diff.stderr.strip()}. Treating the diff as code.', title='Could not list the changed files', escape=False) diff --git a/util/conda_vendor.py b/util/conda_vendor.py index 521e196f8d..b5faf837c9 100644 --- a/util/conda_vendor.py +++ b/util/conda_vendor.py @@ -701,11 +701,11 @@ def refresh(table: 'dict[str, dict[str, Pin]]', fetch: 'Callable[[str], dict[str notes.append(('notice', f'{name} on {subdir} moves from {pin.version} to {newest}, the newest ' f'release with a wheel there; due for review by {kept[subdir].review_by}')) elif newest is None: - # #2025: a pin no release can move to used to stay without a word. + # #2025: a pin no release can move to stays, with a warning. notes.append(('warning', f'{name} {pin.version} on {subdir} stays as it is, and may no longer ' f'hold: {stuck(project, subdir, pythons, have)}')) elif Version(newest) < current < ceiling: - # #2028: the pinned release itself lost a wheel, which used to pass without a word too. + # #2028: the pinned release itself lost a wheel, which warns too. notes.append(('warning', f'{name} {pin.version} on {subdir} stays as it is, and may no longer ' f'hold: {lost(project, subdir, pythons, pin.version)}, and the ' f'newest release up to {have} with wheels for all of {_tags(pythons)} ' diff --git a/util/coverage_report.py b/util/coverage_report.py index 2002b7c557..1ab0497743 100644 --- a/util/coverage_report.py +++ b/util/coverage_report.py @@ -93,7 +93,7 @@ def check(log: 'str' = LOG) -> 'int': if len(root) != 1 or not workers or missing: why = f'no data file from pid {missing}' if missing else 'the process log is missing or unreadable' _actions.summary(f'**Coverage not reported:** {why} (#1387); a total would under-count.') - # #2047: unescaped, as before the helper. + # #2047: deliberately unescaped. _actions.annotate('error', f'Coverage data is incomplete: {why}.', escape=False) return 1 return 0 diff --git a/util/leg_summary.py b/util/leg_summary.py index 3feaf2fa7f..11cff082f1 100644 --- a/util/leg_summary.py +++ b/util/leg_summary.py @@ -72,7 +72,7 @@ def main(argv: 'Optional[list[str]]' = None) -> 'int': print(f'{leg}: no step failed.') return 0 # The runner splits an annotation's properties on `,`, so the title's are escaped; - # #2047: nothing else is, as before the helper. + # #2047: nothing else is. title = leg.replace(',', '%2C') with _actions.open_summary() as summary: for step in failed: diff --git a/util/pip_install.py b/util/pip_install.py index d0f915a76e..89ff855ffe 100644 --- a/util/pip_install.py +++ b/util/pip_install.py @@ -14,8 +14,8 @@ ``RUNNER_TEMP`` in the environment. It runs, on the same interpreter: 1. ``python -m pip install -U pip setuptools wheel``, its output passed through. A - failure ends the script with pip's status and no annotation, as ``set -e`` ended - the step. + failure ends the script with pip's status and no annotation, as ``set -e`` + would end a shell step. 2. ``python -m pip install -e SPEC``, its standard error merged into its standard output, which it copies, as it arrives, both to its own standard output and to :file:`$RUNNER_TEMP/pip-install.log`. On a failure it writes one @@ -108,7 +108,7 @@ def main(argv: 'Optional[list[str]]' = None) -> 'int': # The runner splits the properties on `,` before it un-escapes `%2C`, so the # leg's `(KTLO, non-blocking)` would end the title at "(KTLO" (#1633). #2047: - # escaped as before the helper, so pip's bytes pass through as they are. + # escaped here, not by the helper, so pip's bytes pass through as they are. message = reason(output).replace(b'%', b'%25') + SKIPPED _actions.say(_actions.annotation(level, message.decode('utf-8', 'surrogateescape'), title=TITLE + leg.replace(',', '%2C'), escape=False)) diff --git a/util/pyshark_encap_map.py b/util/pyshark_encap_map.py index 79cdae9261..5a108c6931 100644 --- a/util/pyshark_encap_map.py +++ b/util/pyshark_encap_map.py @@ -10,7 +10,7 @@ one round trip through Wireshark's own ``wiretap/pcap-common.c``, and this script is that round trip, made runnable instead of ad hoc. -The method, unchanged from #850 +The method, as #850 used it -------------------------------- For every encapsulation :program:`editcap` accepts under ``-T``: @@ -55,8 +55,8 @@ banner lines, not one: ``editcap: "" isn't a valid encapsulation type`` (the error for the empty argument itself) and then ``editcap: The available encapsulation types for the "-T" flag are:`` immediately after it -- so a -line-oriented split still has two non-entries to exclude, not the single line -an earlier draft of this docstring assumed. The true count is **226**, and +line-oriented split still has two non-entries to exclude, not one. The true +count is **226**, and the shape that yields it is `` - ``, anchored to leading whitespace so neither banner line -- both flush against the left margin, with no leading whitespace at all -- can match: diff --git a/util/release_conda.py b/util/release_conda.py index 8120c94506..616079cf12 100644 --- a/util/release_conda.py +++ b/util/release_conda.py @@ -36,15 +36,14 @@ :data:`PLATFORMS` maps each leg of either job to the platform it publishes. -The steps formerly held this as shell, each job its own copy of the build and -upload, under ``set -euo pipefail``. A failing command ends the script with its +Both jobs run this one copy of the build and upload. It fails as a shell step +under ``set -euo pipefail`` would: a failing command ends the script with its status, a missing one with 127, and a variable it reads that is unset with -``: unbound variable`` and status 1, as they ended the step. The build's -second search for packages, which could find nothing the first had not, is -gone. Where the shell trimmed a label with ``xargs``, this collapses its white -space alone, so a quote or backslash is taken as it is rather than failing the -step: every label is ``main`` or ``dev``. On ``windows-latest`` the paths are -Windows' own rather than Git Bash's ``/tmp/...``. +``: unbound variable`` and status 1. The build searches for packages +once. A label has its white space collapsed and nothing else, so a quote or +backslash is taken as it is, where ``xargs`` would fail the step on it: every +label is ``main`` or ``dev``. On ``windows-latest`` the paths are Windows' own +rather than Git Bash's ``/tmp/...``. The script is standard library only, and keeps to Python 3.8's syntax. diff --git a/util/release_published.py b/util/release_published.py index e064aeb850..706e8a6c3a 100644 --- a/util/release_published.py +++ b/util/release_published.py @@ -22,15 +22,15 @@ ``false``, the safe default, without failing ``version_check`` (see :func:`fetch`). Both are ``true`` only once what they count reaches what they expect, so a version expecting nothing is ``true`` unread. The one count runs -on ``python3 -`` as a snippet of its own, which keeps a body that does not -parse failing the step as it did, with ``python3``'s status and traceback. - -Both steps formerly held this as shell under ``set -euo pipefail`` and the -runner's ``bash -e``, and the two did the same but for their endpoint, their -expectation and their count; so they share this script. It keeps their -outcomes: the lists are read as ``read -r -a`` read them, from the first line, -split on spaces and tabs; ``curl``'s ``-w`` code and the ``000`` its failure -added are the ``http`` the log line names; a count that is not an integer is +on ``python3 -`` as a snippet of its own, so a body that does not parse fails +the step, with ``python3``'s status and traceback. + +The two steps differ only in their endpoint, their expectation and their +count, so they share this script, which keeps the outcomes of a shell step +under ``set -euo pipefail`` and the runner's ``bash -e``: the lists are read +as ``read -r -a`` reads them, from the first line, split on spaces and tabs; +``curl``'s ``-w`` code and the ``000`` its failure adds are the ``http`` the +log line names; a count that is not an integer is ``[``'s error and reads ``false``; a variable it reads that is unset, which Actions never leaves it, is ``: unbound variable`` and status 1. @@ -117,11 +117,11 @@ def anaconda_source(path: 'str') -> 'str': class Unbound(Exception): - """A variable read as ``$NAME`` that is unset, which ``set -u`` refused.""" + """A variable read as ``$NAME`` that is unset, which ``set -u`` refuses.""" class Failed(Exception): - """A command whose failure ended the shell under ``set -e``, with its status.""" + """A command whose failure ends a shell under ``set -e``, with its status.""" def __init__(self, status: 'int') -> 'None': super().__init__(status) @@ -276,7 +276,7 @@ def anaconda(path: 'str') -> 'int': """``PCAPKIT_CONDA_COMPLETE``: each platform the version was released for, for each supported version.""" builds = var('PCAPKIT_CONDA_BUILDS') subdirs = released_subdirs() - # Exported, as the step did, so curl and the snippet see it. + # Exported, so curl and the snippet see it. os.environ['PCAPKIT_CONDA_SUBDIRS'] = os.fsdecode(subdirs) expected = len(words(subdirs)) * len(words(builds)) return check('anaconda', expected, ANACONDA_URL.encode() + var('PCAPKIT_VERSION'), path, anaconda_source) diff --git a/util/release_reset_build.py b/util/release_reset_build.py index c319455be3..3b5e47445b 100644 --- a/util/release_reset_build.py +++ b/util/release_reset_build.py @@ -21,13 +21,12 @@ fresh fetch, three times at most, then an ``::error`` and status 1. The tag is never touched: it was made before anything here reads ``main``. -The step formerly held this as shell under ``set -euo pipefail``; this prints -the same lines and annotations, unescaped as ``echo`` wrote them, and runs the -same ``git`` commands with the same arguments. A ``git`` that fails where the -shell had no ``if`` ends the script with its status, as ``set -e`` ended the -step, and a variable the step's ``env:`` names that is unset, which Actions -never leaves it, ends it with ``: unbound variable`` and status 1 where -``set -u`` did. +It behaves as a shell step under ``set -euo pipefail`` would: its lines and +annotations are unescaped, as ``echo`` writes them, a ``git`` that fails where +nothing tests its status ends the script with that status, as ``set -e`` would, +and a variable the step's ``env:`` names that is unset, which Actions never +leaves it, ends it with ``: unbound variable`` and status 1, as +``set -u`` would. ``tag`` has no setup-python, so this runs on the image's ``python3``: standard library only, and Python 3.8's syntax. Its ``git switch`` replaces the checkout, @@ -57,14 +56,14 @@ #: How many pushes to ``main`` are tried before giving up. ATTEMPTS = 3 -#: What ``__version__``'s line starts with; ``sed -n 's/^__version__ = //p'`` printed the rest. +#: What ``__version__``'s line starts with; ``sed -n 's/^__version__ = //p'`` prints the rest. VERSION_PREFIX = '__version__ = ' -#: The bytes ``tr -d '[:space:]'`` deleted. +#: The bytes ``tr -d '[:space:]'`` deletes. SPACE = ' \t\n\v\f\r' class Unbound(Exception): - """A variable read as ``$NAME`` that is unset, which ``set -u`` refused.""" + """A variable read as ``$NAME`` that is unset, which ``set -u`` refuses.""" def var(name: 'str') -> 'str': @@ -96,7 +95,7 @@ def git(*args: 'str', capture: 'bool' = False) -> 'Tuple[int, str]': def version_of(text: 'str') -> 'str': - """What ``sed -n 's/^__version__ = //p'`` printed of ``text``, as ``$(...)`` gives it.""" + """What ``sed -n 's/^__version__ = //p'`` prints of ``text``, as ``$(...)`` gives it.""" return '\n'.join(line[len(VERSION_PREFIX):] for line in text.split('\n') if line.startswith(VERSION_PREFIX)).rstrip('\n') @@ -104,7 +103,7 @@ def version_of(text: 'str') -> 'str': def taken_builds(listing: 'str', builds: 'str') -> 'str': """The ``+N`` of each ref in ``git ls-remote``'s ``listing`` under ``builds`` but ``+0``, space-separated. - Each line is split as ``read -r _ ref`` split it, on spaces and tabs, the + Each line is split as ``read -r _ ref`` splits it, on spaces and tabs, the first field dropped. """ @@ -118,7 +117,7 @@ def taken_builds(listing: 'str', builds: 'str') -> 'str': def say(level: 'str', title: 'str', message: 'str') -> 'None': - """Print the ``::level`` annotation as the shell's ``echo`` did, unescaped.""" + """Print the ``::level`` annotation as a shell's ``echo`` does, unescaped.""" _actions.say(_actions.annotation(level, message, title=title, escape=False)) diff --git a/util/release_status.py b/util/release_status.py index 4ff9d90b81..4fdbedc318 100644 --- a/util/release_status.py +++ b/util/release_status.py @@ -27,11 +27,10 @@ Before any of them, a notice reports ``pypi``'s KTLO legs where either recorded an outcome (#1633), since neither reaches ``needs.pypi.result``. -The step formerly held this as shell, run under ``set -euo pipefail``; this -prints the same annotations, unescaped as ``echo`` wrote them. A variable the -step's ``env:`` names that is unset, which Actions never leaves it, ends the -script with ``: unbound variable`` and status 1 at the point the shell's -``set -u`` would have, rather than with a traceback. +It prints its annotations unescaped, as a shell step's ``echo`` writes them. A +variable the step's ``env:`` names that is unset, which Actions never leaves +it, ends the script with ``: unbound variable`` and status 1, where a +shell's ``set -u`` would, rather than with a traceback. The job checks out only :file:`util/` and has no setup-python, so this runs on the image's ``python3``: standard library only, and Python 3.8's syntax. @@ -66,7 +65,7 @@ class Unbound(Exception): - """A variable read as ``$NAME`` that is unset, which ``set -u`` refused.""" + """A variable read as ``$NAME`` that is unset, which ``set -u`` refuses.""" def var(name: 'str') -> 'str': @@ -78,7 +77,7 @@ def var(name: 'str') -> 'str': def say(level: 'str', title: 'str', message: 'str') -> 'None': - """Print the ``::level`` annotation as the shell's ``echo`` did, unescaped.""" + """Print the ``::level`` annotation as a shell's ``echo`` does, unescaped.""" _actions.say(_actions.annotation(level, message, title=title, escape=False)) diff --git a/util/release_tags.py b/util/release_tags.py index 8bb96580b6..3b017055bd 100644 --- a/util/release_tags.py +++ b/util/release_tags.py @@ -22,10 +22,10 @@ exactly where ``check`` would refuse a tag that exists. A failed fetch is ignored, since the checkout may already hold the tag, and a tag that does not resolve is no commit. ``PCAPKIT_RUN_SHA`` that does not resolve ends the script -with ``git``'s status, as ``set -e`` ended the step. +with ``git``'s status, as ``set -e`` would end a shell step. -The steps formerly held this as shell. Its unquoted ``${PCAPKIT_TAGS}`` was -also subject to pathname expansion, which this skips: every tag is ``v`` or +``PCAPKIT_TAGS`` is split without the pathname expansion an unquoted shell +``${PCAPKIT_TAGS}`` would apply, which changes nothing: every tag is ``v`` or ``conda-`` and a :pep:`440` version, which holds no ``*``, ``?`` or ``[``. The ``conda`` legs run it before setup-python, so ``python3`` is the image's, diff --git a/util/release_tiers.py b/util/release_tiers.py index b79058f7a5..65756d7441 100644 --- a/util/release_tiers.py +++ b/util/release_tiers.py @@ -18,17 +18,17 @@ ``::error`` and status 1. A tag on this run's commit counts this run's, as they are the same. -The step formerly held this as shell under ``set -euo pipefail`` and the -runner's ``bash -e``, and this keeps its outcomes: a ``git``, ``mktemp`` or +It keeps the outcomes of a shell step under ``set -euo pipefail`` and the +runner's ``bash -e``: a ``git``, ``mktemp`` or ``python3`` that fails ends the script with its status, but for the first look for the tag, which a fetch follows, the looks for the tier file and the helper, and the read of ``HEAD``, which only compares; a command missing from ``PATH`` is status 127; a file that does not open for writing is status 1; a variable it reads that is unset, which Actions never leaves it, is ``: unbound variable`` and status 1; a command killed by a signal is ``128 + N``, without -bash's own report of the kill. The outputs are written as the shell's ``echo`` -wrote them, a line each, and the scratch directory is left behind as ``mktemp --d`` left it. +bash's own report of the kill. The outputs are written as ``echo`` writes +them, a line each, and the scratch directory is left behind as ``mktemp -d`` +leaves it. It runs on setup-python's ``python3``, the interpreter it runs the tag's reader on, but keeps to the standard library and Python 3.8's syntax as the @@ -62,11 +62,11 @@ class Unbound(Exception): - """A variable read as ``$NAME`` that is unset, which ``set -u`` refused.""" + """A variable read as ``$NAME`` that is unset, which ``set -u`` refuses.""" class Failed(Exception): - """A command whose failure ended the shell under ``set -e``, with its status.""" + """A command whose failure ends a shell under ``set -e``, with its status.""" def __init__(self, status: 'int') -> 'None': super().__init__(status) @@ -120,12 +120,12 @@ def last_line(output: 'bytes') -> 'bytes': def say(text: 'bytes') -> 'None': - """Write ``text`` and a newline to standard output, as ``echo`` did.""" + """Write ``text`` and a newline to standard output, as ``echo`` does.""" _actions.emit(text + b'\n') def refused(path: 'bytes', error: 'OSError') -> 'int': - """Say why a redirection to ``path`` did not open, as bash did; status 1.""" + """Say why a redirection to ``path`` did not open, as bash does; status 1.""" sys.stdout.flush() print(f'{os.fsdecode(path)}: {os.strerror(error.errno or 0)}', file=sys.stderr, flush=True) return 1 diff --git a/util/release_upload.py b/util/release_upload.py index ff3895a88c..df49ed287d 100644 --- a/util/release_upload.py +++ b/util/release_upload.py @@ -21,11 +21,11 @@ Update a release, where GitHub re-checks the release's target against the workflow-file rule (#1634). A missing release fails the view, before any upload. -Each line's words, split on spaces and tabs, are expanded as the former shell's -unquoted ``$pattern`` expanded them: a word holding ``*``, ``?`` or ``[`` stands -for the files it matches, sorted, and for itself where it matches none; a word -that is not then a regular file gets an ``::error`` and exits 1. A failing -``gh`` ends the script with its status, as ``set -e`` ended the step. +Each line's words, split on spaces and tabs, are expanded as an unquoted shell +``$pattern`` expands them: a word holding ``*``, ``?`` or ``[`` stands for the +files it matches, sorted, and for itself where it matches none; a word that is +not then a regular file gets an ``::error`` and exits 1. A failing ``gh`` ends +the script with its status, as ``set -e`` would end a shell step. The script is standard library only, and keeps to Python 3.8's syntax. @@ -78,7 +78,7 @@ def gh(*args: 'str', capture: 'bool' = False) -> 'Tuple[int, str]': def states(published: 'str') -> 'Dict[str, str]': """``{name: state}`` of ``published``, the view's lines; a name listed twice keeps its last state. - As ``IFS=$'\\t' read -r asset asset_state`` split each line: the tabs around + As ``IFS=$'\\t' read -r asset asset_state`` splits each line: the tabs around it dropped, the name up to the next tab, and the state the rest, its leading tabs dropped. @@ -91,7 +91,7 @@ def states(published: 'str') -> 'Dict[str, str]': def expand(pattern: 'str') -> 'List[str]': - """The files one line of ``$PCAPKIT_ASSETS`` names, as the shell's unquoted ``$pattern`` gave them.""" + """The files one line of ``$PCAPKIT_ASSETS`` names, as an unquoted shell ``$pattern`` gives them.""" files = [] # type: List[str] for word in re.split(r'[ \t]+', pattern): if not word: diff --git a/util/run_unittest_leg.py b/util/run_unittest_leg.py index adc21ea210..baf51a2b57 100644 --- a/util/run_unittest_leg.py +++ b/util/run_unittest_leg.py @@ -34,8 +34,8 @@ A cheaper alternative exists, and is recorded here rather than left undocumented: running ``pytest --noconftest`` over just those same modules, in that order, disables the very same autouse fixture directly and -reproduces the identical ``subTest`` failures in about 70s on the pre-fix -tree, under the exact :program:`pytest` version this CI already installs. +reproduces the identical ``subTest`` failures in about 70s on a tree without +#981's fix, under the exact :program:`pytest` version this CI already installs. Whether that single invocation would have been sufficient instead of this dedicated runner and its per-directory matrix was not evaluated when this script was written; this paragraph records that gap rather than inventing a @@ -50,7 +50,7 @@ :file:`.github/workflows/unit-tests.yml`'s ``unittest-ordering`` job calls once per entry of its own matrix -- the leg list lives in that workflow, not as a module-level constant here. :mod:`tests.protocols` is the largest leg; -its CI timings, before and after it was sharded, are in that workflow's own +its CI timings, whole and sharded, are in that workflow's own comment rather than restated here. At the other end sit :mod:`tests.dumpkit` and :mod:`tests.interface`, small legs within a few seconds of each other in wall time (~39s and ~38s). Neither small leg is @@ -75,8 +75,7 @@ any purging module in that directory a chance to desync a root module that assumes its own import is current -- matching the reproduction exactly for ``protocols`` and ``const``, and giving the same opportunity to every other -directory this script is pointed at, most of which have never been tried in -that configuration before. +directory this script is pointed at. GitHub issue #1029 -- sharding a leg: ``directory`` may be a deeper path such as ``protocols/internet``, and ``--exclude SUBDIR`` (repeatable) drops a diff --git a/util/superseded.py b/util/superseded.py index 457baabe19..295fd84f74 100644 --- a/util/superseded.py +++ b/util/superseded.py @@ -33,7 +33,7 @@ line and ``stopped=true`` to ``$GITHUB_OUTPUT``, and exits 0: a stopped pytest writes no JUnit file, so the upload after it reads the output and expects none (#2073). Otherwise it exits with ``COMMAND``'s own status, ``128 + N`` for a signal ``N`` as a shell reports it, so a test -failure on a current commit reads as it did without the watcher. SIGINT, SIGTERM and SIGHUP sent to the +failure on a current commit reads as it would without the watcher. SIGINT, SIGTERM and SIGHUP sent to the watcher are passed on to ``COMMAND``. Every wait is bounded. Where ``COMMAND`` outlives SIGKILL by ``--kill-wait`` diff --git a/util/update_python_versions.py b/util/update_python_versions.py index 3cad3ec24d..ea7aa95285 100644 --- a/util/update_python_versions.py +++ b/util/update_python_versions.py @@ -265,7 +265,7 @@ def markdown_tables(text: 'str') -> 'list[list[list[Any]]]': Parsed with markdown-it-py (#1894), imported here for the reason :func:`fetch` imports :mod:`requests` there. An escaped pipe, ``\\|``, stays inside its cell, in a code span too, as GitHub renders it; splitting each - line on ``|`` cut such a cell in two and shifted every column after it. + line on ``|`` would cut such a cell in two and shift every column after it. """ from markdown_it import MarkdownIt # pylint: disable=import-outside-toplevel