diff --git a/.github/workflows/cron-vendor.yml b/.github/workflows/cron-vendor.yml index 2b30994bb8..a4733b47e1 100644 --- a/.github/workflows/cron-vendor.yml +++ b/.github/workflows/cron-vendor.yml @@ -121,7 +121,15 @@ jobs: export PCAPKIT_CI_MODE=1 pcapkit-vendor - isort -l100 -ppcapkit pcapkit/const/*/*.py + # `find -mindepth 2`, not a `pcapkit/const/*/*.py` shell glob (#765): + # this job runs on `macos-latest`, whose default `bash` on PATH is + # the system's, unlike the Makefile's own `SHELL := $(shell command + # -v bash ...)`, which is confirmed (elsewhere) to pick up a modern + # one -- `shopt -s globstar` is not a safe assumption here, and + # `find` needs neither globstar nor a modern bash to reach any + # depth. `-mindepth 2` keeps the same floor the old glob had: it + # still skips `pcapkit/const/__init__.py` itself. + isort -l100 -ppcapkit $(find pcapkit/const -mindepth 2 -name '*.py') - name: Verify Changed files uses: tj-actions/verify-changed-files@v17 @@ -134,8 +142,8 @@ jobs: python util/bump_version.py isort -l100 -ppcapkit --skip-glob '**/__init__.py' pcapkit - isort -l100 -ppcapkit pcapkit/const/*/*.py - isort -l100 -ppcapkit pcapkit/vendor/*/*.py + isort -l100 -ppcapkit $(find pcapkit/const -mindepth 2 -name '*.py') + isort -l100 -ppcapkit $(find pcapkit/vendor -mindepth 2 -name '*.py') - name: Commit changes if: steps.verify-changed-files.outputs.files_changed == 'true' diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 41f767ef1c..7236c484f4 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -22,29 +22,83 @@ name: "Lint" # bandit 8 findings (7 medium, 1 low, 0 high) exit 1 [932cb48d1] # mypy 112 errors in 38 files (506 checked) exit 1 [932cb48d1] # vermin 106 files flagged; needs 3.11, targets 3.6 in vermin.ini exit 1 [932cb48d1] -# pylint 90 E, 4765 W, 542 C -- stable; R and the total are not, see below exit 30 [932cb48d1] +# pylint 90 E, 4765 W, 542 C -- stable; R and the total are not, see below exit 30 [1a852698b] # -# pylint's R and total are deliberately not pinned to a single number: repeated -# runs on the identical tree at 932cb48d1 gave R of 645-680 and totals of -# 6044-6079 (two runs measured here: 659/6058 and 680/6079; a cross-review -# measured four more: 662/647/647/645, totals 6061/6046/6046/6044), entirely -# from `R0401` cyclic-import, whose count depends on module-processing order -# rather than on the code -- `duplicate-code` (400) and every other R check -# held steady across all of them. Separately, the job's own grep-based count -# (the `pylint` step below) and this header can legitimately disagree on the -# *same* run: four of `PYLINT_FLAGS`' `--disable=` entries name checks pylint -# has since removed or renamed (`old-division`, `no-absolute-import`, -# `input-builtin` -> `R0022 useless-option-value`; `eq-without-hash` -> -# `W0012 unknown-option-value`), plus one plugin in `--load-plugins=` that no -# longer exists in the installed pylint (`pylint.extensions.emptystring` -> -# `E0013 bad-plugin-value`, see #767) -- five stale flags in all, and pylint -# attributes every one of them to `Command line` rather than to a file. -# Whether a count includes them is a choice, not a fact about the tree -- C -# above is untouched either way (none of the five is class C); E and W above -# both exclude their one Command-line message each (91 and 4766 with them, 90 -# and 4765 without -- pylint's own `messageTypeCount` agrees: 90/4765/542); -# R's swing above already includes its three either way, dwarfed by -# `R0401`'s own. +# pylint's R and total are deliberately not pinned to a single number. Before +# #767 below, repeated runs on the identical tree gave R of 645-680 and totals +# of 6044-6079 at 932cb48d1 (two runs measured here: 659/6058 and 680/6079; a +# cross-review measured four more: 662/647/647/645, totals 6061/6046/6046/6044), +# entirely from `R0401` cyclic-import, whose count depends on module-processing +# order rather than on the code -- `duplicate-code` (400) and every other R +# check held steady across all of them. That range's own basis is not this +# line's, though: every one of those runs predates #767, so each *R* already +# carries the three `R0022` the old flags spent on `Command line` (not just +# the total -- `R0022` is class R), and each *total* carries one `E0013` and +# one `W0012` on top of that: five stale messages in all, `91 + 4766 + 542 + +# R_old` where `R_old` is itself `R_real + 3`. On the corrected basis +# (`90 + 4765 + 542 + R`) that range is really R 642-677, total 6039-6074 -- +# not R 645-680, total 6044-6079 as printed above the line predating #767. +# +# After #767, re-measured on that corrected basis at 1a852698b: two runs here +# gave R 662 and 648 (coincidentally sharing a digit with one of the four +# pre-#767 cross-review runs above, which is a different measurement on a +# different basis, not a repeat of it), totals 6059 and 6045, and a +# cross-review measured five more -- 642/643/646/649/659, totals +# 6039/6040/6043/6046/6056. Combined: R 642-662, total 6039-6059 +# [1a852698b]. The floor matches the pre-#767 range's own corrected floor +# exactly (642 both ways) because it is the same tree state either side of +# that diff -- the range only looks lower now because three of its messages +# have been deleted, not because the code changed. Already superseded the +# same day it was measured, too: a cross-review measured `R0801 +# duplicate-code` alone moving 400 -> 573 on `merge(f31e5114f, 83b58ebda)` -- +# this branch merged onto the `main` it was rebased against before #769 +# landed there -- taking R to 823 and the total to 6220 on that merge. Still +# true of `main` itself today, since this PR touches no `pcapkit/` file, but +# named for the tree it was actually measured on rather than asserted of +# `main` directly. This range is a moving target to re-measure, never a +# constant to defend. +# +# The pin above moved from 932cb48d1 to 1a852698b because `pcapkit/` is not +# actually unchanged between them, despite what an earlier revision of this +# comment claimed: four files moved (`pcapkit/const/reg/apptype/apptype.py`, +# `pcapkit/foundation/extraction.py`, `pcapkit/protocols/schema/schema.py`, +# `pcapkit/vendor/reg/apptype/apptype.py`; tree 7270ad50 -> 2b9ac808), #764's +# port-validation logic among them. E, W and C read identically either side of +# that diff -- 90/4765/542 both times -- but 1a852698b is the tree actually +# re-measured for this line, so that is what it is pinned to now, rather than +# repeating a byte-identity claim that does not hold. +# +# Four of `PYLINT_FLAGS`' `--disable=` entries named checks pylint has +# genuinely removed -- `old-division`, `no-absolute-import` and +# `input-builtin` (-> `R0022 useless-option-value`, and pylint's own message +# says so: "was removed from pylint") -- plus `eq-without-hash`, which is not +# gone, only unloaded: `pylint.extensions.eq_without_hash` still ships `W1641`, +# but nothing in `--load-plugins=` ever named it, so `--disable=eq-without-hash` +# was as unrecognised as if the check had been removed (-> `W0012 +# unknown-option-value`). Same shape of breakage as `emptystring` below, a +# different cause. All four are dropped either way -- loading the fifth plugin +# to enable `eq-without-hash` properly would find nothing today (`W1641` is 0 +# over `pcapkit/`), so there is nothing lost in dropping it instead. +# +# `--load-plugins=` also named `pylint.extensions.emptystring`, which no +# longer exists in the installed pylint (-> `E0013 bad-plugin-value` every +# run). Unlike the four above, though, the check it used to provide did not +# stop running when the plugin did: `compare-to-empty-string` was folded into +# the core `refactoring` checker at pylint 3.0 as +# `use-implicit-booleaness-not-comparison-to-string` (C1804), reachable under +# either name with no plugin load at all. #767's own premise -- that the +# broken plugin reference made the check "inert" and it "has never run" -- is +# wrong: `--list-msgs-enabled` run under the old flags (broken plugin +# reference included) and the new ones side by side gives the same 399-line +# enabled-message list either way, and C1804 fires under the old flags on a +# minimal repro despite the `E0013`. So this half of `PYLINT_FLAGS`' change is +# flag hygiene, not enablement: it drops a dead plugin reference and keeps the +# check it used to gate under that check's current name, and the only thing +# that actually changes is that `E0013` (and the other four's Command-line +# noise) stops firing. With the five gone, the job's own grep-based count (the +# `pylint` step below) and this header have nothing left to disagree about on +# the same run -- the figures above are both the raw and the file-scoped +# count, for E, W and C alike. # # None of the four is clean, so none can block today: a job that is red the day # it lands teaches everyone to scroll past red, which costs more than the checks diff --git a/Makefile b/Makefile index 07437b154b..33608b608b 100644 --- a/Makefile +++ b/Makefile @@ -5,9 +5,17 @@ export PIPENV_CACHE_DIR ?= $(CURDIR)/.pipenv-cache export PIP_CACHE_DIR ?= $(CURDIR)/.pip-cache export all_proxy= -# Recipes below use bash features (brace expansion), so bash is required; take it -# from PATH rather than a fixed prefix, as Homebrew, Linuxbrew and system installs -# all put it somewhere different. +# No recipe below currently 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 +# 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 today, and it costs nothing to take +# from PATH rather than a fixed prefix, as Homebrew, Linuxbrew and system +# installs all put it somewhere different. SHELL := $(shell command -v bash 2>/dev/null || echo /bin/bash) VERSION = $(shell cat pcapkit/__init__.py | grep "^__version__" | sed "s/__version__ = '\(.*\)'/\1/") @@ -123,7 +131,8 @@ docs-autobuild: isort: pipenv run isort -l100 -ppcapkit --skip-glob '**/__init__.py' pcapkit $(wildcard temp/sort.py) - pipenv run isort -l100 -ppcapkit pcapkit/{const,vendor}/*/*.py + pipenv run isort -l100 -ppcapkit $$(find pcapkit/const -mindepth 2 -name '*.py') + pipenv run isort -l100 -ppcapkit $$(find pcapkit/vendor -mindepth 2 -name '*.py') pipenv run isort -l100 -ppcapkit util/*.py examples/generators/*.py # The command prefix that puts the lint tools on PATH. Locally that is pipenv, as @@ -140,7 +149,21 @@ RUN ?= pipenv run # recipe has always read, and vermin de-duplicates the paths (it reports 496 # files analyzed either way), so it is preserved verbatim rather than tidied. VERMIN_FLAGS = --backport argparse --backport enum --backport importlib --backport ipaddress --backport typing --backport typing_extensions --no-parse-comments --eval-annotations -vv -PYLINT_FLAGS = --load-plugins=pylint.extensions.check_elif,pylint.extensions.docstyle,pylint.extensions.emptystring,pylint.extensions.overlapping_exceptions --disable=all --enable=F,E,W,R,basic,classes,format,imports,refactoring,else_if_used,docstyle,compare-to-empty-string,overlapping-except --disable=blacklisted-name,invalid-name,missing-class-docstring,missing-function-docstring,missing-module-docstring,design,too-many-lines,eq-without-hash,old-division,no-absolute-import,input-builtin,too-many-nested-blocks,broad-except,singleton-comparison,ungrouped-imports --max-line-length=120 --init-import=yes +# `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. +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/tests/project/test_isort_clean.py b/tests/project/test_isort_clean.py index a824195074..9383d614c8 100644 --- a/tests/project/test_isort_clean.py +++ b/tests/project/test_isort_clean.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -"""Pins ``make isort`` clean on all three of its lines (#757). +"""Pins ``make isort`` clean on all four of its lines (#757). #757 was filed from a reproduction that ran bare ``isort --check-only --diff pcapkit/protocols/schema/schema.py``, with none of the flags ``make isort`` @@ -15,21 +15,22 @@ ("120 for ``pylint`` and 100 for ``isort``, not PEP 8's 79"): a deliberate, written-down split, not a misconfiguration. Under *that* width the ``misc`` import is not flagged at all (89 < 100). What *was* flagged, on the commit -this test landed with, were three things line 125's flags alone would not -have caught: +this test landed with, were three things the whole-tree line's flags alone +would not have caught: * :file:`pcapkit/protocols/schema/schema.py` -- its ``pcapkit.utilities.warnings`` import was wrapped across two lines at some earlier width, and unwrapped it is 96 characters, under 100, so isort's real verdict was to collapse it back to one line rather than wrap another import. * :file:`examples/generators/dispatch.py` -- two function-local import blocks - isort wants a blank line after (line 127's targets, unreachable from line - 125's ``pcapkit`` argument). + isort wants a blank line after (the util/examples-generators line's + targets, unreachable from the whole-tree line's ``pcapkit`` argument). * :file:`examples/generators/options.py` -- an ``import ipaddress`` isort wants moved above the ``from pcapkit...`` block, and a ``from scapy.all import ...`` isort wants reordered by its constant/class split (``IP, TCP`` before - ``Ether, IPv6, Raw``). Also line 127 only; only import order changed, since - this module is loaded and run at test time, not just read at review time. + ``Ether, IPv6, Raw``). Also the util/examples-generators line only; only + import order changed, since this module is loaded and run at test time, + not just read at review time. Three test modules load it by path via ``importlib.util.spec_from_file_location``: :file:`tests/protocols/test_option_generator_tcp_base_unit.py`, @@ -40,16 +41,79 @@ directly, too. So this module does not re-litigate the width. It just pins isort's own -verdict on each of the three lines -- via real ``isort`` invocations with the +verdict on each of the four lines -- via real ``isort`` invocations with the Makefile's exact flags, not a hand-rolled guess about what those flags imply -- so a future edit that reintroduces an unsorted or mis-wrapped import anywhere ``make isort`` reaches fails here instead of surfacing as "clean locally, red -in CI" the way #757 did. Earlier revisions of this test covered line 125 -only (:file:`pcapkit`, skipping ``__init__.py``) and so missed exactly the -:file:`examples/generators/` files above and the ``__init__.py`` files that -sit under :file:`pcapkit/const/*/` or :file:`pcapkit/vendor/*/` -- line 126 has -no ``--skip-glob``, so it re-covers whichever of the 71 files line 125 skips -happen to live there, even though line 125 skips all 71 package-wide. +in CI" the way #757 did. Earlier revisions of this test covered the +whole-tree line only (:file:`pcapkit`, skipping ``__init__.py``) and so +missed exactly the :file:`examples/generators/` files above and the +``__init__.py`` files that sit at least one directory below +:file:`pcapkit/const/` or :file:`pcapkit/vendor/` -- the const and vendor +lines have no ``--skip-glob``, so between them they re-cover whichever of +the 71 files the whole-tree line skips happen to live there, even though +the whole-tree line skips all 71 package-wide. The const and vendor lines +used to be one, :file:`pcapkit/const/*/*.py` and :file:`pcapkit/vendor/*/*.py` +combined into a single brace-expanded glob that stopped at a fixed two +levels, which the #754 split outgrew the moment +:file:`pcapkit/const/reg/apptype/__init__.py` landed a level deeper still and +nothing sorted it any more (#765). + +The first fix tried here widened that one line to +``pcapkit/{const,vendor}/*/**/*.py`` under ``shopt -s globstar``, reaching +any depth below the first subdirectory. That was itself wrong, and in the +same shape as the bug it fixed: ``globstar`` needs bash >= 4.0, +:file:`Makefile`'s ``SHELL := $(shell command -v bash ...)`` only ever picks +*some* ``bash`` off ``$PATH``, and macOS ships 3.2 as ``/bin/bash`` with no +guarantee a newer one sits ahead of it. Without ``globstar`` the shell option +itself fails, ``shopt`` writes one line to stderr and returns non-zero, and +the *next* command in the recipe still runs under the plain, non-recursive +glob -- silently, since make does not stop a recipe line for a failed +``shopt``, only for the isort invocation that follows it. So on such a +machine that one line would have gone from reaching 264 files to reaching +12, sorting none of the 32 depth-two ``__init__.py`` files it used to cover +(the other two, three levels down under :file:`.../reg/apptype/`, stayed +reachable even by the degraded glob) -- the #765 hole reopened one level up, +by #765's own fix. + +The second fix replaced that with ``$$(find pcapkit/const -mindepth 2 -name +'*.py') $$(find pcapkit/vendor -mindepth 2 -name '*.py')``, both handed to +one ``isort`` invocation: ``find`` needs no particular bash version and no +shell option at all, and reaches any depth the same way. That closed the +bash-version gap but opened a narrower one of its own: merged into a single +invocation, only the case where *both* ``find`` calls come back empty is +loud -- ``isort`` then gets zero paths, exits 1 rather than reading +``stdin``, and make reports the recipe as failed because that command's own +status was non-zero, the same as it would for any other line (no ``set -e`` +needed for this, and none is set). If only *one* of the two came back empty, +though, the other's files would still reach ``isort`` in that same +invocation, which would exit 0 having quietly sorted half the tree. Neither +case is reachable today -- no path under :file:`pcapkit/const/` or +:file:`pcapkit/vendor/` contains whitespace to break the expansion, and +neither directory can be absent where ``make isort`` runs -- but it is the +same shape of latent gap #765 itself was, so the third fix, now in place, is +to split the one invocation into two: one call covers +:file:`pcapkit/const/`, the other covers :file:`pcapkit/vendor/`, matching +:file:`.github/workflows/cron-vendor.yml`'s own two calls exactly (below) and +leaving neither case above reachable even in principle. The leading +``-mindepth 2`` on each stays for the same reason the original glob's +leading ``*/`` did: dropping it would also reach +:file:`pcapkit/const/__init__.py` and :file:`pcapkit/vendor/__init__.py` +themselves, whose hand-grouped import order isort would flatten -- see +:func:`_const_targets` and :func:`_vendor_targets`. + +The identical two-level glob also lived in +:file:`.github/workflows/cron-vendor.yml`, on the path that actually *writes* +the deep files: ``pcapkit-vendor`` regenerates :mod:`pcapkit.const` from the +:mod:`pcapkit.vendor` crawlers, and that job's own ``isort`` calls -- already +two, one per directory -- were as blind to :file:`pcapkit/const/reg/apptype/` +as the Makefile's own line used to be. Fixed there too (#765), the same way +and for the same reason: that job runs on a ``macos-latest`` runner, so +relying on ``bash`` >= 4.0 would have been exactly the mistake the Makefile's +line first made. This module does not cover that fix, though: it only +exercises ``make isort``, and :file:`cron-vendor.yml`'s ``vendor-update`` job +never runs this test suite at all -- it commits and pushes on its own, with +no pytest step in between. ``isort`` is deliberately absent from both ``Pipfile`` and ``pyproject.toml``: :file:`.github/workflows/lint.yml` notes it stays local-only, unlike the other @@ -69,9 +133,10 @@ ROOT = pathlib.Path(__file__).resolve().parents[2] PACKAGE = ROOT / 'pcapkit' +MAKEFILE = ROOT / 'Makefile' -def _line_125_targets() -> 'list[str]': +def _pcapkit_tree_targets() -> 'list[str]': """``pcapkit $(wildcard temp/sort.py)`` -- the scratch file is untracked and normally absent, so this mirrors make's ``$(wildcard ...)`` by only adding it when it actually exists. @@ -84,35 +149,135 @@ def _line_125_targets() -> 'list[str]': return targets -def _line_126_targets() -> 'list[str]': - """``pcapkit/{const,vendor}/*/*.py``, expanded the way bash would.""" - targets = [] # type: list[str] - for sub in ('const', 'vendor'): - targets.extend(sorted(str(path) for path in (PACKAGE / sub).glob('*/*.py'))) - return targets +def _find_mindepth_2_targets(sub: str) -> 'list[str]': + """``$(find pcapkit/ -mindepth 2 -name '*.py')`` (#765) -- + ``Path.glob('*/**/*.py')`` matches the same set ``find -mindepth 2`` does: + at least one subdirectory below ````, then any depth. Pathlib's own + glob has no shell and no bash version to depend on in the first place, so + this mirror was never exposed to the bug the Makefile itself just had. + The leading ``*/`` (equivalently, the ``-mindepth 2``) is deliberate, not + slack -- a bare ``**/*.py`` would also reach :file:`pcapkit//__init__.py` + itself, whose hand-grouped, commented import order (``# base crawler``, + only under ``vendor``; ``# IANA registration``, ``# Miscellanous`` and + ``# per protocol`` under both) isort's ``-l100 -ppcapkit`` would flatten + into one alphabetical block, scattering the comments across it -- + confirmed with ``isort --diff``, not assumed. That file stays reached by + neither this call nor the whole-tree line (which skips every + ``__init__.py``), same as before #765. + + """ + return sorted(str(path) for path in (PACKAGE / sub).glob('*/**/*.py')) + + +def _const_targets() -> 'list[str]': + """``pcapkit/const/`` half of the pair described in + :func:`_find_mindepth_2_targets` -- its own line since #765's second fix + (see the module docstring), matching :file:`cron-vendor.yml`'s own + separate calls for ``const`` and ``vendor``. + + """ + return _find_mindepth_2_targets('const') + +def _vendor_targets() -> 'list[str]': + """``pcapkit/vendor/`` half of the pair described in + :func:`_find_mindepth_2_targets`. -def _line_127_targets() -> 'list[str]': + """ + return _find_mindepth_2_targets('vendor') + + +def _util_examples_targets() -> 'list[str]': """``util/*.py examples/generators/*.py``, expanded the way bash would.""" targets = sorted(str(path) for path in (ROOT / 'util').glob('*.py')) targets.extend(sorted(str(path) for path in (ROOT / 'examples' / 'generators').glob('*.py'))) return targets -#: One entry per line of the Makefile's ``isort:`` target: the flags that -#: precede the targets, and the callable that resolves those targets on this -#: checkout. Kept identical to those three lines on purpose -- a flag changed -#: in one place and not the other is exactly how "clean locally, red in CI" -#: (or the reverse) starts. -MAKEFILE_LINES = { - 125: (['-l100', '-ppcapkit', '--skip-glob', '**/__init__.py'], _line_125_targets), - 126: (['-l100', '-ppcapkit'], _line_126_targets), - 127: (['-l100', '-ppcapkit'], _line_127_targets), -} +def _isort_recipe_line_numbers() -> 'list[int]': + """The physical, 1-indexed line numbers of the Makefile's ``isort:`` + target's own recipe lines, computed fresh from :file:`Makefile` every + run rather than hardcoded -- a hardcoded number is exactly the kind of + silent rot this module exists to catch, and an earlier revision of this + file was guilty of it in its own name: functions called + ``_line_125_targets`` and so on stopped matching reality the moment a + :file:`Makefile` comment two lines above the target grew by a few + lines, and nothing here noticed until a reviewer measured it by hand. + None of the functions below are named after a line number any more for + that reason; :data:`MAKEFILE_LINES` carries the number as data instead, + paired with the callable it belongs to positionally rather than by a + key that could drift out of sync with what it claims to label. + + A recipe line is any line beginning with a tab that immediately follows + ``isort:`` or another such line, a blank line, or a comment line at + column 0 -- make itself treats a blank or a column-0 ``#`` line inside a + recipe as transparent rather than as ending the rule (measured with + ``make -n isort`` as the oracle: it still reports the same four commands + with either dropped in between two recipe lines, which an earlier + revision of this function got wrong by breaking on the first line that + was not tab-indented, comment or not). The first following line that is + genuine content and not tab-indented ends the target -- a new target + declaration, most likely, since nothing above passes that test. + + Matching ``isort:`` requires the *whole* line, not a prefix: an + ``isort-check:`` target, or a ``.PHONY: isort`` line naming ``isort`` as + a dependency rather than declaring it, must not be mistaken for the + target this function is looking for. The one case this does not defend + against is a ``define``/``endef`` block whose body contains a bare + ``isort:`` at column 0 followed by four or more tab-indented lines -- + nothing in this file uses ``define``, so it is undefended rather than + handled. + + """ + lines = MAKEFILE.read_text().splitlines() + numbers = [] # type: list[int] + in_target = False + for lineno, text in enumerate(lines, start=1): + if text == 'isort:': + in_target = True + continue + if not in_target: + continue + if text.startswith('\t'): + numbers.append(lineno) + elif not text.strip() or text.lstrip().startswith('#'): + continue + else: + break + return numbers + + +#: One entry per recipe line of the Makefile's ``isort:`` target, in the +#: order those lines appear: the flags that precede the targets, and the +#: callable that resolves those targets on this checkout. A plain list, not +#: a dict keyed by line number -- see :func:`_isort_recipe_line_numbers` for +#: why a line number is not safe to key anything on here. +#: +#: A flag changed in one place and not the other -- here or in the Makefile +#: -- is exactly how "clean locally, red in CI" (or the reverse) starts, +#: which is the motive for keeping this list matched to the Makefile at +#: all. The mechanism the test below actually has for that is narrower than +#: the motive, though: the length check catches a recipe *line* added or +#: removed without a matching entry here, nothing more. Flags and target +#: expressions are plain literals in this list, never read back out of +#: :file:`Makefile`, so a flag edited there (``-l100`` to ``-l120``, say) +#: is not something this file would notice at all -- pre-existing, and out +#: of scope for :func:`_isort_recipe_line_numbers` to fix. That asymmetry +#: is also what keeps a wrong line number cheap rather than dangerous: the +#: number only labels which real Makefile line a subtest's failure message +#: points at, and never decides which flags or targets the *test* actually +#: runs, so a line-number bug can misdirect where a human looks, but cannot +#: change what isort is asked to check. +MAKEFILE_LINES = [ + (['-l100', '-ppcapkit', '--skip-glob', '**/__init__.py'], _pcapkit_tree_targets), + (['-l100', '-ppcapkit'], _const_targets), + (['-l100', '-ppcapkit'], _vendor_targets), + (['-l100', '-ppcapkit'], _util_examples_targets), +] class TestIsortIsCleanOnThePackage(unittest.TestCase): - """``make isort`` must not be red on a clean checkout, on any of its three + """``make isort`` must not be red on a clean checkout, on any of its four lines (#757). """ @@ -131,7 +296,15 @@ def setUpClass(cls) -> None: def test_check_only_is_clean_on_every_makefile_line(self) -> None: """Each line of the Makefile's ``isort:`` target, run the way it runs.""" - for line, (flags, targets_fn) in sorted(MAKEFILE_LINES.items()): + line_numbers = _isort_recipe_line_numbers() + self.assertEqual( + len(line_numbers), len(MAKEFILE_LINES), + f"Makefile's isort: target has {len(line_numbers)} recipe line(s) " + f'now (at {line_numbers}), not {len(MAKEFILE_LINES)} -- ' + f'MAKEFILE_LINES needs an entry added or removed to match' + ) + + for line, (flags, targets_fn) in zip(line_numbers, MAKEFILE_LINES): with self.subTest(makefile_line=line): targets = targets_fn() self.assertTrue(targets, f'Makefile:{line} resolved to no targets on this '