diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 4435398f72b..4c0df28b323 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -9,6 +9,7 @@ Include the following checklist in your PR: * [ ] Patches conform to the [coding conventions](https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#other-conventions). * [ ] Any changes that could be relevant to users [have been recorded in the changelog](https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#changelog). * [ ] [Is the change significant?](https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#is-my-change-significant) If so, remember to add `significance: significant` in the changelog file. +* [ ] The `Check API` job is clean, or the API changes are documented in a changelog file. * [ ] The documentation has been updated, if necessary. * [ ] [Manual QA notes](https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#qa-notes) have been included. * [ ] Tests have been added. (*Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!*) @@ -22,4 +23,5 @@ Include the following checklist in your PR: Include the following checklist in your PR: * [ ] Patches conform to the [coding conventions](https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#other-conventions). +* [ ] The `Check API` job is clean, or the API changes are documented in a changelog file. * [ ] Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions). diff --git a/.github/scripts/api-changelog-check.py b/.github/scripts/api-changelog-check.py new file mode 100644 index 00000000000..852d4c76ec9 --- /dev/null +++ b/.github/scripts/api-changelog-check.py @@ -0,0 +1,79 @@ +import os +import re +import sys + +# Checks that an API change found by packdiff is documented in a changelog +# entry added by the PR: +# 1. (blocking) some changelog file added/modified by the PR must list the +# package in its `packages:` frontmatter field; +# 2. (warning) the entries matching the package should mention at least one +# of the changed module/declaration names from the packdiff output. +# +# Usage: api-changelog-check.py PACKAGE API_DIFF_FILE [CHANGELOG_FILE ...] + +package = sys.argv[1] +diff_path = sys.argv[2] +changelog_files = sys.argv[3:] + +# ---- collect the changed names from the packdiff output --------------------- + +names = set() +for line in open(diff_path, encoding="utf-8"): + m = re.match(r"^\[[ARC]\] ([\w'.]+)", line) # top level: a changed module + if m: + names.add(m.group(1)) + continue + m = re.match(r"^\s+\[[ARCD]\] ([\w'.]+)", line) # nested: a declaration + if m: + names.add(m.group(1).rsplit(".", 1)[-1]) + +# ---- parse the `packages:` field of each changelog file --------------------- + + +def front_packages(path): + text = open(path, encoding="utf-8").read() + # Entries are either .cabal-style key/value files or markdown files with a + # YAML front matter; in both cases `packages:` is a top-level line. + m = re.search(r"^packages:\s*(.+?)\s*$", text, re.M) + if not m: + return [] + return [p for p in re.split(r"[\s,\[\]#]+", m.group(1)) if p] + + +matching = [p for p in changelog_files if package in front_packages(p)] + +if not matching: + listing = "\n".join(f" - {p}" for p in changelog_files) or " (none)" + print( + f"::error::The API of {package} changed with respect to the base" + f" revision, but none of the changelog entries added by this PR lists" + f" {package} in its `packages:` field. See the job summary for the" + f" diff. Add or update an entry under changelog.d/ with" + f" `packages: [{package}]`, or revert the API change." + f"\nChangelog files changed by this PR:\n{listing}" + ) + sys.exit(1) + +# ---- soft check: do the entries mention the changed API? -------------------- + +text = "\n".join(open(p, encoding="utf-8").read() for p in matching) + + +def mentioned(name): + pattern = r"(?> "$GITHUB_OUTPUT" + + - uses: actions/cache@v6 + with: + path: ${{ steps.setup-haskell.outputs.cabal-store }} + key: ${{ steps.get-osver.outputs.osver }}-check-api-${{ env.GHC_VERSION }}-${{ github.sha }} + restore-keys: ${{ steps.get-osver.outputs.osver }}-check-api-${{ env.GHC_VERSION }}- + + - name: "Work around git problem https://bugs.launchpad.net/ubuntu/+source/git/+bug/1993586 (cabal PR #8546)" + run: git config --global protocol.file.allow always + + - name: Install packdiff + run: | + mkdir -p "$HOME/.local/bin" + curl -sSL "https://github.com/composewell/packdiff/archive/${PACKDIFF_COMMIT}.tar.gz" | tar -xz -C "$RUNNER_TEMP" + cd "$RUNNER_TEMP/packdiff-${PACKDIFF_COMMIT}" + cabal install exe:packdiff --installdir="$HOME/.local/bin" --overwrite-policy=always + + - name: Run packdiff + run: | + set -o pipefail + case "${{ github.event_name }}" in + pull_request) base="${{ github.event.pull_request.base.sha }}" ;; + push) base="${{ github.event.before }}" ;; + *) base="" ;; + esac + # github.event.before (push) is the all-zero commit id when the + # branch has no previous commit, e.g. it was newly created or + # force-pushed; fall back to the merge base in that case. + zero_sha="0000000000000000000000000000000000000000" + if [ -z "$base" ] || [ "$base" = "$zero_sha" ]; then + base="$(git merge-base origin/master HEAD)" + fi + head="$(git rev-parse HEAD)" + echo "Diffing ${{ matrix.package }} API: $base -> $head" + packdiff diff ${{ matrix.package }} "$base" ${{ matrix.package }} "$head" | tee api-diff.txt + + sed -n '/API Annotations/,$p' api-diff.txt > api-diff-summary.txt + { + echo "## API diff \`${{ matrix.package }}\`: \`$(git rev-parse --short "$base")\` -> \`$(git rev-parse --short "$head")\`" + echo '```' + cat api-diff-summary.txt + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + + # Diff entries look like "[C] Module.Name"; the annotation legend + # ("[A] : Added") has a colon right after the marker and must not match. + if ! grep -qE '^\[[ARC]\] [^ :]' api-diff.txt; then + echo "No API changes detected in ${{ matrix.package }}." + exit 0 + fi + + # The API changed. This is acceptable when the PR documents the + # change in a changelog entry (see the PR template checklist): + # the job passes, but keeps showing the diff in its summary. + changed_changelogs="$(git diff --name-only --diff-filter=AM "$base" "$head" -- changelog.d/)" + if [ -z "$changed_changelogs" ]; then + echo "::error::The API of ${{ matrix.package }} changed with respect to the base revision, and this PR adds no changelog entry. See the job summary for the diff. Add a changelog file under changelog.d/ describing the change (or revert the API change)." + exit 1 + fi + python3 .github/scripts/api-changelog-check.py "${{ matrix.package }}" api-diff.txt $changed_changelogs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index dc34539b437..c854a4f47ae 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -174,6 +174,27 @@ fail annoyingly once you push it. `make checks` will do these checks. The list o checks is expected to grow over time, to make it easier to avoid CI turnaround on simple problems. +## API diff + +CI runs an informational "Check API" job (`.github/workflows/check-api.yml`) on +every pull request. It computes the public API diff of `Cabal-syntax`, `Cabal`, +`cabal-install`, `cabal-install-solver` and `Cabal-hooks` between the base revision and the head +of the PR using [packdiff](https://github.com/composewell/packdiff), and prints +it in the job log and the job summary. + +> [!NOTE] +> `packdiff` literally runs `git checkout` on both revisions. +> +> `packdiff` derives its output from the haddock hoogle files, so it does not cover `other-modules`. + +To run the same diff locally: + +```console +$ make api-install # once; installs packdiff from the pinned commit +$ make api-diff # all library packages, against origin/master +$ make api-diff PKG=Cabal-syntax API_BASE=3.14 # one package, against a branch/tag +``` + ## QA Notes Manual Quality Assurance (QA) is performed to ensure that the changes impacting diff --git a/Makefile b/Makefile index 9268d18e562..431c02bd588 100644 --- a/Makefile +++ b/Makefile @@ -223,6 +223,42 @@ cabal-install-test-accept: rm -rf .ghc.environment.* cd cabal-testsuite && `cabal list-bin cabal-tests` --with-cabal=`cabal list-bin cabal` --hide-successes -j3 --accept ${TEST} +# API diff (packdiff) +############################################################################## + +# https://github.com/composewell/packdiff; the same commit is pinned in +# .github/workflows/check-api.yml and used by the "Check API" CI job. +# packdiff is not on Hackage yet, so it is installed from the pinned commit. +PACKDIFF_COMMIT := 54e786de55f091cdd3b912bd72ccc0e5e252aa77 +PACKDIFF_URL := https://github.com/composewell/packdiff/archive/$(PACKDIFF_COMMIT).tar.gz + +API_PACKAGES := Cabal-syntax Cabal cabal-install cabal-install-solver Cabal-hooks +API_BASE ?= origin/master + +.PHONY: api-install +api-install: ## Install the packdiff tool used for API diffing. + rm -rf "$${TMPDIR:-/tmp}/packdiff-install" + mkdir -p "$${TMPDIR:-/tmp}/packdiff-install" + curl -sSL $(PACKDIFF_URL) | tar -xz -C "$${TMPDIR:-/tmp}/packdiff-install" + cd "$${TMPDIR:-/tmp}/packdiff-install/packdiff-$(PACKDIFF_COMMIT)" && \ + cabal install exe:packdiff --installdir=$(HOME)/.local/bin --overwrite-policy=always + +.PHONY: api-diff +api-diff: ## API diff of PKG (default: all library packages) between API_BASE (default: origin/master) and HEAD. + @command -v packdiff >/dev/null || { echo "packdiff not found; run 'make api-install'"; exit 1; } + @# NB: packdiff literally runs 'git checkout' on the given revisions, so: + @# * revisions are resolved to SHAs *before* running (a literal "HEAD" + @# would be re-resolved after switching to the base revision); + @# * the worktree must be clean (commit or stash first); + @# * the base revision must be buildable with the local GHC; + @# * on success the worktree is left at HEAD, on failure at the base. + @base=$$(git rev-parse $(API_BASE)); \ + head=$$(git rev-parse HEAD); \ + for pkg in $(if $(PKG),$(PKG),$(API_PACKAGES)); do \ + echo "== packdiff $$pkg: $(API_BASE) -> HEAD =="; \ + packdiff diff $$pkg $$base $$pkg $$head; \ + done + # Docker validation # Use this carefully, on big machine you can say