Skip to content

ci(conda): move platform pins up and give them a review-by date (#1851) - #1885

Merged
JarryShaw merged 1 commit into
mainfrom
ci/1851-platform-pin-refresh
Oct 17, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
ci/1851-platform-pin-refresh

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

As ruled on #1851, this combines two changes: the weekly job moves platform pins up on its own, and each pin carries a review-by date.

  • New table file. The pins move from util/conda_vendor.py into conda/platform-pins.toml. Each [package.subdir] entry has version, review-by and reason, so the job rewrites a data file rather than its own source.
  • conda-update step. The job gains a --refresh-pins step before it writes conda/requirements.txt. It asks PyPI for the newest release, up to the installed one, that has a wheel for every conda Python on the pinned platform. The match uses packaging.tags and accepts abi3 and universal2.
    • If that release is newer than the pin, the pin moves up and gets a fresh date.
    • If it is the installed release, the pin is dropped.
    • A PyPI failure warns and leaves the pin unchanged.
  • Offline --check. --check stays offline and fails on a pin past its review-by date, reported like its other problems. The horizon is a quarter (REVIEW_HORIZON).
  • Older tests. The fix(conda): pin cryptography 48.0.1 on Intel macOS, which has no newer wheel #1827 and fix(conda): vendored dependencies ship without their data files #1814 tests now read the pin from the table and fix "today", so a pin moving or falling due does not turn every PR red.

tests/project/test_conda_platform_pin_refresh_1851.py fails 34 tests on the base, and fails for the matching reason under each single mutation. No test touches the network. A live --refresh-pins run against PyPI changed nothing, which is correct: no cryptography release after 48.0.1 has an Intel or universal2 macOS wheel.

Unverified: the step on a real runner.

Closes #1851

@JarryShaw JarryShaw added ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 17, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 1fa05b899. A Sonnet cross-review confirmed the core of the change:

  • Tag matching: checked against real cryptography release data and a 36,300-combination packaging.tags oracle. osx-64 resolves to 48.0.1, and every miss was conservative.
  • Bump and drop: behave as described.
  • Failures: every network and JSON failure leaves the table byte-identical and exits 0.
  • Wiring: the step sits in conda-update before the requirements writer.
  • Tests: 17 single mutations each fail the new tests.

Required:

  1. isort. util/conda_vendor.py:532 is a 117-character import that isort (-l100 -ppcapkit) wraps, so tests/project/test_isort_clean.py fails here and passes on the base. I reproduced it.
  2. Retiring a pin. A pin that already equals the installed release, with nothing newer, is never dropped; it sits until its review date. The pin is then redundant, so --refresh-pins should drop it, as it drops a pin it has just moved up to the installed release.

Smaller:

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 17, 2026
@JarryShaw
JarryShaw force-pushed the ci/1851-platform-pin-refresh branch from 1fa05b8 to 81d55de Compare October 17, 2026 03:26
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 81d55deb8. The Sonnet reviewer re-checked the changes since 1fa05b899.

Not blocking: no test pins that a pin above the installed release is kept. A >= mutation survives, though the code does keep it.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 90.05% (unit tier, Python 3.15, 81d55deb8, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 2 20 2 96.77%
pcapkit/const 19595 881 2618 867 92.11%
pcapkit/corekit 2462 69 780 35 96.55%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2914 164 1016 34 93.59%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17406 224 4580 189 98.10%
pcapkit/toolkit 872 177 298 8 77.18%
pcapkit/utilities 512 2 158 4 99.10%
pcapkit/vendor 4407 2342 1010 157 43.27%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Can we actually use requests and bs4[html5] for the scripts (not only the one changed in this PR but also other scripts in the utils and other places)? If reading from subprocess outputs, we can also introduce pexepct and psutils if applicable.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Yes for requests, and for psutil in one place. I found nothing for bs4[html5lib] or pexpect to replace. I checked every script outside pcapkit/:

  • requests fits the urllib fetches in util/conda_vendor.py (this PR's --refresh-pins), util/ci_timing.py, util/update_python_versions.py and examples/generators/pcapng.py.
  • psutil fits util/vendor_test_gate.py, which polls ps for the memory of the test run's process group.
  • bs4[html5lib]: no script outside the vendor crawlers parses HTML. What they parse is JSON, Markdown, JUnit XML and tshark's PDML.
  • pexpect: every subprocess they run is non-interactive (git, gh, ps, tshark, pytest).

This PR will move --refresh-pins to requests and install it in cron-conda.yml. The import stays inside the fetch, so --smoke still runs on the conda legs with only pip installed. #1894 covers the other scripts, each with the install step it needs.

I'd leave util/project_status.py as it is unless you say otherwise. It calls gh api rather than fetching anything itself. Its job runs on pull_request_target, holds PROJECT_TOKEN, and installs nothing on purpose, so a pip install there would widen what runs with the token.

- Move PLATFORM_PINS into conda/platform-pins.toml, so the weekly job can
  rewrite it; each pin carries a version, a review-by date and a reason.
- Add util/conda_vendor.py --refresh-pins: it reads PyPI's JSON, finds the
  newest release up to the installed one with a wheel every conda Python
  installs on the pinned platform, abi3 and universal2 included, and moves
  the pin there, due for review REVIEW_HORIZON out. A pin that reaches the
  installed release, or already pins it, is dropped. A PyPI failure warns
  and keeps the pin.
- Read PyPI through requests, imported only for that request so --smoke
  still needs only the standard library, with each requests error raised
  as OSError; conda-update installs requests, which .[all] does not bring.
- Run it in conda-update before the requirements are written.
- --check stays offline and now fails on a pin past its review-by date.
- Keep the table out of BOT_MANAGED_PATHS, pinned by a test, and correct
  that comment's claim about which files tests read.
- Read the pin from the table in the #1827 tests, and fix today in the
  #1814 --check tests, so a moved or overdue pin does not break them.

Closes #1851
@JarryShaw
JarryShaw force-pushed the ci/1851-platform-pin-refresh branch from 81d55de to 44fe6d4 Compare October 17, 2026 13:57
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Then maybe some libraries like lxml or markdown that can ease the parsing of XML and Markdown formats?

@JarryShaw

Copy link
Copy Markdown
Owner Author

Markdown: yes. parse_images() in util/update_python_versions.py:245-268 reads the runner-images README's table by splitting each line on |. An escaped pipe or a pipe inside a code span would shift its columns. A parser with GFM tables fixes that. markdown-it-py hands back the cells as tokens; markdown with its tables extension renders HTML, which bs4 would then read. I'll use markdown-it-py unless you prefer the other, and I've added this to #1894.

XML: I'd leave it. Both XML readers already use a real parser, xml.etree.ElementTree:

  • util/ci_timing.py:640-646 reads pytest's JUnit file;
  • util/pyshark_encap_map.py:338 reads tshark's PDML.

lxml exposes the same API, so the code would read the same, at the cost of a compiled dependency in the ci-timing job. I'd keep the standard library there unless you want lxml for XPath or its stricter parsing.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 44fe6d42a. A Sonnet re-review of the requests amend checked the following:

  • requests is imported only inside _fetch. Even with it blocked, --smoke exits 0 and imports none of its dependencies.
  • Failures don't crash the refresh. Against localhost fault servers, every one of 37 failure modes (bad status, bad encoding, truncated body, timeout, redirect loop, dead proxy) came out as an UNREADABLE error, so refresh() keeps the pin and warns.
  • cron-conda.yml installs requests before the refresh step. .[all] does not bring it in.
  • Every mutation it tried that changes the fetch or its install step is caught by a test.
  • The conda tests, tests/project and the tier guard pass.

One gap, not blocking: the workflow test would also pass if requests appeared only after a shell # on the install line.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

I kinda prefer lxml's strict parsing tbh. I recall it has wheels?

@JarryShaw

Copy link
Copy Markdown
Owner Author

It does. lxml 6.1.3 has wheels for these interpreters:

  • CPython 3.8 to 3.15, including the free-threaded 3.14t and 3.15t, on Linux, macOS arm64 and Windows;
  • PyPy 3.9 to 3.11, on Linux and Windows.

GraalPy has no wheel. 3.7 has none either, because lxml 6 needs 3.8, but that leg cannot compile pcapkit anyway.

So both XML readers will move to lxml in #1894: parse_junit() in util/ci_timing.py and the PDML read in util/pyshark_encap_map.py. They will use a parser that resolves no entities and makes no network access. lxml joins the test extra for every interpreter except GraalPy. The tests that need it skip there through a dependency gate, so no leg falls back to a looser parser.

@JarryShaw
JarryShaw merged commit 76e7e97 into main Oct 17, 2026
56 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 17, 2026
@JarryShaw
JarryShaw deleted the ci/1851-platform-pin-refresh branch October 17, 2026 16:22
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Pull requests that change CI or workflow configuration (ci: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

ci(conda): nothing retires a PLATFORM_PINS entry once upstream ships the wheel again

1 participant