diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml index a08d5a6d3f..93bc001f2c 100644 --- a/.github/workflows/releases.yml +++ b/.github/workflows/releases.yml @@ -48,24 +48,102 @@ jobs: name: releases path: dist + # The sdist contents are defined by an allowlist in pyproject.toml, so the + # failure mode to guard against is shipping too little: a new file that + # consumers need, added without a matching allowlist entry. This job and + # `test_dist_conda` below inspect the built artifact rather than the working + # tree, because a git checkout has files the tarball does not. + # + # Seconds, no network beyond the artifact download, so it runs everywhere the + # workflow does -- including pull requests. test_dist_pypi: name: Test distribution artifacts needs: [build_artifacts] runs-on: ubuntu-latest steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: releases path: dist - - name: test + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + name: Install Python + with: + python-version: '3.12' + + # Catches what the conda build cannot: paths that no test opens and no + # build step reads, and paths that must never ship (packages/, which is + # three separately released distributions -- conda would not object to + # those, but we do). + - name: Check sdist contents + run: python ci/check_sdist_contents.py dist + + # 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. Run the real recipe rather + # than a copy of it -- a copy would go stale the moment the feedstock changed, + # and silently, which is the same class of bug this job exists to catch. + # + # Not on pull requests. It runs the whole suite from an unpacked sdist, which + # is ~10 minutes whether or not conda is involved, and it depends on a fetch + # from conda-forge/zarr-feedstock, so a recipe change there would redden + # unrelated PRs. On `push` to main it still catches a bad merge within + # minutes, attributed to one commit, and it gates `upload_pypi` so no release + # can publish an sdist conda cannot consume. + # + # To run it against a branch before merging, use `workflow_dispatch`. Note + # that dispatch only offers branches of the repository it runs in, so for a + # pull request from a fork that means dispatching this workflow in the fork, + # not here. + test_dist_conda: + name: Test conda-forge can consume the sdist + needs: [build_artifacts] + if: github.event_name != 'pull_request' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: releases + path: dist + + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + name: Install Python + with: + python-version: '3.12' + + - name: Fetch the conda-forge recipe run: | - ls - ls dist + curl -fsSL --retry 3 -o "${RUNNER_TEMP}/recipe-upstream.yaml" \ + https://raw.githubusercontent.com/conda-forge/zarr-feedstock/main/recipe/recipe.yaml + + - name: Point the recipe at the sdist we just built + run: | + python -m pip install pyyaml==6.0.3 + python ci/conda_recipe_for_sdist.py \ + "${RUNNER_TEMP}/recipe-upstream.yaml" \ + "$(ls dist/*.tar.gz)" \ + conda.recipe/recipe.yaml + + # Runs the recipe's own `tests:` block: an import check, `pip check`, and + # our suite from a tree containing only the recipe's `files.source`. + - uses: prefix-dev/rattler-build-action@1ca5f45832f419a46d1326ccc5861d7e14d67c44 # v0.2.39 + with: + rattler-build-version: v0.73.0 + recipe-path: conda.recipe/recipe.yaml + upload-artifact: false + build-args: -c conda-forge --output-dir ${{ runner.temp }}/conda-build upload_pypi: name: Upload to PyPI - needs: [build_artifacts, test_dist_pypi] + needs: [build_artifacts, test_dist_pypi, test_dist_conda] runs-on: ubuntu-latest if: github.event_name == 'release' environment: diff --git a/.gitignore b/.gitignore index 59b6632a3c..9fbb810b25 100644 --- a/.gitignore +++ b/.gitignore @@ -7,6 +7,9 @@ __pycache__/ # Distribution / packaging .Python +# Written by ci/conda_recipe_for_sdist.py when checking the sdist against the +# conda-forge recipe; generated per run, never committed. +conda.recipe/ env/ .venv/ build/ diff --git a/changes/4261.misc.md b/changes/4261.misc.md new file mode 100644 index 0000000000..ca2a3fbb1e --- /dev/null +++ b/changes/4261.misc.md @@ -0,0 +1 @@ +The contents of the `zarr` source distribution are now defined by an explicit allowlist rather than a blocklist. Previously the sdist bundled the whole `packages/` tree — `zarr-indexing`, `zarr-metadata` and `zarr-http-server`, which are released as their own distributions — along with CI configuration and other repository files. The sdist also now ships `docs/`, so the test suite it carries can be collected and run from an unpacked sdist. diff --git a/ci/check_sdist_contents.py b/ci/check_sdist_contents.py new file mode 100644 index 0000000000..6ac71a7ebe --- /dev/null +++ b/ci/check_sdist_contents.py @@ -0,0 +1,149 @@ +""" +Check a built sdist ships everything its consumers need, and nothing they don't. + +Usage: + python check_sdist_contents.py SDIST + +SDIST is a path to a `.tar.gz`, or a directory containing exactly one. + +`[tool.hatch.build.targets.sdist]` in pyproject.toml is an allowlist. 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: instead of +shipping too much silently, an allowlist can ship too little. This script +guards that direction. + +Wherever possible the expectations are *derived* from configuration that is +maintained for other reasons, rather than restated here. A hand-written list of +"files the sdist must contain" would just be a second allowlist to forget to +update, which is the problem it is meant to solve. + +Everything is read out of the tarball, including the configuration the +expectations are derived from. The check is therefore a property of the +artifact alone: it gives the same answer against a release sdist downloaded +from PyPI as it does against a fresh `hatch build`, with no working tree in +the picture -- which matters, because a git checkout has files the tarball +does not. +""" + +import sys +import tarfile +import tomllib +from pathlib import Path + +# Paths no test opens, so the sdist test run in `releases.yml` cannot vouch for +# them, but that packagers do need. conda-forge's recipe installs the sdist +# (pyproject.toml, README.md, src/) and reads `license_file: LICENSE.txt`; the +# version file is written into the sdist by the hatch-vcs hook, because an +# unpacked sdist has no git history to derive a version from. +# https://github.com/conda-forge/zarr-feedstock/blob/main/recipe/recipe.yaml +REQUIRED_PATHS = [ + "pyproject.toml", + "README.md", + "LICENSE.txt", + "PKG-INFO", + "src/zarr/__init__.py", + "src/zarr/py.typed", + "src/zarr/_version.py", +] + +# Each subpackage under `packages/` is its own distribution with its own PyPI +# release, its own version tags and its own sdist. Before the allowlist they +# were bundled into every `zarr` sdist -- 2.9M of unrelated sources -- which is +# the regression this check exists to prevent recurring. +FORBIDDEN_PREFIXES = ["packages/"] + + +def sdist_members(sdist: Path) -> set[str]: + """Every path inside the tarball, relative to its top-level directory.""" + with tarfile.open(sdist) as tar: + names = tar.getnames() + # Members are `zarr-/`; strip the leading component so the + # expectations below don't have to know the version. + return {name.split("/", 1)[1] for name in names if "/" in name} + + +def read_member(sdist: Path, path: str) -> bytes: + """Read one member of the tarball, addressed relative to its top-level directory.""" + with tarfile.open(sdist) as tar: + roots = {name.split("/", 1)[0] for name in tar.getnames()} + if len(roots) != 1: + raise SystemExit(f"Expected one top-level directory in {sdist}, got {sorted(roots)}") + member = tar.extractfile(f"{roots.pop()}/{path}") + if member is None: + raise SystemExit(f"{path} is missing from {sdist.name}") + return member.read() + + +def testpaths(sdist: Path) -> list[str]: + """`testpaths` from the pyproject.toml the sdist ships. + + Derived rather than duplicated: adding a directory to `testpaths` without + adding it to the sdist allowlist is exactly the mistake this catches. It is + also the mistake that was already live -- `docs/user-guide` has been a + testpath while `docs/` was excluded, so `pytest` on an unpacked sdist died + at collection. + + Read from the tarball rather than the working tree so the two halves of the + comparison always come from the same artifact. + """ + config = tomllib.loads(read_member(sdist, "pyproject.toml").decode()) + return config["tool"]["pytest"]["ini_options"]["testpaths"] + + +def check(sdist: Path) -> int: + print(f"Checking {sdist.name}") + members = sdist_members(sdist) + print(f"Found {len(members)} paths") + print() + + missing_required = [p for p in REQUIRED_PATHS if p not in members] + # A testpath is a directory; it is present if anything ships beneath it. + # Skipped when pyproject.toml itself did not ship: it is already reported as + # a missing required path, and there is nothing left to read `testpaths` out + # of. + expected = [] if "pyproject.toml" in missing_required else testpaths(sdist) + missing_testpaths = [p for p in expected if not any(m.startswith(f"{p}/") for m in members)] + forbidden = sorted(m for m in members if any(m.startswith(p) for p in FORBIDDEN_PREFIXES)) + + if not (missing_required or missing_testpaths or forbidden): + print("OK") + return 0 + + if missing_required: + print("Required paths missing from the sdist") + print("-------------------------------------") + print("\n".join(missing_required)) + print() + if missing_testpaths: + print("testpaths entries missing from the sdist") + print("----------------------------------------") + print("\n".join(missing_testpaths)) + print("`pytest` on an unpacked sdist will fail to collect these.") + print() + if forbidden: + print("Paths that must not ship in the zarr sdist") + print("------------------------------------------") + print("\n".join(forbidden[:20])) + if len(forbidden) > 20: + print(f"... and {len(forbidden) - 20} more") + print() + + print( + "Fix by editing `[tool.hatch.build.targets.sdist]` in pyproject.toml. " + "It is an allowlist: a path ships only if it is listed." + ) + return 1 + + +if __name__ == "__main__": + if len(sys.argv) != 2: + print(__doc__) + sys.exit(2) + target = Path(sys.argv[1]).resolve() + if target.is_dir(): + tarballs = sorted(target.glob("*.tar.gz")) + if len(tarballs) != 1: + print(f"Expected exactly one .tar.gz in {target}, found {len(tarballs)}") + sys.exit(2) + target = tarballs[0] + sys.exit(check(target)) diff --git a/ci/conda_recipe_for_sdist.py b/ci/conda_recipe_for_sdist.py new file mode 100644 index 0000000000..a79f273a81 --- /dev/null +++ b/ci/conda_recipe_for_sdist.py @@ -0,0 +1,96 @@ +""" +Point the conda-forge recipe at a locally built sdist. + +Usage: + python conda_recipe_for_sdist.py RECIPE SDIST OUTPUT + +The `zarr` sdist contents are an allowlist (see `[tool.hatch.build.targets.sdist]` +in pyproject.toml), so the failure mode to guard against is shipping too little. +conda-forge is the consumer most exposed to that: it builds from the sdist rather +than the wheel, and runs our test suite from a directory holding only what its +recipe copies out of the tarball. + +Rather than restate that recipe here -- a copy would drift the moment the +feedstock changed -- CI fetches the real one and this script repoints it at the +sdist we just built. Everything that makes the check meaningful (the test files, +the pytest invocation, `pip_check`, `license_file`) comes from the feedstock +unmodified. + +Note that only the parts of the recipe that identify *which* tarball to build are +touched. The feedstock's `requirements:` are regenerated from our pyproject.toml +by grayskull on each version bump (`bot: inspection: update-grayskull` in its +conda-forge.yml), so they already track this repo; the `tests:` block is +hand-maintained there and is exactly what we want to run verbatim. +""" + +import hashlib +import sys +import tarfile +from pathlib import Path +from typing import Any + +import yaml + + +def sdist_root(sdist: Path) -> str: + """The tarball's single top-level directory, e.g. `zarr-3.3.1`.""" + with tarfile.open(sdist) as tar: + roots = {name.split("/", 1)[0] for name in tar.getnames()} + if len(roots) != 1: + raise SystemExit(f"Expected one top-level directory in {sdist}, got {roots}") + return roots.pop() + + +def conda_version(root: str) -> str: + """Derive a conda-acceptable version from the sdist directory name. + + A build off a tag gives a clean `3.3.1`, but any other commit gives a + setuptools-scm local version like `3.3.1.dev23+g52a63801`. Conda versions + cannot contain `+`, and the local segment identifies the checkout rather + than the release, so drop it. The value only labels the throwaway package + this check builds; nothing is published from it. + """ + version = root.split("-", 1)[1] + return version.split("+", 1)[0] + + +def rewrite(recipe: dict[str, Any], sdist: Path) -> dict[str, Any]: + root = sdist_root(sdist) + context = recipe.setdefault("context", {}) + context["version"] = conda_version(root) + # The recipe interpolates `${{ sha256 }}` into `source`, which is replaced + # wholesale below; keep the key consistent anyway so a partially templated + # recipe cannot silently reference a stale digest. + context["sha256"] = hashlib.sha256(sdist.read_bytes()).hexdigest() + + # A `url` source rather than a `path` one, so this mirrors what conda-forge + # actually does: fetch an archive and unpack it. It also sidesteps a trap -- + # `path` sources honour .gitignore by default, and `src/zarr/_version.py` is + # gitignored. It is written into the sdist by the hatch-vcs build hook + # precisely because an unpacked sdist has no git history to derive a version + # from, so silently dropping it would break the build. + recipe["source"] = { + "url": sdist.resolve().as_uri(), + "sha256": context["sha256"], + } + return recipe + + +if __name__ == "__main__": + if len(sys.argv) != 4: + print(__doc__) + sys.exit(2) + recipe_path, sdist_path, output_path = (Path(p) for p in sys.argv[1:]) + + with recipe_path.open("rb") as f: + recipe = yaml.safe_load(f) + + recipe = rewrite(recipe, sdist_path) + + output_path.parent.mkdir(parents=True, exist_ok=True) + with output_path.open("w") as f: + yaml.safe_dump(recipe, f, sort_keys=False) + + print(f"Wrote {output_path}") + print(f" version: {recipe['context']['version']}") + print(f" source: {recipe['source']['url']}") diff --git a/pyproject.toml b/pyproject.toml index e0fbfc08fc..5e8129a6a9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -2,11 +2,23 @@ requires = ["hatchling>=1.29.0", "hatch-vcs"] build-backend = "hatchling.build" +# An allowlist, not a blocklist: anything new — a subpackage under `packages/`, a +# config file in the repository root — stays out of the sdist unless it is named +# here. Beyond `src` and `tests`, the entries are what keeps the shipped test +# suite and docs build runnable from an unpacked sdist: `tests/test_docs.py` +# walks `docs/` and `testpaths` collects `docs/user-guide`; the pages under +# `docs/user-guide/examples/` pull their source out of `examples/` via pymdownx +# snippet includes; `mkdocs.yml` and `mkdocs_hooks.py` let `mkdocs build` run +# too. `pyproject.toml`, `README.md`, `LICENSE.txt` and `.gitignore` are added by +# hatchling itself. [tool.hatch.build.targets.sdist] -exclude = [ - "/.github", - "/bench", +include = [ + "/src", + "/tests", "/docs", + "/examples", + "/mkdocs.yml", + "/mkdocs_hooks.py", ] [project]