Skip to content

Fix release-notes classifier mis-tagging skia-sync PRs; make path tags data-driven#4410

Merged
mattleibow merged 3 commits into
mainfrom
mattleibow-skia-sync-internal-label
Jul 12, 2026
Merged

Fix release-notes classifier mis-tagging skia-sync PRs; make path tags data-driven#4410
mattleibow merged 3 commits into
mainfrom
mattleibow-skia-sync-internal-label

Conversation

@mattleibow

@mattleibow mattleibow commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

The release-notes generator (scripts/infra/docs/release-notes-data.py) tags every merged PR product / mixed / internal from the files it touched, so the Polish phase can focus on product changes. Automated [skia-sync] submodule bumps — which change the shipped native Skia — kept coming out internal and being dropped from the notes (e.g. 4.151.0-unreleased.data.json tags #4382 and 4.148.1-unreleased tags #4262 internal, despite both bumping externals/skia).

Root cause (commit 1)

_files_by_commit() runs git log --format=%x1e%H --name-only and parsed each record by matching a leading \x1e per line. But:

  1. run() returns result.stdout.strip(), and \x1e (record separator) is whitespace to Python's str.strip(), so the leading \x1e on the first record is stripped off.
  2. git log lists commits newest-first, so that first, separator-less record is the newest commit in the range — very often the latest [skia-sync] bump.

The line.startswith("\x1e") check then missed that commit, its file list was dropped, and an empty file set falls through to internal.

Fix: parse by splitting on \x1e, which recovers every record regardless of the stripped leading separator.

Sharpen the path tiers (commit 2)

  • product (real API/behaviour/native change): binding/ + source/ (managed API & Views) and externals/skia — exact, not externals/, so it no longer sweeps in the sibling externals/depot_tools build toolchain or externals/.gitignore.
  • mixed (affects the package, judged from the title): native/ (per-platform build config; incl. the shipped WinUI native subtree under native/winui/) and docs (the mono/SkiaSharp-API-docs submodule XML — moved up from internal; slash-less prefix to match the bare gitlink without colliding with documentation/).
  • internal: CI, workflows, agent skills, the docs site (documentation/), tests, samples, build/meta, and externals/depot_tools.

Make the mapping data-driven (commit 3)

The path→tag mapping is no longer hardcoded in the script. It now lives in scripts/infra/docs/release-notes-paths.json — ordered tiers (tag + patterns) plus a default — the single deterministic place to edit it:

{ "tiers": [ { "tag": "product", "patterns": ["binding/", "source/", "externals/skia"] },
             { "tag": "mixed",   "patterns": ["native/", "docs"] } ],
  "default": "internal" }

_pr_category loads the tiers (cached) and returns the first tier with a matching file, else the default. A pattern matches by prefix (str.startswith, unchanged behaviour) or, if it contains a glob metachar (* ? [), by fnmatch.fnmatchcase — so paths, prefixes, and wildcards are all supported. A missing/malformed config is a hard error (no silent fallback). Behaviour is identical to the previous hardcoded tuples.

Verification

PR / path tag
#4382 [skia-sync] Merge upstream chrome/m151 product
#4294 [skia-sync] Update Skia to milestone m151 product
#4325 Update docs submodule to latest main mixed
externals/depot_tools-only / externals/.gitignore internal
#3209 depot_tools + native/ mixed
native/-only mixed
#4292 docs: … site page (documentation/) internal

Config loads from disk, glob patterns work (fnmatch), a broad ~40-commit range stays a healthy mix, and the module compiles clean.

Notes

  • Committed historical *.data.json self-correct as pages regenerate on the next prepare.sh / scheduled release-notes run. No _DATA_JSON_FORMAT_VERSION bump (schema unchanged).
  • Docs updated: documentation/dev/release-notes-and-api-diffs.md (incl. the config-file inventory) and .agents/skills/release-notes/SKILL.md.

Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com

_files_by_commit() prefixed each commit with a \x1e record separator and
parsed by matching a leading \x1e per line. But run() returns
stdout.strip(), and \x1e is whitespace to Python's str.strip(), so the
first record's leading separator was stripped. Since git log lists
commits newest-first, the newest commit in every range lost its file
list and was classified as 'internal' by _pr_category() (empty file
set). On an active line the newest commit is very often the latest
automated [skia-sync] submodule bump, so those product PRs (they touch
externals/skia) were repeatedly mis-tagged 'internal' and dropped from
release notes.

Parse by splitting on \x1e instead, which recovers every record whether
or not its leading separator survived strip().

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

📦 Try the packages from this PR

Warning

Do not run these scripts without first reviewing the code in this PR.

Step 1 — Download the packages

bash / macOS / Linux:

curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4410

PowerShell / Windows:

iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4410"

Step 2 — Add the local NuGet source

dotnet nuget add source ~/.skiasharp/hives/pr-4410/packages --name skiasharp-pr-4410
More options
Option Description
--successful-only / -SuccessfulOnly Only use successful builds
--force / -Force Overwrite previously downloaded packages
--list / -List List available artifacts without downloading
--build-id ID / -BuildId ID Download from a specific build

Or download manually from Azure Pipelines — look for the nuget artifact on the build for this PR.

Remove the source when you're done:

dotnet nuget remove source skiasharp-pr-4410

@mattleibow
mattleibow force-pushed the mattleibow-skia-sync-internal-label branch from dfe8928 to 061e602 Compare July 12, 2026 06:11
@github-actions

Copy link
Copy Markdown
Contributor

📖 Documentation Preview

The documentation for this PR has been deployed and is available at:

🔗 View Staging Site
🔗 View Staging Docs
🔗 View Staging Gallery (Blazor)
🔗 View Staging Gallery (Uno Platform)
🔗 View Staging SkiaFiddle

This preview will be updated automatically when you push new commits to this PR.


This comment is automatically updated by the documentation staging workflow.

…to skia

The classifier tags each PR product / mixed / internal by touched files.
Refine the package-affecting-but-not-API paths and the externals/ split:

- product is the strong signal: binding/ + source/ (managed API & Views) and
  externals/skia (the native Skia submodule, with its vendored HarfBuzz). The
  prefix is exact 'externals/skia', not 'externals/', so it does NOT sweep in
  the sibling externals/depot_tools build-toolchain submodule or
  externals/.gitignore.
- native/ (per-platform build config: compile flags / gn args) shapes the
  shipped native binaries, so it is not internal — but a native/-only PR is
  usually infra, not a consumer-facing change, so it is 'mixed' (Polish judges
  from the title) rather than firm 'product'. Also covers the shipped WinUI
  native subtree under native/winui/.
- docs (the SkiaSharp-API-docs submodule, mdoc XML) ships as the packages'
  IntelliSense docs, so it is not internal — but a docs bump is doc content,
  not an API/behaviour change, so it is 'mixed' too (moved up from internal). A
  submodule bump appears in the parent as the bare 'docs' gitlink, so the prefix
  is slash-less (like externals/skia) and does not collide with 'documentation/'
  (the docs site, still internal).
- externals/depot_tools (build toolchain) and externals/.gitignore are build
  infra that never ships and does not shape the binary, so they stay internal.

Docs (SKILL.md, dev spec) updated to match.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mattleibow
mattleibow force-pushed the mattleibow-skia-sync-internal-label branch from 061e602 to fc98373 Compare July 12, 2026 09:54
The product/mixed/internal path mapping was hardcoded in release-notes-data.py
as _SHIP_PATH_PREFIXES / _BUILD_PATH_PREFIXES tuples. Move it into a committed,
data-driven config so the mapping is edited in one deterministic place instead
of in script logic.

- scripts/infra/docs/release-notes-paths.json: ordered tiers (each a tag +
  patterns) plus a default. Self-documenting via description/why/notes fields.
- release-notes-data.py: _pr_category now loads the tiers (cached) and returns
  the first tier with a matching file, else the default. A pattern matches by
  prefix (str.startswith, unchanged behaviour) or, if it contains a glob
  metacharacter (* ? [), by fnmatch.fnmatchcase — so paths, prefixes, and
  wildcards are all supported. A missing/malformed config is a hard error (no
  silent fallback to a guessed mapping).

Behaviour is identical to the previous hardcoded tuples (verified across the
prefix cases and real sync/docs/depot_tools PRs). Dev spec updated with the
config-file inventory entry and a pointer to the JSON as the edit surface.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mattleibow mattleibow changed the title Fix release-notes classifier tagging skia-sync (and native/docs) PRs as internal Fix release-notes classifier mis-tagging skia-sync PRs; make path tags data-driven Jul 12, 2026
@mattleibow
mattleibow merged commit c5e7355 into main Jul 12, 2026
86 checks passed
@mattleibow
mattleibow deleted the mattleibow-skia-sync-internal-label branch July 12, 2026 18:48
mattleibow added a commit that referenced this pull request Jul 12, 2026
PR #4410 added 'import fnmatch' to release-notes-data.py but the edit replaced
'import argparse' instead of adding alongside it, so main() crashes at startup:

    NameError: name 'argparse' is not defined

release-notes-data.py is only ever run as a script (via prepare.sh), so
py_compile and import-time checks pass — the NameError only fires when main()
builds its ArgumentParser. This broke the 'Sync - Release Notes & API Diffs'
workflow on main (run 29204563881: Prepare step exit code 1).

Re-add 'import argparse' in alphabetical order. Verified with
'python3 scripts/infra/docs/release-notes-data.py --help' (exit 0).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mattleibow mattleibow added this to the 4.151.0-preview.2 milestone Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant