diff --git a/.agents/skills/release-notes/SKILL.md b/.agents/skills/release-notes/SKILL.md index d7ac4839506..a2c4d972f3e 100644 --- a/.agents/skills/release-notes/SKILL.md +++ b/.agents/skills/release-notes/SKILL.md @@ -23,8 +23,8 @@ to read. If yes, write about it. If no (CI tweaks, internal refactors, doc/workflow plumbing, test infra), leave it out — the renderer already collapses that noise. `data.json` tags every PR `product` / `mixed` / `internal`; treat `internal` as -invisible unless it changed shipped behaviour, and for `mixed` judge from the -title. +invisible unless it changed shipped behaviour, and for `mixed` (build config in +`native/`, or a `docs` API-docs bump) judge from the title. ## Running the full pipeline (prepare → write prose → render) diff --git a/documentation/dev/release-notes-and-api-diffs.md b/documentation/dev/release-notes-and-api-diffs.md index c96076a2892..0f2b9ff24f3 100644 --- a/documentation/dev/release-notes-and-api-diffs.md +++ b/documentation/dev/release-notes-and-api-diffs.md @@ -342,6 +342,8 @@ scripts/infra/docs/ (all doc engines, together) prunes stale -unreleased pages and retires HarfBuzz hub pages release-notes-schema/prose.schema.json the prose contract the agent fills versions.json supersession + baseline config (§1); shared repo-wide + release-notes-paths.json PR path→tag classification (§4.4): the deterministic, + editable product/mixed/internal path map read by release-notes-data.py docker/ reproducible image + run.sh wrapper; `api-diffs` invokes the `docs-api-diff` target directly @@ -987,7 +989,7 @@ built from. It is timestamp-free and includes, at minimum: - `harfbuzz` on released pages — `{ "version", "api_diff_link", "prs" }` for the co-shipped HarfBuzzSharp section (§4.5). It is absent on `-unreleased` pages. - `prs` — the flat PR map, including title, URL, author, `community`, and the - deterministic `tag` (`product`, `mixed`, `internal`). + deterministic `tag` (`product`, `mixed`, or `internal`). - `contributors` — the authoritative non-maintainer, non-bot roster the renderer uses for the community table. - `previews` — per-preview/RC buckets, when present. Each carries a `key`, the human @@ -1034,18 +1036,33 @@ principles are fixed here. every PR is whether the **shipped SkiaSharp/HarfBuzzSharp library** — its public API, runtime behavior, native binary, or NuGet package — changed for a consumer. Prepare makes this deterministic: `release-notes-data.py` tags every PR in `data.json` as - **`product`**, **`mixed`**, or **`internal`** by the files it changed. - - **`product`** — touches shipped code (`binding/`, `externals/`, `source/`). Written up. - - **`internal`** — touches none of those (CI, workflows, agent skills, docs site, - tests, samples, build/meta). Dropped into the one collapse line. - - **`mixed`** — touches only build config (`native/`): it may change the shipped - binary via a compile flag or be pure infra. Polish judges from the title/context - already in `data.json`; it does not open the PR. - - `native/` is deliberately **not** treated as shipped code: it is build - configuration, and the thing that actually ships is `externals/skia/`. Polish drops - `internal`, writes up `product`, and inspects `mixed`; moving the classification out - of the LLM makes product-focus reliable run-to-run. + **`product`**, **`mixed`**, or **`internal`** by the files it changed. The path→tag + mapping is **not** hardcoded in the script — it lives in + [`scripts/infra/docs/release-notes-paths.json`](../../scripts/infra/docs/release-notes-paths.json), + the single deterministic place to edit it. It is a short list of ordered tiers (each a + `tag` + `patterns`) plus a `default`; the first tier with a matching file wins. A pattern + matches by prefix (`str.startswith`), or as a glob if it contains `* ? [`. + - **`product`** — touches shipped code with a real API / behaviour / native change: + `binding/` + `source/` (managed API & Views) and `externals/skia` (the native Skia + submodule, with its vendored HarfBuzz). Written up. + - **`mixed`** — affects the shipped package but is not itself an API/behaviour change, so + Polish judges from the title: `native/` (per-platform build config — compile flags/gn + args that shape the native binaries, usually infra) and `docs` (the mdoc API-docs + submodule that ships as IntelliSense XML — doc content, not behaviour). + - **`internal`** — the `default`: touches none of those (CI, workflows, agent skills, docs + *site*, tests, samples, build/meta, and the `externals/depot_tools` build-toolchain + submodule). Dropped into the one collapse line. + + `native/` shapes the shipped binaries and `docs` ships as doc XML, so neither is + `internal`; but neither is a direct API/behaviour change, so both are `mixed` (inspected + from the title) rather than firm `product`. `docs` and `externals/skia` are submodules, so + in the parent repo they appear as bare gitlink paths (`docs`, `externals/skia`) and the + prefixes match those exactly — the `externals/skia` prefix is deliberately not just + `externals/`, which would sweep in the sibling `externals/depot_tools` build-toolchain + submodule (internal) and `externals/.gitignore`; the `docs` prefix is slash-less so it hits + the gitlink without colliding with `documentation/`. Polish drops `internal`, writes up + `product`, and inspects `mixed`; moving the classification out of the LLM (and into the + JSON) makes product-focus reliable run-to-run. 2. **Highlights are a hook, not a summary.** The `## Highlights` section always exists and is assembled by `release-notes-render.py`. The prose targets ~80 words and is hard-capped at 100 words total across `highlights_headline` + `highlights_body`, naming only the diff --git a/scripts/infra/docs/release-notes-data.py b/scripts/infra/docs/release-notes-data.py index a1fbacf03e8..acdaa35afd1 100644 --- a/scripts/infra/docs/release-notes-data.py +++ b/scripts/infra/docs/release-notes-data.py @@ -94,7 +94,7 @@ from __future__ import annotations -import argparse +import fnmatch import hashlib import json import re @@ -141,6 +141,12 @@ VERSIONS_JSON_PATH = Path("scripts/infra/docs/versions.json") +# PR path-classification config (loaded lazily from +# scripts/infra/docs/release-notes-paths.json). The single deterministic place that +# owns which touched paths map to which release-note tag — see _pr_category(). +PATH_TAGS_JSON_PATH = Path("scripts/infra/docs/release-notes-paths.json") +_PATH_TAGS_CONFIG = None # type: Optional[tuple] + # HarfBuzz-owned paths (spec §1.5/§4.5). A HarfBuzz family page lists ONLY the # commits touching these — the HarfBuzzSharp binding + its native assets, the # native libHarfBuzzSharp build, and HarfBuzz tests — NEVER any SkiaSharp.* source @@ -1305,26 +1311,61 @@ def determine_diff_range(branch): # can FOCUS on product, INSPECT mixed, and MENTION internal (§4.4) — a lexical filter # instead of re-judging every PR from its title each run (the source of leak variance). # -# product — touches SHIPPED code (managed API, native code, Views). A real change a -# consumer can see. Companion test/benchmark/generated files are ignored. -# mixed — touches only BUILD config (native/): may change the shipped binary via a -# compile define (e.g. a rasteriser flag), or may be pure infra (Docker -# image, SDK pin). Polish guesses from the title/context in data.json -# (it does not open the PR) — surface a behaviour change, drop pure infra. -# internal — touches NEITHER: a pure repository process (CI, workflows, agent skills, -# docs site, tests, samples, build/meta files). Dropped into the collapse line. -_SHIP_PATH_PREFIXES = ("binding/", "externals/", "source/") -_BUILD_PATH_PREFIXES = ("native/",) +# The path→tag mapping is NOT hardcoded here: it lives in the committed +# scripts/infra/docs/release-notes-paths.json, the single deterministic place to edit it +# (ordered tiers of prefixes/globs + a default). See that file's `description`/`notes` +# and spec §4.4 for the rationale (why `externals/skia` is exact, why `docs` is +# slash-less, why `native/` is mixed, etc.). + + +def _load_path_tags(): + # type: () -> tuple + """Load release-notes-paths.json as ``([(tag, patterns), ...], default)`` (cached). + + Ordered tiers + a default tag — the deterministic PR path-classification config + (spec §4.4). Read once and memoised; a missing/malformed file is a hard error (the + file is committed and required — do not silently fall back to a guessed mapping). + """ + global _PATH_TAGS_CONFIG + if _PATH_TAGS_CONFIG is None: + if not PATH_TAGS_JSON_PATH.exists(): + raise FileNotFoundError( + "Missing %s — the PR path-classification config (spec §4.4)." + % PATH_TAGS_JSON_PATH) + with open(PATH_TAGS_JSON_PATH) as f: + cfg = json.load(f) + tiers = [(t["tag"], tuple(t.get("patterns", []))) + for t in cfg.get("tiers", [])] + _PATH_TAGS_CONFIG = (tiers, cfg.get("default", "internal")) + return _PATH_TAGS_CONFIG + + +def _path_matches(path, pattern): + # type: (str, str) -> bool + """Does a file ``path`` match a config ``pattern``? + + Plain patterns are PREFIXES (``str.startswith`` — so ``binding/`` catches everything + under it and a bare submodule gitlink like ``externals/skia`` / ``docs`` matches + itself). A pattern containing a glob metacharacter (``* ? [``) is matched with + ``fnmatch.fnmatchcase`` (case-sensitive, platform-independent) instead. + """ + if any(c in pattern for c in "*?["): + return fnmatch.fnmatchcase(path, pattern) + return path.startswith(pattern) def _pr_category(files): # type: (set) -> str - """Classify a PR ``product`` / ``mixed`` / ``internal`` by touched files (§4.4).""" - if any(f.startswith(_SHIP_PATH_PREFIXES) for f in files): - return "product" - if any(f.startswith(_BUILD_PATH_PREFIXES) for f in files): - return "mixed" - return "internal" + """Classify a PR ``product`` / ``mixed`` / ``internal`` by touched files (§4.4). + + Tiers from release-notes-paths.json are tried in order; the first tier with any + matching file wins, else the configured default. + """ + tiers, default = _load_path_tags() + for tag, patterns in tiers: + if any(_path_matches(f, p) for f in files for p in patterns): + return tag + return default # Automation accounts — never credited as human contributors (§4.5). The workflow @@ -1371,17 +1412,31 @@ def _files_by_commit(from_ref, to_ref): One ``git log --name-only`` call (no pathspec filter — we want each PR's FULL file set to classify it product vs internal). A ``\\x1e`` record separator prefixes each hash line so file lines can never be mistaken for a hash. + + We SPLIT on ``\\x1e`` rather than matching a leading ``\\x1e`` per line: git + lists commits newest-first, and ``run()`` returns ``stdout.strip()`` — and + ``\\x1e`` is whitespace to Python's ``str.strip()``, so the FIRST (newest) + record loses its leading separator. A ``startswith("\\x1e")`` scan would then + silently drop that newest commit's file list, tagging it ``internal`` — which + is exactly how the latest automated ``[skia-sync]`` submodule bump (usually the + newest commit in the range) kept getting mis-tagged. Splitting recovers every + record whether or not its leading separator survived. """ out = run(["git", "log", "--no-renames", "--name-only", "--format=%x1e%H", "{}..{}".format(from_ref, to_ref)]) files_by = {} # type: dict - cur = None - for line in out.split("\n"): - if line.startswith("\x1e"): - cur = line[1:].strip() - files_by[cur] = set() - elif line.strip() and cur is not None: - files_by[cur].add(line.strip()) + for record in out.split("\x1e"): + record = record.strip() + if not record: + continue + # First line is the commit hash; the rest (past the blank line git emits + # before --name-only output) are the touched paths. Merge commits have no + # --name-only output, so they map to an empty set (still `internal`). + lines = record.split("\n") + commit = lines[0].strip() + if not commit: + continue + files_by[commit] = {ln.strip() for ln in lines[1:] if ln.strip()} return files_by diff --git a/scripts/infra/docs/release-notes-paths.json b/scripts/infra/docs/release-notes-paths.json new file mode 100644 index 00000000000..117c8c3db6e --- /dev/null +++ b/scripts/infra/docs/release-notes-paths.json @@ -0,0 +1,30 @@ +{ + "description": "Path-based PR classification for the release notes. release-notes-data.py reads this file to tag every merged PR by the files it touched (behaviour spec: documentation/dev/release-notes-and-api-diffs.md, section 4.4). This JSON is the single deterministic place to edit the mapping - the script does not hardcode which paths are product/mixed/internal.", + "matching": "Tiers are evaluated top-to-bottom; the FIRST tier with any file matching any of its patterns wins, otherwise `default`. A pattern matches a file path by PREFIX (str.startswith) - so `binding/` catches everything under it, and a bare submodule gitlink such as `externals/skia` or `docs` matches itself. A pattern that contains a glob metacharacter (* ? [) is matched with fnmatch instead (e.g. `native/*/libHarfBuzzSharp/**`).", + "tiers": [ + { + "tag": "product", + "why": "Shipped code - a real API / behaviour / native change a consumer can see. Written up in the notes.", + "patterns": [ + "binding/", + "source/", + "externals/skia" + ] + }, + { + "tag": "mixed", + "why": "Affects the shipped package but is not itself an API/behaviour change; Polish judges from the PR title/context and surfaces only a real behaviour change, dropping pure infra / doc churn.", + "patterns": [ + "native/", + "docs" + ] + } + ], + "default": "internal", + "notes": [ + "`externals/skia` is intentionally exact, NOT `externals/`: the sibling `externals/depot_tools` (build toolchain) and `externals/.gitignore` are build infra and must stay `default` (internal).", + "`docs` is the mono/SkiaSharp-API-docs submodule (mdoc XML that ships as the packages' IntelliSense docs). It is slash-less so it matches the bare `docs` gitlink and does not collide with `documentation/` - the docs site, which stays internal.", + "`native/` covers the per-platform build config (gn args / compile flags) and the shipped WinUI native subtree under `native/winui/`.", + "Anything not matched (CI, workflows, agent skills, the `documentation/` docs site, tests, samples, build/meta files, and `externals/depot_tools`) falls through to `default`." + ] +}