diff --git a/.github/workflows/api-compatibility.yml b/.github/workflows/api-compatibility.yml new file mode 100644 index 00000000..2308d125 --- /dev/null +++ b/.github/workflows/api-compatibility.yml @@ -0,0 +1,40 @@ +name: API compatibility + +# A breaking change to a public name or signature must be declared with `!` in the PR +# title, and a release's versions must match the changes since each package's last +# release. See scripts/api_breaking.py. + +on: + pull_request: + # edited / labeled / unlabeled: a new title or the api-compatible label re-runs the check. + types: [opened, synchronize, reopened, edited, labeled, unlabeled] + +permissions: + contents: read + +jobs: + api-compatibility: + name: API compatibility + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 # the base commit and every release tag + - uses: astral-sh/setup-uv@v5 + with: + python-version: "3.12" + - run: uv lock + - run: uv sync --all-packages + - name: A breaking change is declared + if: github.head_ref != 'release-please--branches--main' + env: + BASE: ${{ github.event.pull_request.base.sha }} + TITLE: ${{ github.event.pull_request.title }} + LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} + run: uv run python -m scripts.api_breaking pr --base "$BASE" --title "$TITLE" --labels "$LABELS" + - name: The release versions match their changes + if: github.head_ref == 'release-please--branches--main' + env: + BASE: ${{ github.event.pull_request.base.sha }} + LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} + run: uv run python -m scripts.api_breaking release --base "$BASE" --labels "$LABELS" diff --git a/scripts/api_breaking.py b/scripts/api_breaking.py new file mode 100644 index 00000000..c3f6947b --- /dev/null +++ b/scripts/api_breaking.py @@ -0,0 +1,253 @@ +"""Fail on an undeclared breaking change to the public API. + + uv run python -m scripts.api_breaking pr --base --title [--labels a,b] + uv run python -m scripts.api_breaking release --base [--labels a,b] + +``pr`` compares each public module with ``--base``. A breaking change passes only if +the PR title declares it with ``!`` (``feat!: ...``, ``fix(client)!: ...``), which makes +release-please cut a major version (a minor one before 1.0.0), or if an owner applied +the ``api-compatible`` label because they judge the flagged change compatible. + +``release`` runs on the release PR. For each package whose version changes, it +compares the package's public modules with its last release tag, and fails on a +breaking change unless the new version is a major one (a minor one before 1.0.0), or +an owner applied the ``api-compatible`` label to the release PR. + +Only names in ``__all__`` count. Experimental modules are skipped: an experimental +feature may change in a minor release. +""" + +from __future__ import annotations + +import argparse +import json +import os +import re +import subprocess +import sys +from pathlib import Path +from typing import Any + +import griffe + +from scripts.api_report import ROOT, public_modules, resolve + +BREAKING_TITLE = re.compile(r"^[a-z]+(\([^)]*\))?!:") +COMPATIBLE_LABEL = "api-compatible" + + +def stable_modules(root: Path) -> dict[str, Path]: + return {m: p for m, p in public_modules(root).items() if ".experimental." not in m} + + +def load(root: Path, ref: str | None, tops: set[str]) -> griffe.ModulesCollection: + """Load the top-level packages ``tops`` from the working tree, or from ``ref``.""" + loader = griffe.GriffeLoader( + search_paths=sorted(str(p) for p in root.glob("packages/*/src")) + ) + relative = sorted(str(p.relative_to(root)) for p in root.glob("packages/*/src")) + for top in sorted(tops): + if ref is None: + loader.load(top) + continue + try: + griffe.load_git( + top, + ref=ref, + repo=root, + search_paths=relative, + modules_collection=loader.modules_collection, + ) + except (ImportError, griffe.GitError): # the package did not exist yet + pass + loader.resolve_aliases(external=False, implicit=False) + return loader.modules_collection + + +def compare( + old: griffe.ModulesCollection, new: griffe.ModulesCollection, modules: list[str] +) -> list[str]: + """Explain every breaking change to the ``__all__`` names of ``modules``. + + griffe compares the two modules' public members: with ``__all__`` defined, only + its names, following re-exports to where they are defined. A name dropped from + ``__all__`` while its object still exists is breaking too, and griffe does not + see it, so it is checked here. + """ + style = ( + griffe.ExplanationStyle.GITHUB + if os.environ.get("GITHUB_ACTIONS") + else griffe.ExplanationStyle.MARKDOWN + ) + found: list[str] = [] + for module in modules: + try: + old_module = old.get_member(module) + except KeyError: # a new module breaks nothing + continue + try: + new_module = new.get_member(module) + except KeyError: + found.append(f"{module}: the module was removed") + continue + dropped = sorted( + set(map(str, old_module.exports or [])) + - set(map(str, new_module.exports or [])) + ) + found += [f"{module}.{name}: removed from __all__" for name in dropped] + found += [ + b.explain(style) + for b in griffe.find_breaking_changes(old_module, new_module) + if not ( + b.kind is griffe.BreakageKind.OBJECT_REMOVED and b.obj.name in dropped + ) + and ".experimental." not in f"{b.obj.path}." + ] + shared = sorted( + set(map(str, old_module.exports or [])) + & set(map(str, new_module.exports or [])) + ) + for name in shared: + found += returns_changed( + f"{module}.{name}", + resolve(old, old_module, name), + resolve(new, new_module, name), + ) + return found + + +def returns_changed(path: str, old: Any, new: Any) -> list[str]: + """griffe does not flag a changed return type, which callers may not be able to handle.""" + pairs = [(path, old, new)] + if old.is_class and new.is_class: + pairs += [ + (f"{path}.{name}", member, new.members[name]) + for name, member in old.members.items() + if not name.startswith("_") and name in new.members + ] + return [ + f"{where}: return type changed from {o.returns} to {n.returns}" + for where, o, n in pairs + if not o.is_alias + and not n.is_alias + and o.is_function + and n.is_function + and str(o.returns) != str(n.returns) + ] + + +def tops_of(modules: list[str]) -> set[str]: + return {m.split(".")[0] for m in modules} + + +def check_pr(root: Path, base: str, title: str, labels: list[str]) -> int: + modules = sorted(stable_modules(root)) + found = compare( + load(root, base, tops_of(modules)), load(root, None, tops_of(modules)), modules + ) + if not found: + print("No breaking changes to the public API.") + return 0 + print("\n".join(found)) + if BREAKING_TITLE.match(title): + print( + "\nDeclared breaking with `!` in the PR title: release-please will cut a major version " + "(a minor one before 1.0.0)." + ) + return 0 + if COMPATIBLE_LABEL in labels: + print( + f"\nAn owner judged these changes compatible (`{COMPATIBLE_LABEL}` label)." + ) + return 0 + print( + f"\n{len(found)} breaking change(s) to the public API. Declare a breaking change with `!` " + "in the PR title (`feat!: ...`, `fix(scope)!: ...`). If an owner judges the change " + f"compatible, they apply the `{COMPATIBLE_LABEL}` label instead." + ) + return 1 + + +def version(text: str) -> tuple[int, int, int]: + major, minor, patch = (int(n) for n in re.findall(r"\d+", text)[:3]) + return major, minor, patch + + +def allows_breaking(old: str, new: str) -> bool: + """True if ``old`` → ``new`` is a major bump, or a minor one before 1.0.0.""" + (o_major, o_minor, _), (n_major, n_minor, _) = version(old), version(new) + return n_major > o_major or (o_major == n_major == 0 and n_minor > o_minor) + + +def git(root: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run(["git", *args], cwd=root, capture_output=True, text=True) + + +def check_release(root: Path, base: str, labels: list[str] | None = None) -> int: + config = json.loads((root / "release-please-config.json").read_text())["packages"] + new_versions = json.loads((root / ".release-please-manifest.json").read_text()) + old_versions = json.loads( + git(root, "show", f"{base}:.release-please-manifest.json").stdout + ) + by_package: dict[str, list[str]] = {} + for module, package_dir in stable_modules(root).items(): + by_package.setdefault(str(package_dir.relative_to(root)), []).append(module) + failed = False + for package, new in sorted(new_versions.items()): + old = old_versions.get(package) + modules = sorted(by_package.get(package, [])) + if old is None or old == new or not modules: + continue + tag = f"{config[package]['component']}-{old}" + if git(root, "rev-parse", "--verify", "--quiet", f"refs/tags/{tag}").returncode: + print( + f"{package}: no release tag {tag}, so there is nothing to compare with" + ) + continue + found = compare( + load(root, tag, tops_of(modules)), + load(root, None, tops_of(modules)), + modules, + ) + if not found or allows_breaking(old, new): + print(f"{package}: {old} -> {new} matches its changes since {tag}") + continue + failed = True + print("\n".join(found)) + print( + f"{package}: {old} -> {new}, but the changes above since {tag} are breaking. " + "It needs a major version (a minor one before 1.0.0): a merged PR that made one " + "of them was not declared with `!` in its title." + ) + if failed and COMPATIBLE_LABEL in (labels or []): + print( + f"\nAn owner judged these changes compatible (`{COMPATIBLE_LABEL}` label)." + ) + return 0 + if failed: + print( + f"\nIf an owner judges these changes compatible, they apply the `{COMPATIBLE_LABEL}` " + "label to this release PR." + ) + return 1 if failed else 0 + + +def main(argv: list[str]) -> int: + parser = argparse.ArgumentParser(description=__doc__.split("\n\n")[0]) + sub = parser.add_subparsers(dest="command", required=True) + pr = sub.add_parser("pr") + pr.add_argument("--base", required=True) + pr.add_argument("--title", required=True) + pr.add_argument("--labels", default="") + release = sub.add_parser("release") + release.add_argument("--base", required=True) + release.add_argument("--labels", default="") + args = parser.parse_args(argv) + labels = [label.strip() for label in args.labels.split(",") if label.strip()] + if args.command == "pr": + return check_pr(ROOT, args.base, args.title, labels) + return check_release(ROOT, args.base, labels) + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/api_report.py b/scripts/api_report.py index ca1f2f9b..45c4386b 100644 --- a/scripts/api_report.py +++ b/scripts/api_report.py @@ -41,7 +41,7 @@ def public_modules(root: Path) -> dict[str, Path]: return found -def resolve(loader: griffe.GriffeLoader, module: Any, name: str) -> Any: +def resolve(collection: griffe.ModulesCollection, module: Any, name: str) -> Any: """Return what ``from module import name`` gives an application. An imported name wins over a submodule of the same name, as it does at run @@ -51,7 +51,7 @@ def resolve(loader: griffe.GriffeLoader, module: Any, name: str) -> Any: path = module.imports.get(name) if path: try: - obj = loader.modules_collection.get_member(path) + obj = collection.get_member(path) except KeyError: # imported from another distribution; keep the alias pass if obj is None: @@ -130,7 +130,9 @@ def report(loader: griffe.GriffeLoader, module_path: str) -> str: raise SystemExit(f"{module_path}: no __all__") lines = [HEADER.format(module=module_path).rstrip("\n")] for name in sorted(module.exports, key=lambda n: (n.lower(), n)): - lines += describe(resolve(loader, module, str(name)), str(name)) + lines += describe( + resolve(loader.modules_collection, module, str(name)), str(name) + ) return "\n".join(lines) + "\n" diff --git a/tests/test_api_breaking.py b/tests/test_api_breaking.py new file mode 100644 index 00000000..c695021b --- /dev/null +++ b/tests/test_api_breaking.py @@ -0,0 +1,218 @@ +"""Breaking changes to the public API must be declared, and releases must match them.""" + +from __future__ import annotations + +import json +import subprocess +import textwrap +from pathlib import Path + +import pytest + +from scripts import api_breaking + +V1 = """ +from demo_sdk.run import run + +def make(value: str) -> str: + return value + +__all__ = ["make", "run"] +""" +RUN = "def run(key: str) -> str:\n return key\n" +BETA = '__all__ = ["try_it"]\n\ndef try_it() -> None: ...\n' + + +def git(repo: Path, *args: str) -> None: + subprocess.run( + ["git", *args], + cwd=repo, + check=True, + capture_output=True, + ) + + +def write(path: Path, text: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(textwrap.dedent(text)) + + +@pytest.fixture +def repo(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """A one-package repository whose first commit is released as ``demo-sdk-1.2.0``.""" + monkeypatch.delenv("GITHUB_ACTIONS", raising=False) + # Keep a developer's global git config (signing, hooks) out of these repositories, + # including the git that griffe runs to read an old ref. + gitconfig = tmp_path.parent / f"{tmp_path.name}.gitconfig" + gitconfig.write_text("[user]\n\tname = test\n\temail = test@example.com\n") + monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(gitconfig)) + monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1") + src = tmp_path / "packages/demo/src/demo_sdk" + write(src / "__init__.py", V1) + write(src / "run.py", RUN) + write(src / "experimental/__init__.py", "") + write(src / "experimental/beta.py", BETA) + write( + tmp_path / "release-please-config.json", + json.dumps({"packages": {"packages/demo": {"component": "demo-sdk"}}}), + ) + write( + tmp_path / ".release-please-manifest.json", + json.dumps({"packages/demo": "1.2.0"}), + ) + git(tmp_path, "init", "-q") + git(tmp_path, "add", "-A") + git(tmp_path, "commit", "-q", "-m", "v1") + git(tmp_path, "tag", "demo-sdk-1.2.0") + return tmp_path + + +def change(repo: Path, relative: str, text: str) -> None: + write(repo / "packages/demo/src/demo_sdk" / relative, text) + git(repo, "commit", "-q", "-am", "change") + + +def breakages(repo: Path) -> list[str]: + modules = sorted(api_breaking.stable_modules(repo)) + tops = api_breaking.tops_of(modules) + return api_breaking.compare( + api_breaking.load(repo, "demo-sdk-1.2.0", tops), + api_breaking.load(repo, None, tops), + modules, + ) + + +def test_no_change_breaks_nothing(repo: Path) -> None: + assert breakages(repo) == [] + + +@pytest.mark.parametrize( + ("source", "expected"), + [ + ("def run(key: str, strict: bool) -> str: ...\n", "added as required"), + ("def run(name: str) -> str: ...\n", "removed"), + ("def run(key: str) -> int: ...\n", "return type changed from str to int"), + ], +) +def test_a_breaking_signature_change_is_found( + repo: Path, source: str, expected: str +) -> None: + change(repo, "run.py", source) + found = breakages(repo) + assert len(found) >= 1 + assert any(expected.lower() in line.lower() for line in found), found + + +def test_an_added_optional_parameter_or_name_breaks_nothing(repo: Path) -> None: + change(repo, "run.py", "def run(key: str, strict: bool = False) -> str: ...\n") + change( + repo, + "__init__.py", + V1.replace('"make", "run"', '"extra", "make", "run"') + "extra = 1\n", + ) + assert breakages(repo) == [] + + +def test_a_name_removed_from_all_is_breaking(repo: Path) -> None: + change(repo, "__init__.py", V1.replace('"make", "run"', '"run"')) + assert breakages(repo) == ["demo_sdk.make: removed from __all__"] + + +def test_an_experimental_module_may_break(repo: Path) -> None: + change( + repo, + "experimental/beta.py", + '__all__ = ["try_it"]\n\ndef try_it(x: int) -> None: ...\n', + ) + assert breakages(repo) == [] + + +@pytest.mark.parametrize( + ("title", "labels", "code"), + [ + ("feat: change run", [], 1), + ("feat!: change run", [], 0), + ("fix(client)!: change run", [], 0), + ("feat: !important", [], 1), + ("feat: change run", ["api-compatible"], 0), + ], +) +def test_a_breaking_pr_must_declare_it( + repo: Path, title: str, labels: list[str], code: int +) -> None: + change(repo, "run.py", "def run(key: str, strict: bool) -> str: ...\n") + assert api_breaking.check_pr(repo, "demo-sdk-1.2.0", title, labels) == code + + +def test_a_pr_without_breaking_changes_passes(repo: Path) -> None: + assert api_breaking.check_pr(repo, "demo-sdk-1.2.0", "feat: nothing", []) == 0 + + +def release( + repo: Path, + released: str, + proposed: str, + breaking: bool, + labels: list[str] | None = None, +) -> int: + """Release ``released`` as tagged, then propose ``proposed`` the way a release PR does.""" + git(repo, "tag", "-d", "demo-sdk-1.2.0") + write( + repo / ".release-please-manifest.json", json.dumps({"packages/demo": released}) + ) + git(repo, "commit", "-q", "--allow-empty", "-am", "released") + git(repo, "tag", f"demo-sdk-{released}") + base = subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=repo, + capture_output=True, + text=True, + check=True, + ).stdout.strip() + if breaking: + change(repo, "run.py", "def run(key: str, strict: bool) -> str: ...\n") + write( + repo / ".release-please-manifest.json", json.dumps({"packages/demo": proposed}) + ) + git(repo, "commit", "-q", "-am", "release PR") + return api_breaking.check_release(repo, base, labels) + + +@pytest.mark.parametrize( + ("released", "proposed", "breaking", "code"), + [ + ("1.2.0", "1.3.0", True, 1), + ("1.2.0", "1.2.1", True, 1), + ("1.2.0", "2.0.0", True, 0), + ("1.2.0", "1.3.0", False, 0), + ("0.3.0", "0.4.0", True, 0), + ("0.3.0", "0.3.1", True, 1), + ("0.9.0", "1.0.0", True, 0), + ], +) +def test_a_release_version_matches_its_changes( + repo: Path, released: str, proposed: str, breaking: bool, code: int +) -> None: + assert release(repo, released, proposed, breaking) == code + + +def test_a_package_with_no_release_tag_is_skipped( + repo: Path, capsys: pytest.CaptureFixture[str] +) -> None: + base = "HEAD" + git(repo, "tag", "-d", "demo-sdk-1.2.0") + write( + repo / ".release-please-manifest.json", json.dumps({"packages/demo": "1.3.0"}) + ) + git(repo, "commit", "-q", "-am", "release PR") + assert api_breaking.check_release(repo, f"{base}~1") == 0 + assert "no release tag demo-sdk-1.2.0" in capsys.readouterr().out + + +def test_an_owner_can_judge_a_release_compatible(repo: Path) -> None: + # The label passes an undeclared breaking change on the release PR, as on any other PR. + assert release(repo, "1.2.0", "1.2.1", True, ["api-compatible"]) == 0 + + +def test_another_label_does_not_pass_a_breaking_release(repo: Path) -> None: + assert release(repo, "1.2.0", "1.2.1", True, ["docs"]) == 1