ci: check the sdist can actually be consumed - #4263
Open
d-v-b wants to merge 2 commits into
Open
Conversation
d-v-b
marked this pull request as ready for review
August 13, 2026 11:15
The sdist contents are defined by an allowlist in pyproject.toml. That is the safe default -- a new subpackage under packages/ stays out of the zarr sdist unless someone opts it in -- but it inverts the failure mode. A blocklist ships too much silently; an allowlist can ship too little, by adding a file consumers need without a matching allowlist entry. The 'Test distribution artifacts' job previously ran 'ls dist' and asserted nothing. Two checks now replace it, split by what they cost, both inspecting the built tarball rather than the working tree since a git checkout has files the sdist does not. test_dist_pypi runs ci/check_sdist_contents.py: every 'testpaths' entry from pyproject.toml ships, packages/ does not, and the handful of paths packagers read but no test opens (LICENSE.txt, PKG-INFO, py.typed, the hatch-vcs version file) are present. The testpaths check is derived from config maintained for other reasons rather than restated, so it cannot drift into being a second allowlist to forget. Seconds and hermetic, so it runs on pull requests too. Against the pre-allowlist sdist it reports both defects that were live: docs/user-guide missing, and 370 packages/ paths. test_dist_conda builds the sdist with the real conda-forge recipe, fetched from the feedstock at run time and repointed at the tarball we just built by ci/conda_recipe_for_sdist.py. conda-forge is the consumer most exposed to a missing file: it builds from the sdist rather than the wheel and runs our suite from a directory holding only what its recipe copies out of the tarball. Fetching beats reproducing -- a copy of that recipe would go stale silently, which is the same class of bug this job exists to catch. Note the recipe's requirements are already regenerated from our pyproject.toml by grayskull on each version bump (bot: inspection: update-grayskull in the feedstock's conda-forge.yml); the tests block is hand-maintained there. That job is skipped on pull_request. It runs the whole suite from an unpacked sdist, measured at 591.95s of a 613s run, so conda's build and solve are about 22 seconds of it and there is no cheaper variant of the same coverage. On push to main it still catches a bad merge within minutes and attributes it to one commit, and it gates upload_pypi so no release can publish an sdist conda cannot consume. workflow_dispatch runs it against a branch on demand. Keeping it off pull requests also stops a recipe change in the feedstock from reddening unrelated PRs. A url source rather than a path one, deliberately: path sources honour .gitignore, and src/zarr/_version.py is gitignored. It is written into the sdist by the hatch-vcs hook precisely because an unpacked sdist has no git history, so a path source would drop it and break the build. Verified on CI before the trigger was narrowed: imports test passed, pip check passed, 6884 passed / 475 skipped / 4 xfailed. Assisted-by: ClaudeCode:claude-opus-5
check_sdist_contents.py derived `testpaths` from the working tree's pyproject.toml while comparing it against the tarball's contents, so the two halves of the comparison could come from different trees -- and the job comment claims the opposite, that these checks inspect the artifact rather than the checkout. Read pyproject.toml out of the tarball instead, which makes the check a property of the artifact alone: the same answer against a release sdist downloaded from PyPI as against a fresh `hatch build`. Also pin pyyaml, matching how hatch is installed in test.yml, and correct the workflow_dispatch note: dispatch only offers branches of the repository it runs in, so for a fork PR that means dispatching in the fork. Assisted-by: ClaudeCode:claude-opus-5
d-v-b
force-pushed
the
claude/zarr-sdist-ci-checks
branch
from
August 13, 2026 14:33
22cf142 to
749ab76
Compare
github-actions
Bot
added
the
needs release notes
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4263 +/- ##
=======================================
Coverage 94.00% 94.00%
=======================================
Files 91 91
Lines 12795 12795
=======================================
Hits 12028 12028
Misses 767 767 🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
builds on #4261 to add a pre-release CI job that checks if the sdist contains what our primary consumer (conda forge) needs.
claude wrote this. see the original PR here: d-v-b#291
For reviewers
This adds a fair bit of logic to our CI so I would appreciate human eyes on it. It's possible that the entire motivation here is misguided -- maybe some folks think inadvertantly breaking conda forge builds is fine and the solution to that should be confined to our feedstock repo, in which case I'm happy to close this PR.
Author attestation
TODO
docs/user-guide/*.mdchanges/