From abcb19ad846c00ac513d02e386038df5f869a8e1 Mon Sep 17 00:00:00 2001 From: John Langford Date: Mon, 24 Aug 2026 13:05:01 -0700 Subject: [PATCH 1/2] packaging: publish to PyPI from a v* tag via Trusted Publishing Dion is installable only from git today, so a downstream project that publishes to PyPI cannot declare it as a dependency at all -- PyPI rejects direct-URL requirements in uploaded distributions. vllm-project/speculators#1031 works around this with a lazy import and a manual install message. Requested in #116. Adds .github/workflows/release.yml: every change to packaging builds and validates the distributions, and a v* tag publishes them. Publishing goes through PyPI Trusted Publishing (OIDC) against a `pypi` environment, so no API token lives in this repo. The tag is checked against setup.py's version before anything is uploaded. RELEASING.md documents the one-time PyPI-side setup and the steps to cut a release. Two things had to be fixed first; both build cleanly and pass `twine check`, which is why neither has been noticed: - author_email was `{kwangjunahn, byronxu}@microsoft.com`, deliberately obfuscated against harvesters. It does not parse as an address (email.utils.parseaddr returns ('', '')) and PyPI validates the field on upload, so the first release would have been rejected at the very last step. Those addresses no longer reach anyone in any case. The author list is paper attribution and is unchanged; only the contact is now live, and project_urls points bug reports at the issue tracker instead of an inbox. - MANIFEST.in did not exist, so the sdist shipped without the requirements_*.txt files that setup.py reads at build time. read_requirements warns and returns [] when they are absent, so building from the sdist *succeeds* and produces a wheel declaring no dependencies at all -- no numpy, no torch. Verified by round-tripping: before, an sdist-built wheel had zero base Requires-Dist; after, it carries numpy and torch>=2.7.1. tests/test_packaging.py covers both. It reads setup.py with ast, so it needs neither torch nor a build, and re-runs against the built artifacts in CI via DION_DIST_DIR -- twine check inspects only the long description and would not catch either bug. Also adds the MIT and Python-version classifiers, which PyPI facets on, and a pyproject.toml [build-system] table so builds stop going through the deprecated `setup.py bdist_wheel` path. --- .github/workflows/release.yml | 94 ++++++++++++++++++++++++ .gitignore | 4 +- MANIFEST.in | 12 +++ RELEASING.md | 65 +++++++++++++++++ pyproject.toml | 6 ++ setup.py | 21 +++++- tests/test_packaging.py | 133 ++++++++++++++++++++++++++++++++++ 7 files changed, 333 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/release.yml create mode 100644 MANIFEST.in create mode 100644 RELEASING.md create mode 100644 pyproject.toml create mode 100644 tests/test_packaging.py diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..7262193 --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,94 @@ +name: Release + +# Builds and validates the distributions on every change that can affect +# packaging, and publishes to PyPI when a v* tag is pushed. Publishing uses +# PyPI Trusted Publishing (OIDC), so there is no API token stored in this repo. +# See RELEASING.md for the one-time PyPI-side setup and the release steps. + +on: + push: + tags: + - "v*" + pull_request: + paths: + - "setup.py" + - "pyproject.toml" + - "MANIFEST.in" + - "requirements_*.txt" + - "tests/test_packaging.py" + - ".github/workflows/release.yml" + workflow_dispatch: + +permissions: + contents: read + +jobs: + build: + name: Build and validate distributions + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install build tooling + run: python -m pip install --upgrade build twine pytest packaging + + - name: Check packaging metadata + run: python -m pytest tests/test_packaging.py -v + + - name: Build sdist and wheel + run: python -m build + + - name: Check distribution metadata + run: python -m twine check --strict dist/* + + # twine check only renders the long description. These re-run the + # packaging tests against the actual artifacts, which is what catches an + # sdist that dropped its requirements files. + - name: Check built artifacts + env: + DION_DIST_DIR: dist + run: python -m pytest tests/test_packaging.py -v + + - name: Verify the tag matches the packaged version + if: startsWith(github.ref, 'refs/tags/v') + run: | + packaged=$(sed -n 's/^version = "\([^"]*\)".*/\1/p' setup.py) + tagged="${GITHUB_REF_NAME#v}" + if [ -z "$packaged" ]; then + echo "::error::could not read the version literal out of setup.py" + exit 1 + fi + if [ "$packaged" != "$tagged" ]; then + echo "::error::tag ${GITHUB_REF_NAME} would publish version ${packaged}; bump setup.py or retag" + exit 1 + fi + echo "tag ${GITHUB_REF_NAME} matches packaged version ${packaged}" + + - uses: actions/upload-artifact@v4 + with: + name: dist + path: dist/ + + publish: + name: Publish to PyPI + needs: build + if: startsWith(github.ref, 'refs/tags/v') + runs-on: ubuntu-latest + # The environment gates the OIDC identity that PyPI trusts, and is where a + # manual approval can be required before anything reaches PyPI. + environment: + name: pypi + url: https://pypi.org/p/dion + permissions: + id-token: write + steps: + - uses: actions/download-artifact@v4 + with: + name: dist + path: dist/ + + - uses: pypa/gh-action-pypi-publish@release/v1 diff --git a/.gitignore b/.gitignore index 34d7e8d..eaef29e 100644 --- a/.gitignore +++ b/.gitignore @@ -8,4 +8,6 @@ output/ .venv/ submit.ipynb aztool/ -dion.egg-info/ \ No newline at end of file +dion.egg-info/ +dist/ +build/ diff --git a/MANIFEST.in b/MANIFEST.in new file mode 100644 index 0000000..5fa48f1 --- /dev/null +++ b/MANIFEST.in @@ -0,0 +1,12 @@ +# setup.py reads these at build time to populate install_requires and +# extras_require. Without them in the sdist, `read_requirements` finds nothing, +# warns, and returns an empty list -- so building from the sdist yields a wheel +# that declares no dependencies at all. See tests/test_packaging.py. +include requirements_dion.txt +include requirements_dev.txt +include requirements_train.txt +include requirements_gns.txt +include LICENSE +include NOTICE.md +include README.md +include CHANGELOG.md diff --git a/RELEASING.md b/RELEASING.md new file mode 100644 index 0000000..e41a270 --- /dev/null +++ b/RELEASING.md @@ -0,0 +1,65 @@ +# Releasing Dion + +Dion publishes to [PyPI](https://pypi.org/p/dion) from +`.github/workflows/release.yml` when a `v*` tag is pushed. Publishing uses PyPI +[Trusted Publishing](https://docs.pypi.org/trusted-publishers/), so no API token +is stored in this repository. + +## One-time setup + +Both steps need a human with the right accounts; neither can be done from a PR. + +**1. Register the pending publisher on PyPI.** The `dion` name is unclaimed, so +the first upload creates the project. Log in to pypi.org with the account that +should own it, go to *Your projects → Publishing → Add a new pending publisher*, +and enter: + +| Field | Value | +| --- | --- | +| PyPI Project Name | `dion` | +| Owner | `microsoft` | +| Repository name | `dion` | +| Workflow name | `release.yml` | +| Environment name | `pypi` | + +A pending publisher is what lets the workflow create a project that does not +exist yet. Once the first release lands it becomes an ordinary trusted publisher. + +**2. Create the `pypi` environment.** In *Settings → Environments*, add an +environment named `pypi`. The name must match the workflow and the pending +publisher exactly. Adding required reviewers here puts a human approval in front +of every upload, which is worth doing on a public package. + +## Cutting a release + +1. Bump `version` in `setup.py`. Versions below `1.0` are pre-release: breaking + changes are allowed, but a released version number can never be reused. +2. Move the `[Unreleased]` entries in `CHANGELOG.md` under a new + `## [X.Y.Z] - YYYY-MM-DD` heading. +3. Open a PR with both changes and merge it. The `Release` workflow builds and + validates the distributions on that PR, so packaging breakage surfaces before + the tag exists. +4. Tag the merge commit and push: + + ```bash + git tag vX.Y.Z && git push origin vX.Y.Z + ``` + +5. The workflow rebuilds, verifies the tag matches `setup.py`'s version, and + publishes. If the `pypi` environment requires reviewers, approve the run. + +## Notes + +- **A version is permanent.** PyPI does not allow reuse of a version number or + of a distribution filename, even after deletion. A bad release is yanked and + superseded, never replaced. Test on TestPyPI first if a release is unusual: + configure a second pending publisher at test.pypi.org and run the workflow + against it, or upload once by hand with + `twine upload -r testpypi dist/*`. +- **`twine check` is not a metadata check.** It renders the long description and + little else. The fields PyPI actually validates on upload — `author_email` + above all — are covered by `tests/test_packaging.py`, which runs in CI. +- **The sdist has to carry `requirements_*.txt`.** `setup.py` reads them at build + time to populate `install_requires`; if `MANIFEST.in` stops shipping them, a + build from the sdist still succeeds but declares no dependencies at all. + `tests/test_packaging.py` asserts against the built artifacts to catch this. diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 0000000..36ea2cc --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,6 @@ +# Package metadata lives in setup.py. This file exists so that builds go through +# PEP 517 (`python -m build`, `pip install .`) with a declared, isolated build +# environment, rather than the deprecated `python setup.py bdist_wheel` path. +[build-system] +requires = ["setuptools>=64", "wheel"] +build-backend = "setuptools.build_meta" diff --git a/setup.py b/setup.py index 7ff2618..cb91ec2 100644 --- a/setup.py +++ b/setup.py @@ -56,15 +56,34 @@ def read_requirements(path): # }, # Author information: author="Ahn, Kwangjun and Xu, Byron and Abreu, Natalie and Langford, John", # as listed in the paper - author_email="{kwangjunahn, byronxu}@microsoft.com", # left this form to prevent bots from harvesting emails + # PyPI validates this field and rejects an unparseable address, so it has to be a + # real mailbox. Bug reports belong on the issue tracker (see project_urls) rather + # than in a maintainer's inbox. + author_email="jcl@microsoft.com", # Description of the package: description="Dion: Distributed Orthonormal Updates", long_description=readme_contents, long_description_content_type="text/markdown", + project_urls={ + "Homepage": "https://github.com/microsoft/dion", + "Source": "https://github.com/microsoft/dion", + "Issues": "https://github.com/microsoft/dion/issues", + "Changelog": "https://github.com/microsoft/dion/blob/main/CHANGELOG.md", + }, # Plugins entry point classifiers=[ + "Development Status :: 4 - Beta", + "Intended Audience :: Science/Research", + "License :: OSI Approved :: MIT License", + "Operating System :: OS Independent", "Programming Language :: Python", "Programming Language :: Python :: 3", + "Programming Language :: Python :: 3.9", + "Programming Language :: Python :: 3.10", + "Programming Language :: Python :: 3.11", + "Programming Language :: Python :: 3.12", + "Programming Language :: Python :: 3.13", + "Topic :: Scientific/Engineering :: Artificial Intelligence", ], python_requires=">=3.9", license="MIT", diff --git a/tests/test_packaging.py b/tests/test_packaging.py new file mode 100644 index 0000000..cb8548a --- /dev/null +++ b/tests/test_packaging.py @@ -0,0 +1,133 @@ +"""Tests for the packaging metadata that PyPI validates at upload time. + +These guard two failure modes that every local check passes: + +- PyPI rejects an unparseable ``author_email``, but ``twine check`` only + renders the long description and never looks at the field. The address + shipped here for the package's first year (``{user1, user2}@microsoft.com``, + obfuscated against harvesters) builds and passes ``twine check`` cleanly and + fails only against the upload API. +- ``setup.py`` reads ``requirements_*.txt`` at build time. If those files are + absent from the sdist, ``read_requirements`` warns and returns ``[]``, so + building from the sdist succeeds and produces a wheel declaring *no* + dependencies. ``MANIFEST.in`` is what keeps them in. + +Metadata is read out of ``setup.py`` with ``ast`` so the tests need neither a +build step nor torch. The artifact tests run only when ``DION_DIST_DIR`` points +at a built ``dist/`` directory; CI sets it after ``python -m build``. +""" + +import ast +import os +import tarfile +import zipfile +from email.utils import parseaddr +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +SETUP_PY = REPO_ROOT / "setup.py" +MANIFEST_IN = REPO_ROOT / "MANIFEST.in" + + +def _setup_call(): + tree = ast.parse(SETUP_PY.read_text()) + for node in ast.walk(tree): + if isinstance(node, ast.Call) and getattr(node.func, "id", None) == "setup": + return {kw.arg: kw.value for kw in node.keywords} + raise AssertionError("no setup() call found in setup.py") + + +def _module_assignment(name): + tree = ast.parse(SETUP_PY.read_text()) + for node in tree.body: + if isinstance(node, ast.Assign): + for target in node.targets: + if isinstance(target, ast.Name) and target.id == name: + return ast.literal_eval(node.value) + raise AssertionError(f"no module-level assignment to {name} in setup.py") + + +def _keyword(name): + node = _setup_call().get(name) + if node is None: + raise AssertionError(f"setup() has no {name} keyword") + return ast.literal_eval(node) + + +def test_author_email_is_a_deliverable_address(): + """PyPI's upload API rejects an address that does not parse.""" + email = _keyword("author_email") + name, addr = parseaddr(email) + assert addr, f"author_email {email!r} does not parse as an email address" + assert "@" in addr and not addr.startswith("@") and not addr.endswith("@") + assert "," not in addr and "{" not in addr and "}" not in addr + + +def test_version_is_pep440(): + """A tag-driven release publishes this string; PyPI requires PEP 440.""" + Version = pytest.importorskip("packaging.version").Version + Version(_module_assignment("version")) + + +def test_project_urls_point_at_the_repository(): + """Without these the PyPI page has no link back to the source.""" + urls = _keyword("project_urls") + assert "Issues" in urls, "no issue tracker link; bug reports land in a maintainer inbox" + for label, url in urls.items(): + assert url.startswith("https://github.com/microsoft/dion"), ( + f"project_urls[{label!r}] does not point at the repository: {url}" + ) + + +def test_classifiers_declare_the_license(): + """setup.py's license="MIT" is free text; the classifier is what PyPI facets on.""" + classifiers = _keyword("classifiers") + assert "License :: OSI Approved :: MIT License" in classifiers + + +@pytest.mark.parametrize( + "requirements_file", + sorted(p.name for p in REPO_ROOT.glob("requirements_*.txt")), +) +def test_manifest_ships_every_requirements_file(requirements_file): + """Any requirements file setup.py may read has to survive into the sdist.""" + manifest = MANIFEST_IN.read_text() + assert f"include {requirements_file}" in manifest, ( + f"{requirements_file} is missing from MANIFEST.in, so an sdist build would " + f"silently drop the dependencies it declares" + ) + + +def _dist_dir(): + dist = os.environ.get("DION_DIST_DIR") + if not dist: + pytest.skip("DION_DIST_DIR not set; run `python -m build` first") + return Path(dist) + + +def _one(pattern): + matches = sorted(_dist_dir().glob(pattern)) + assert len(matches) == 1, f"expected exactly one {pattern} in dist/, found {matches}" + return matches[0] + + +def test_sdist_contains_the_requirements_files(): + with tarfile.open(_one("*.tar.gz")) as tar: + names = {Path(n).name for n in tar.getnames()} + missing = [p.name for p in REPO_ROOT.glob("requirements_*.txt") if p.name not in names] + assert not missing, f"sdist is missing {missing}; a build from it would declare no deps" + + +def test_wheel_declares_its_runtime_dependencies(): + with zipfile.ZipFile(_one("*.whl")) as wheel: + metadata_name = next(n for n in wheel.namelist() if n.endswith(".dist-info/METADATA")) + metadata = wheel.read(metadata_name).decode() + requires = [ + line.split(":", 1)[1].strip() + for line in metadata.splitlines() + if line.startswith("Requires-Dist:") and "extra ==" not in line + ] + assert any(r.startswith("torch") for r in requires), f"wheel declares no torch: {requires}" + assert any(r.startswith("numpy") for r in requires), f"wheel declares no numpy: {requires}" From a16be63c24cfaa1d9e20353ce58b01d8eac9d897 Mon Sep 17 00:00:00 2001 From: John Langford Date: Mon, 24 Aug 2026 13:19:22 -0700 Subject: [PATCH 2/2] packaging: harden the release workflow and widen the packaging guards Review follow-ups on the PyPI publishing workflow. Supply chain: pin every action to a commit SHA. The publish job holds an OIDC identity PyPI trusts to upload as `dion`, and `pypa/gh-action-pypi-publish@release/v1` is a mutable *branch* -- a compromise of it runs code inside that job. Adds .github/dependabot.yml so the pins do not rot, and `persist-credentials: false` on the checkout, which does not need to keep a token in .git/config. Tag gate: verify the tag against the version baked into the built distribution filenames rather than re-parsing setup.py with sed. That is the version that will actually be uploaded, it removes the second parser for the same field, and comparing PEP 440 Versions rejects a tag that is not a valid version at all. PR trigger: the paths filter missed README.md -- which is the long_description that `twine check --strict` renders, so a README change could break a release with no pre-tag signal -- along with the other files MANIFEST.in ships and `dion/**`, whose layout find_packages() reads. MANIFEST.in: glob `requirements_*.txt` instead of listing the four files, so a requirements file added later is shipped rather than merely detected as missing. The test now matches names against the include patterns. Tests: - test_wheel_ships_every_package_module -- nothing checked that the wheel contains the code. find_packages() silently drops a subdirectory with no __init__.py, and CI builds the wheel from the sdist, so this covers both. - test_python_classifiers_agree_with_python_requires -- the classifiers advertise 3.9-3.13 with nothing tying them to python_requires. - test_repo_has_requirements_files -- an empty glob made the parametrized manifest test vacuous, which is the silent-pass failure this file exists to prevent. - import packaging directly instead of pytest.importorskip; pytest depends on it, so the skip could only ever hide the PEP 440 check. RELEASING.md: document that Actions must be enabled for repo-file workflows here -- no workflow under .github/ has ever run in this repository, and release.yml did not trigger on its own pull request -- plus why `python -m build` is spelled without flags (only the no-flag form builds the wheel from the sdist, which is the round trip the dependency assertions rely on), the SHA pinning, and that a `v*` tag alone reaches the upload step. --- .github/dependabot.yml | 9 ++++ .github/workflows/release.yml | 87 +++++++++++++++++++++++++++-------- MANIFEST.in | 15 +++--- RELEASING.md | 35 ++++++++++++-- tests/test_packaging.py | 73 ++++++++++++++++++++++++----- 5 files changed, 176 insertions(+), 43 deletions(-) create mode 100644 .github/dependabot.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..9b15406 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,9 @@ +# The release workflow pins its actions to commit SHAs so that a moved tag in +# someone else's repository cannot run new code in the job that holds the PyPI +# OIDC identity. Dependabot is what keeps those pins from going stale. +version: 2 +updates: + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "monthly" diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 7262193..66fe86e 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -4,6 +4,11 @@ name: Release # packaging, and publishes to PyPI when a v* tag is pushed. Publishing uses # PyPI Trusted Publishing (OIDC), so there is no API token stored in this repo. # See RELEASING.md for the one-time PyPI-side setup and the release steps. +# +# Third-party actions are pinned to a commit SHA rather than a tag or branch: +# the publish job holds an OIDC identity that PyPI trusts to upload as `dion`, +# so a moved tag in someone else's repository must not be able to run new code +# inside it. .github/dependabot.yml keeps the pins current. on: push: @@ -15,6 +20,15 @@ on: - "pyproject.toml" - "MANIFEST.in" - "requirements_*.txt" + # long_description, and the files MANIFEST.in ships. + - "README.md" + - "LICENSE" + - "NOTICE.md" + - "CHANGELOG.md" + # find_packages() decides what lands in the wheel, so the package layout + # is packaging input too -- a new subdirectory without an __init__.py is + # silently dropped from the distribution. + - "dion/**" - "tests/test_packaging.py" - ".github/workflows/release.yml" workflow_dispatch: @@ -27,9 +41,11 @@ jobs: name: Build and validate distributions runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + persist-credentials: false - - uses: actions/setup-python@v5 + - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 with: python-version: "3.12" @@ -39,6 +55,11 @@ jobs: - name: Check packaging metadata run: python -m pytest tests/test_packaging.py -v + # The bare `python -m build` is load-bearing: with no flags it builds the + # sdist from the source tree and then the wheel *from that sdist*, which + # is the round trip that exposes an sdist missing its requirements files. + # `python -m build --sdist --wheel` builds both from the source tree + # instead and would leave the wheel checks below passing on a broken sdist. - name: Build sdist and wheel run: python -m build @@ -53,22 +74,52 @@ jobs: DION_DIST_DIR: dist run: python -m pytest tests/test_packaging.py -v - - name: Verify the tag matches the packaged version + # Checks the version baked into the built filenames -- the one that will + # actually be uploaded -- rather than re-reading setup.py, so the gate + # cannot drift from what the publish job pushes to PyPI. + - name: Verify the tag matches the built distributions if: startsWith(github.ref, 'refs/tags/v') + env: + TAG_NAME: ${{ github.ref_name }} run: | - packaged=$(sed -n 's/^version = "\([^"]*\)".*/\1/p' setup.py) - tagged="${GITHUB_REF_NAME#v}" - if [ -z "$packaged" ]; then - echo "::error::could not read the version literal out of setup.py" - exit 1 - fi - if [ "$packaged" != "$tagged" ]; then - echo "::error::tag ${GITHUB_REF_NAME} would publish version ${packaged}; bump setup.py or retag" - exit 1 - fi - echo "tag ${GITHUB_REF_NAME} matches packaged version ${packaged}" - - - uses: actions/upload-artifact@v4 + python - <<'PY' + import os + import pathlib + from packaging.utils import parse_sdist_filename, parse_wheel_filename + from packaging.version import InvalidVersion, Version + + + def fail(message): + print(f"::error::{message}") + raise SystemExit(1) + + + def only(pattern): + matches = sorted(pathlib.Path("dist").glob(pattern)) + if len(matches) != 1: + fail(f"expected exactly one {pattern} in dist/, found {[p.name for p in matches]}") + return matches[0] + + + built = { + parse_wheel_filename(only("*.whl").name)[1], + parse_sdist_filename(only("*.tar.gz").name)[1], + } + if len(built) != 1: + fail(f"sdist and wheel disagree on the version: {sorted(str(v) for v in built)}") + packaged = built.pop() + + tag = os.environ["TAG_NAME"] + try: + tagged = Version(tag[1:]) + except InvalidVersion: + fail(f"tag {tag} is not a PEP 440 version, so it cannot name a release") + if tagged != packaged: + fail(f"tag {tag} would publish version {packaged}; bump setup.py or retag") + print(f"tag {tag} matches the built version {packaged}") + PY + + - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: name: dist path: dist/ @@ -86,9 +137,9 @@ jobs: permissions: id-token: write steps: - - uses: actions/download-artifact@v4 + - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 with: name: dist path: dist/ - - uses: pypa/gh-action-pypi-publish@release/v1 + - uses: pypa/gh-action-pypi-publish@dc37677b2e1c63e2034f94d8a5b11f265b73ba33 # v1.14.2 diff --git a/MANIFEST.in b/MANIFEST.in index 5fa48f1..38640b4 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -1,11 +1,10 @@ -# setup.py reads these at build time to populate install_requires and -# extras_require. Without them in the sdist, `read_requirements` finds nothing, -# warns, and returns an empty list -- so building from the sdist yields a wheel -# that declares no dependencies at all. See tests/test_packaging.py. -include requirements_dion.txt -include requirements_dev.txt -include requirements_train.txt -include requirements_gns.txt +# setup.py reads the requirements files at build time to populate +# install_requires and extras_require. Without them in the sdist, +# `read_requirements` finds nothing, warns, and returns an empty list -- so +# building from the sdist yields a wheel that declares no dependencies at all. +# The glob covers requirements files added later, which an explicit list would +# not. See tests/test_packaging.py. +include requirements_*.txt include LICENSE include NOTICE.md include README.md diff --git a/RELEASING.md b/RELEASING.md index e41a270..f2e5c2a 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -7,9 +7,19 @@ is stored in this repository. ## One-time setup -Both steps need a human with the right accounts; neither can be done from a PR. +All three steps need a human with the right accounts; none can be done from a PR. -**1. Register the pending publisher on PyPI.** The `dion` name is unclaimed, so +**1. Enable GitHub Actions for workflows in this repository.** As of this +writing no workflow defined in `.github/` has ever run here — the only entries +under *Actions* are the org's managed ones (CodeQL default setup, Copilot +review, Dependabot), and `release.yml` did not trigger on the pull request that +added it. Reading the setting needs admin, so check *Settings → Actions → +General* and confirm that actions are allowed and that `actions/*` and +`pypa/gh-action-pypi-publish` are permitted by the allow-list, if one is in +force. Without this neither half of the workflow runs: no pre-tag validation, +and no publish. + +**2. Register the pending publisher on PyPI.** The `dion` name is unclaimed, so the first upload creates the project. Log in to pypi.org with the account that should own it, go to *Your projects → Publishing → Add a new pending publisher*, and enter: @@ -25,7 +35,7 @@ and enter: A pending publisher is what lets the workflow create a project that does not exist yet. Once the first release lands it becomes an ordinary trusted publisher. -**2. Create the `pypi` environment.** In *Settings → Environments*, add an +**3. Create the `pypi` environment.** In *Settings → Environments*, add an environment named `pypi`. The name must match the workflow and the pending publisher exactly. Adding required reviewers here puts a human approval in front of every upload, which is worth doing on a public package. @@ -45,8 +55,9 @@ of every upload, which is worth doing on a public package. git tag vX.Y.Z && git push origin vX.Y.Z ``` -5. The workflow rebuilds, verifies the tag matches `setup.py`'s version, and - publishes. If the `pypi` environment requires reviewers, approve the run. +5. The workflow rebuilds, verifies the tag matches the version baked into the + built distributions, and publishes. If the `pypi` environment requires + reviewers, approve the run. ## Notes @@ -63,3 +74,17 @@ of every upload, which is worth doing on a public package. time to populate `install_requires`; if `MANIFEST.in` stops shipping them, a build from the sdist still succeeds but declares no dependencies at all. `tests/test_packaging.py` asserts against the built artifacts to catch this. +- **`python -m build` is spelled without flags on purpose.** With no arguments it + builds the sdist from the source tree and then the wheel *from that sdist*, + which is the round trip that exposes the failure above. `python -m build + --sdist --wheel` builds both from the source tree and would leave the wheel + assertions passing over a broken sdist. +- **The workflow pins its actions to commit SHAs.** The publish job holds an + OIDC identity that PyPI trusts to upload as `dion`, so it must not run code + fetched from a mutable tag or branch in someone else's repository — + `pypa/gh-action-pypi-publish@release/v1` is a branch. `.github/dependabot.yml` + raises PRs to move the pins forward; the trailing `# vX.Y.Z` comment on each + is what it reads to know the current version. +- **A tag is enough to publish.** Anyone who can push a `v*` tag — or run the + workflow manually against one — reaches the upload step. Required reviewers on + the `pypi` environment are what stands between that and PyPI. diff --git a/tests/test_packaging.py b/tests/test_packaging.py index cb8548a..bd04499 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -1,6 +1,6 @@ """Tests for the packaging metadata that PyPI validates at upload time. -These guard two failure modes that every local check passes: +These guard failure modes that every local check passes: - PyPI rejects an unparseable ``author_email``, but ``twine check`` only renders the long description and never looks at the field. The address @@ -11,13 +11,18 @@ absent from the sdist, ``read_requirements`` warns and returns ``[]``, so building from the sdist succeeds and produces a wheel declaring *no* dependencies. ``MANIFEST.in`` is what keeps them in. +- ``find_packages`` only picks up directories that have an ``__init__.py``, so + a new subpackage without one is dropped from the distribution silently. Metadata is read out of ``setup.py`` with ``ast`` so the tests need neither a build step nor torch. The artifact tests run only when ``DION_DIST_DIR`` points -at a built ``dist/`` directory; CI sets it after ``python -m build``. +at a built ``dist/`` directory; CI sets it after ``python -m build``. That bare +``python -m build`` builds the wheel *from the sdist*, so the wheel assertions +below also cover what the sdist carries. """ import ast +import fnmatch import os import tarfile import zipfile @@ -25,10 +30,14 @@ from pathlib import Path import pytest +from packaging.specifiers import SpecifierSet +from packaging.version import Version REPO_ROOT = Path(__file__).resolve().parent.parent SETUP_PY = REPO_ROOT / "setup.py" MANIFEST_IN = REPO_ROOT / "MANIFEST.in" +PACKAGE_DIR = REPO_ROOT / "dion" +REQUIREMENTS_FILES = sorted(p.name for p in REPO_ROOT.glob("requirements_*.txt")) def _setup_call(): @@ -56,6 +65,15 @@ def _keyword(name): return ast.literal_eval(node) +def _manifest_include_patterns(): + patterns = [] + for line in MANIFEST_IN.read_text().splitlines(): + line = line.strip() + if line.startswith("include "): + patterns.extend(line.split()[1:]) + return patterns + + def test_author_email_is_a_deliverable_address(): """PyPI's upload API rejects an address that does not parse.""" email = _keyword("author_email") @@ -67,7 +85,6 @@ def test_author_email_is_a_deliverable_address(): def test_version_is_pep440(): """A tag-driven release publishes this string; PyPI requires PEP 440.""" - Version = pytest.importorskip("packaging.version").Version Version(_module_assignment("version")) @@ -87,16 +104,35 @@ def test_classifiers_declare_the_license(): assert "License :: OSI Approved :: MIT License" in classifiers -@pytest.mark.parametrize( - "requirements_file", - sorted(p.name for p in REPO_ROOT.glob("requirements_*.txt")), -) +def test_python_classifiers_agree_with_python_requires(): + """A classifier PyPI advertises but python_requires excludes is a false claim.""" + supported = SpecifierSet(_keyword("python_requires")) + declared = [ + c.rsplit(" :: ", 1)[1] + for c in _keyword("classifiers") + if c.startswith("Programming Language :: Python :: ") + ] + versions = [v for v in declared if "." in v] + assert versions, "no Programming Language :: Python :: X.Y classifiers to check" + excluded = [v for v in versions if not supported.contains(v)] + assert not excluded, ( + f"classifiers advertise Python {excluded} but python_requires " + f"{str(supported)!r} rejects them" + ) + + +def test_repo_has_requirements_files(): + """The manifest and sdist tests below are parametrized on this glob.""" + assert REQUIREMENTS_FILES, "no requirements_*.txt in the repo root; the tests below are vacuous" + + +@pytest.mark.parametrize("requirements_file", REQUIREMENTS_FILES) def test_manifest_ships_every_requirements_file(requirements_file): """Any requirements file setup.py may read has to survive into the sdist.""" - manifest = MANIFEST_IN.read_text() - assert f"include {requirements_file}" in manifest, ( - f"{requirements_file} is missing from MANIFEST.in, so an sdist build would " - f"silently drop the dependencies it declares" + patterns = _manifest_include_patterns() + assert any(fnmatch.fnmatch(requirements_file, pattern) for pattern in patterns), ( + f"{requirements_file} matches no include line in MANIFEST.in, so an sdist build " + f"would silently drop the dependencies it declares" ) @@ -116,7 +152,7 @@ def _one(pattern): def test_sdist_contains_the_requirements_files(): with tarfile.open(_one("*.tar.gz")) as tar: names = {Path(n).name for n in tar.getnames()} - missing = [p.name for p in REPO_ROOT.glob("requirements_*.txt") if p.name not in names] + missing = [name for name in REQUIREMENTS_FILES if name not in names] assert not missing, f"sdist is missing {missing}; a build from it would declare no deps" @@ -131,3 +167,16 @@ def test_wheel_declares_its_runtime_dependencies(): ] assert any(r.startswith("torch") for r in requires), f"wheel declares no torch: {requires}" assert any(r.startswith("numpy") for r in requires), f"wheel declares no numpy: {requires}" + + +def test_wheel_ships_every_package_module(): + """find_packages() drops a subdirectory that has no __init__.py, without complaint.""" + source = {p.relative_to(REPO_ROOT).as_posix() for p in PACKAGE_DIR.rglob("*.py")} + assert source, f"no modules found under {PACKAGE_DIR}" + with zipfile.ZipFile(_one("*.whl")) as wheel: + shipped = set(wheel.namelist()) + missing = sorted(source - shipped) + assert not missing, ( + f'the wheel is missing {missing}; find_packages(include=["dion", "dion.*"]) ' + f"only picks up directories with an __init__.py" + )