Drop wheel, setuptools and pytest from runtime dependencies - #320
Open
fwittreverce wants to merge 2 commits into
Open
Drop wheel, setuptools and pytest from runtime dependencies#320fwittreverce wants to merge 2 commits into
fwittreverce wants to merge 2 commits into
Conversation
None of the three is imported anywhere under `src/`. `c2pa.py` and `lib.py` import only the standard library; `build.py` — the `download-artifacts` console script — imports `requests` and, lazily, `toml`. Those two stay. They are also already classified correctly elsewhere in the repo: * `[build-system] requires` already lists `setuptools>=68.0.0` and `wheel`, so the build has what it needs and the runtime entries are duplicates. * `requirements-dev.txt` lists `wheel` and `setuptools` under "# Build dependencies" and `pytest` under "# Testing dependencies". * `.github/workflows/build.yml` installs pytest explicitly (`pip install pytest`, lines 285 and 377), so CI does not rely on the runtime declaration either. Removing them is therefore a no-op for this repo's own build and test paths, and it keeps three packages out of every consumer's production environment.
Collaborator
|
For further review and consideration, please make sure to sign the Adobe CLA. You will likely need to close and reopen the PR for the job to pass. |
Dropping pytest from `[project.dependencies]` left it undeclared in pyproject.toml entirely, with `requirements-dev.txt` as the only manifest naming it. `[dependency-groups] dev` states it where it belongs: installed for contributors (`uv sync`, `pip install --group dev`) and, unlike `[project.optional-dependencies]`, absent from the published package metadata — which is the separation this branch is about. The bound matches requirements-dev.txt (`pytest>=8.1.0`) rather than the `>=7.4.0` the runtime entry carried; nothing installs the old one. The comment above the remaining dependencies goes with it. The rationale for keeping `toml` and `requests` belongs in the pull request, not in a manifest that has carried no comments so far.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wheel,setuptoolsandpytestare declared in[project.dependencies], soevery consumer installs three build/test packages into production environments.
None of the three is imported anywhere under
src/.pytestis re-declared as a PEP 735[dependency-groups] deventry rather thandropped outright, so the manifest still names the test dependency —
uv syncand
pip install --group devpick it up — while keeping it out of the publishedpackage metadata, which
[project.optional-dependencies]would not do. Thebound follows
requirements-dev.txt(>=8.1.0) rather than the>=7.4.0theruntime entry carried; nothing installs the older one today.
Evidence
Every import in the installed package, on
main(0.37.9):src/c2pa/c2pa.pysrc/c2pa/lib.pysrc/c2pa/__init__.pysrc/c2pa/build.pyrequests(line 16),toml(line 97, lazy)So
requestsandtomlare genuine runtime dependencies —build.pyis theinstalled
download-artifactsconsole script — and this PR leaves both alone.wheel,setuptoolsandpytestare imported by nothing.This repo already classifies all three correctly in three other places:
[build-system] requiresalready listssetuptools>=68.0.0andwheel,so the build has what it needs; the runtime entries are duplicates.
requirements-dev.txtlistswheelandsetuptoolsunder# Build dependenciesandpytestunder# Testing dependencies..github/workflows/build.ymlinstalls pytest explicitly(
pip install pytest, lines 285 and 377), so CI does not rely on the runtimedeclaration either.
The change is therefore a no-op for this repo's own build and test paths.
Why it is worth doing
They reach production images. In our worker,
uv export --frozen --no-dev—the resolver's production set — lists all three.
setuptoolsin particular hasa CVE history, so a security scan has to triage findings for packages the
application never imports.
They mask missing test dependencies downstream, silently. This is the one
that cost us time. Our own test extra was never actually installed —
uv syncdoes not install extras — and nobody noticed for seven weeks, because
pytestarrived through this dependency chain anyway. Our suite ran on a package no
manifest of ours declared, at a version nobody chose, and a fix we shipped in
that window was inert the whole time. A dependency audit is what eventually
flagged it.
One question, deliberately not in this diff
cryptographyis also unimported undersrc/— it appears only inexamples/and
tests/, and your ownrequirements.txtsays# only used in the training example. It looked like your call rather than mine: dropping it would stoppip install c2pa-pythonfrom giving a reader everything the signing examplesneed. Happy to extend the PR if you would rather it went too.