Repository navigation
design: write down what belongs in the all extra, and why #910
Description
Is your feature request related to a problem? Please describe.
The all extra has no written inclusion criterion. It began, in the maintainer's words on #899, as "originally for all deps (when we only had 3 3p engines and crawler deps)", and the library has since grown past that. Today it is a curated list of eight requirements whose three exclusions each rest on a different ad-hoc justification, so a contributor adding a new extra has nothing to check their decision against.
Measured on origin/main:
ALL DPKT ['dpkt']
ALL PyPCAPFile ['pypcapfile']
ALL PyShark ['pyshark']
ALL Scapy ['scapy']
ALL cli ['emoji']
ALL crypto ['cryptography>=3.4']
ALL vendor ['requests', 'beautifulsoup4']
EXCLUDED NGAP ['pycrate'] -- size (~238 MB) + LGPL-2.1+ vs BSD-3-Clause
EXCLUDED PCAP_CT ['pcap-ct', 'libpcap'] -- published only as pre-releases
EXCLUDED PyPCAP ['pypcap'] -- sdist-only C extension, needs a compiler
EXCLUDED docs [Sphinx, furo, ...] -- not a runtime feature
partial test [pytest, pytest-xdist, isort, ...] -- only requests/beautifulsoup4 reach `all`
Three different reasons for three exclusions, and no rule that predicts them. Size, licence, release maturity and build-toolchain requirements are all cited somewhere in pyproject.toml's comments, each argued locally and at length. Nothing says which of those disqualify an extra in general.
Describe the solution you'd like
Decide and write down the criterion — not merely a revised list. The list follows from the rule; a list without one drifts again the next time an extra is added. Candidate axes the current comments already appeal to:
- Licence — should
allever pull something less permissive than this package's BSD-3-Clause?pycrateis excluded partly for being LGPL-2.1+. - Size — is there a threshold?
pycratelands ~238 MB for one 4.9 MB module, a 50x multiplier over the useful part. - Release maturity —
pcap-ctandlibpcapare excluded for being pre-release only. - Build requirements —
pypcapneeds a compiler and libpcap headers, and fails where only the header is present. - Audience — is
all"every runtime feature an end user might want", or "everything"?vendor(crawler deps) is in,testtooling is mostly out,docsis out. That boundary is real but unstated.
Then reconcile two specific things the audit will surface:
allduplicates other extras' contents inline rather than referencing them.requests[socks]/beautifulsoup4[html5lib]appear in bothvendorandallas literals, so the two drift independently — exactly what happened on fix(ngap): source ProcedureCode/ProtocolIE from pycrate, stop the shared -1 mint (#880) #899, where addingpycratetovendordid not put it inalland the workflow had to ask for both.testis partial, which is almost certainly right but is currently an accident of which names happen to overlap rather than a decision.
Describe alternatives you've considered
Leaving it and deciding case by case. That is the status quo and it is what produced three incompatible justifications; it also means each new extra costs a fresh argument.
Additional context
Raised by the maintainer on #899: "I think that extra might need some revisit to determine what should fit in and what not … Now that the library grew, a new audit on it should be done."
Answering the narrower question he asked there: cryptography>=3.4 is already fully in all via the crypto extra, so crypto needs no change. pycrate is the live question, and its documented exclusion reasons still hold — adding it would make pip install pypcapkit[all] pull an LGPL-2.1+ dependency silently, which is the specific hazard pyproject.toml's own comment names.
#899 itself is unaffected and needs no change for this: it only touched vendor, and all still excludes pycrate as intended.
Metadata
Metadata
Assignees
Labels
Projects
- StatusShow more project fieldsDone
Activity
JarryShaw commentedon Sep 29, 2026
My preference is that
allmeans everything that an end user may want or need, like crypto and pycrate as they are addon-alike musts for the library to function at full functionality.JarryShaw commentedon Sep 29, 2026
Ruling recorded, verbatim:
That reverses my recommendation and I accept it — I argued for leaving
pycrateout. "Full functionality" is a coherent criterion and it is the one the extra's name implies, which is more than the current list can say for itself.One consequence stated once, because your own
pyproject.tomlargued it at length and it should be a decision rather than a side effect:pip install pypcapkit[all]will now pull an LGPL-2.1+ dependency into a BSD-3-Clause package's "everything" extra, and ~238 MB ofpycrate_asn1dirwith it. The comment on theNGAPextra puts the objection as "somethingpip install pypcapkit[all]pulls in silently: LGPL carries obligations BSD does not". Under this ruling that trade is accepted deliberately — which is fine, and it is worth the comment saying so explicitly rather than being left contradicting the code. I will have that rewritten as part of the work rather than left to be discovered.The narrower question your ruling does not settle, and it needs your word before the criterion can be written down. Two extras remain excluded, and their reason is installability, not desirability:
PyPCAP—pypcapis an sdist-only C extension. It needs a compiler and the libpcap development headers, and per the existing comment it "fails where only the header is present -- as on the macOS runners".PCAP_CT—pcap-ctandlibpcapare published only as pre-releases, so including them meansallsilently installs a beta.Both are engines an end user might well want, so under "everything an end user may want or need" they qualify by desire. But including them makes
pip install pypcapkit[all]either fail outright on a machine without a toolchain, or install a pre-release nobody asked for.So: does the criterion read as "everything an end user may want", or "everything an end user may want that installs unattended on a supported platform"? The second keeps those two out for a stated reason rather than an ad-hoc one, and is the only version I can write down as a rule that predicts the current list plus your
pycratedecision. Keepingneeds: decisionfor this alone.Not blocked on it: adding
pycratetoalland reconciling the comments can proceed on your ruling as given.And it makes a comment on the just-merged #899 wrong in a second way.
pyproject.toml:202-210reads "a developer opting intovendorhas already accepted a far heavier and more permissively-licensed dependency set than an end user installingallever would." I had already flagged "more permissively-licensed" as backwards — LGPL-2.1+ is less permissive than BSD-3-Clause. Under this ruling the rest of the sentence is false too, sinceallwill carrypycrateas well. The whole clause goes rather than getting a one-word fix.JarryShaw commentedon Sep 29, 2026
I say we only keep core addons in
all, meaning only CLI, crypto and pycrate to date.vendorextra keeps to the vendor crawler and they're not used for end user.3P engine extras same, only on demand for each one.
Will need to update all scripts that references/uses extras.
JarryShaw commentedon Sep 29, 2026
Ruling recorded, verbatim:
That is a sharper criterion than your first answer and it changes the shape rather than just adding
pycrate. Your earlier wording was "everything that an end user may want or need", which the four third-party engines arguably satisfy; "core addons only" excludes them. Measured —allgoes from 8 requirements to 3:You are right that scripts need updating, and here is the list — seven workflow sites plus three test modules and two docs pages:
Those three test sites assert
all's contents — e.g._dependency_gates.py:126: ".[all]carriespypcapfile,pysharkandscapy, so a guard…". They will fail, and correctly so; they encode the current shape and move with it.The finding that needs your word, because it is the one thing the ruling does not resolve:
allis currently doing double duty as "the CI install", and those are different sets.lint.yml:178-181says so in its own words:So removing
scapy,dpktandpysharkfromallmeans lint starts reporting unresolved imports for them — its comment already tracks exactly 8import-errormessages from the two engines that are absent by marker, and this would add more.deploy-pages.yml:111has the same dependency: the docs build already emits "dependency not found" notices for missing optional packages, and autodoc would gain more.So the ask: should CI get its own extra — a
devorciset naming everything the toolchain needs to resolve imports — or should each workflow name the extras it needs explicitly (.[all,Scapy,DPKT,PyShark]and so on)? The first is one new extra and seven one-line edits; the second is more verbose but makes each workflow's needs self-documenting, and avoids inventing a second catch-all that drifts the wayalljust did.I lean a
devextra, because "everything the linters and docs build need to see" is a real, stable set and naming it once is what stops the next drift. But it is your call, and it is the difference between seven edits and seven longer edits.needs: decisionkept for that alone. Everything else in the ruling is unambiguous and the work can start on it.JarryShaw commentedon Sep 29, 2026
Sure dev extra it is.
JarryShaw commentedon Sep 29, 2026
Ruling recorded, verbatim:
So
allnarrows to the core addons and a newdevextra carries what the toolchain needs to resolve imports. That settles every open point on this issue —needs: decisionoff, worker dispatched.The shape, as ruled:
Seven workflow sites move to
.[dev](or.[all,dev]where both are genuinely wanted), and the worker has been told to decide per site rather than substituting blindly:Three test sites assert
all's contents and will fail correctly — they encode the current shape and move with it:tests/_dependency_gates.py:126and:1407,tests/test_tier_guard.py:1278. Each says something like ".[all]carriespypcapfile,pysharkandscapy", which stops being true.Two docs pages describe the old
alland need updating:docs/source/index.rst:312and:322,docs/source/pep.rst:539-541. Notepep.rst:539currently explains whypypcapis excluded fromall— that reasoning survives, but its framing ("otherwisepip install pypcapkit[all]would demand a compiler") now applies todevas well ifdevcarries the engines.The judgement I have asked the worker to make explicitly, because it is the one place this could go wrong:
lint.yml's comment says its findings "depend on what is importable", and tracks exactly 8import-errormessages from the two engines already absent by Python-version marker. Ifdevcarries the engines, that count should stay at 8; if it does not, lint gains newimport-errorfindings and the comment becomes wrong. Sodevhas to be defined by what the toolchain must see, and the 8-message count is the check that it was got right.Worker briefed to report the before/after
import-errorcount rather than assume, and to leavepypcap/pcap-ctout of everything — your ruling keeps engine extras on demand, and those two additionally break or beta-install, which is why they were never inall.JarryShaw commentedon Sep 29, 2026
PR up: #914 (
b1535e7f3), implementing both rulings. Cross-review dispatched on a different model, briefed to falsify.What landed, against your words "only keep core addons in
all, meaning only CLI, crypto and pycrate to date":vendoris unchanged andpypcap/pcap-ctare untouched, so nothing moved that you did not name. Thedevextra exists because pylint, mypy and autodoc all resolve imports against what is installed, so narrowingallalone would have made four packages newly unresolvable to them — the three workflows that need full resolution now install.[all,dev], andlint.yml's trackedimport-errorcount is claimed to stay at exactly 8.Two things I am having the reviewer attack rather than take on trust. First, whether a
[all]-only user silently loses four engines —dpkt,scapy,pysharkandpypcapfilewere inalland are not now, so the question is whetherengine='dpkt'warns and falls back or raises, and whether that is documented anywhere a user reads. That is the one genuine user-visible consequence of the ruling and it needs to be deliberate. Second, two install sites increate-release.ymlandcron-conda.ymlwere left at bare.[all]on the reasoning that nothing downstream consumes them; if that is wrong it only shows on a real release.The PR also swept stale prose the ruling invalidated —
README.md,docs/source/pep.rst,docs/source/index.rst, and theNGAP/vendorextra comments inpyproject.toml, which both assertedpycratewas deliberately kept out ofall.One measurement I could not reconcile and am not relaying as settled: the author reports a Sphinx baseline of 42 warnings where two other measurements of the same commit give 58. I confirmed 58 myself.
make htmladds no flags andPCAPKIT_DEVMODEis read only bypcapkit/utilities/logging.py, never byconf.py, so neither explains it; missing optional imports account for only 6 of the 16. The reviewer is settling the cause. The delta is 0 either way, which is what the PR actually needs.ci(deps): narrow the all extra to core addons, add a dev extra (#910)
ci(deps): narrow the all extra to core addons, add a dev extra (#910)
ci(deps): narrow the all extra to core addons, add a dev extra (#910) (…
3 remaining items
docs(contributing): add the process conventions page (#918)
docs(contributing): add the process conventions page (#918)
docs(contributing): add the process conventions page (#918)
docs(contributing): add the process conventions page (#918)
docs(contributing): add the process conventions page (#918)
docs(contributing): add the process conventions page (#918)
docs(contributing): add the process conventions page (#918)
docs(contributing): add the process conventions page (#918)
docs(contributing): add the process conventions page (#918)
docs(docs): sweep the Sphinx pages for stale claims, toctree order an…
fix(conda): vendor only the needed dependencies and ship their data f…
fix(conda): vendor only the needed dependencies and ship their data f…