diff --git a/.github/skills/update-skia/SKILL.md b/.github/skills/update-skia/SKILL.md index a947549f1ab..2f6aaf9ecd5 100644 --- a/.github/skills/update-skia/SKILL.md +++ b/.github/skills/update-skia/SKILL.md @@ -26,7 +26,8 @@ Update Google Skia to a new Chrome milestone in SkiaSharp's mono/skia fork. ## Key References - **[references/breaking-changes-checklist.md](references/breaking-changes-checklist.md)** — How to analyze breaking changes between milestones -- **[references/known-gotchas.md](references/known-gotchas.md)** — 10 hard-won lessons and troubleshooting table +- **[references/upstream-merge-guide.md](references/upstream-merge-guide.md)** — Detailed merge strategy and conflict resolution for mono/skia +- **[references/known-gotchas.md](references/known-gotchas.md)** — Hard-won lessons and troubleshooting table - **[references/typical-changes.md](references/typical-changes.md)** — Files typically changed during an update - **[documentation/dev/dependencies.md](../../../documentation/dev/dependencies.md)** — Dependency tracking and cgmanifest.json format - **[RELEASE_NOTES.md in upstream Skia](https://raw.githubusercontent.com/google/skia/main/RELEASE_NOTES.md)** — Official Skia release notes @@ -175,16 +176,34 @@ milestone numbers and paste your breaking change analysis table. The default exp git checkout -b dev/update-skia-{TARGET} ``` -2. **Merge upstream**: +2. **Merge upstream** — Use `--no-commit` for manual conflict resolution: ```bash - git merge upstream/chrome/m{TARGET} + git merge --no-commit upstream/chrome/m{TARGET} ``` -3. **Resolve conflicts** — Common conflicts: - - `DEPS` — Keep our dependency pins, accept upstream structure changes - - `RELEASE_NOTES.md` — Accept upstream - - `infra/bots/jobs.json` — Accept upstream - - Source files in `src/` — Carefully resolve (don't lose our C API) +3. **⚠️ CRITICAL: Genuine conflict-resolved merge required** + + Each conflict MUST be resolved individually. See [references/upstream-merge-guide.md](references/upstream-merge-guide.md) for the full guide. + + **❌ FORBIDDEN: Tree-Override Merge** — Never use `git merge -s ours`, `git read-tree --reset`, or blanket resolution that takes one side for all files. This destroys `git blame` attribution for all C API files. + + **Conflict resolution by file category:** + + | File Category | Strategy | Details | + |--------------|----------|---------| + | `BUILD.gn` | **Combine both** | Keep new milestone's structure AND SkiaSharp's platform flags and `skiasharp_build` target. Usually the most complex file. | + | `DEPS` | **Combine both** | Keep our dependency pins, accept upstream structure changes | + | `RELEASE_NOTES.md` | **Take upstream** | Accept upstream | + | `infra/bots/jobs.json` | **Take upstream** | Accept upstream | + | C API headers (`include/c/`) | **Keep SkiaSharp** | These don't exist upstream, conflicts are rare | + | C API source (`src/c/`) | **Keep SkiaSharp + adapt** | Take implementations, update includes and API calls in post-merge commits | + | GL/Vulkan interfaces | **Combine** | Adapt SkiaSharp's dynamic GL loading to new API structure | + | Infrastructure (`.gitignore`, CI) | **Take upstream** for CI/jobs, **keep SkiaSharp** for `.disabled.*` configs | + +4. **Commit the merge**: + ```bash + git commit # Creates proper two-parent merge + ``` 4. **Verify our C API files survived the merge**: ```bash @@ -205,8 +224,9 @@ milestone numbers and paste your breaking change analysis table. The default exp > git diff upstream/chrome/m{CURRENT}..upstream/chrome/m{TARGET} --diff-filter=AD --name-only -- src/ include/ > # Review added/deleted files > git diff --check # Zero conflict markers remain +> git blame src/c/sk_canvas.cpp | head -20 # Attribution shows original commits, not just merge > ``` -> All three must produce expected output before proceeding. +> All four must produce expected output before proceeding. ### Phase 5: Fix C API Shim Layer @@ -317,6 +337,12 @@ New functions from upstream changes are usually additive and can be deferred. - Use `[Obsolete]` for deprecated APIs with migration guidance - Return `null` from factory methods on failure (don't throw) +**API evolution policy** — when deciding whether to break SkiaSharp's managed API: +1. Prefer minimal changes — absorb internally when existing API is still sensible +2. Allow logical changes — when keeping old API would be misleading or semantically wrong +3. Follow Skia's direction — if Skia renamed/restructured for good reason, follow suit +4. Document all breaking changes — every managed API change needs upstream Skia motivation. Do NOT add backward-compatibility shims for APIs that Skia intentionally removed. + ### Phase 9: Build & Test ```bash @@ -387,9 +413,12 @@ Before proceeding to merge, verify ALL of these: - [ ] mono/skia PR targets `skiasharp` branch - [ ] mono/SkiaSharp PR targets `main` branch - [ ] **SkiaSharp's `externals/skia` submodule points to the mono/skia PR branch** (`git submodule status`) +- [ ] `.gitmodules` branch updated to match new mono/skia branch name - [ ] `cgmanifest.json` updated with new commit hash, version, and chrome_milestone - [ ] `scripts/VERSIONS.txt` updated (ALL version lines, not just milestone) +- [ ] `scripts/azure-pipelines-variables.yml` updated - [ ] `SkiaApi.generated.cs` regenerated and committed +- [ ] `changelogs/SkiaSharp/3.{TARGET}.0/SkiaSharp.md` created with breaking changes documented - [ ] Both PRs cross-reference each other - [ ] Native build passes on at least one platform - [ ] C# build passes with 0 errors @@ -424,6 +453,15 @@ Before proceeding past each step, verify: These files contain lookup information — consult them when you hit a problem or need context, not necessarily upfront: -- **[references/known-gotchas.md](references/known-gotchas.md)** — 10 hard-won lessons from past updates (DEF_STRUCT_MAP, emsdk, BUILD.gn flags, HarfBuzz, DEPS forks, etc.) and a troubleshooting table +- **[references/known-gotchas.md](references/known-gotchas.md)** — Hard-won lessons from past updates and troubleshooting table +- **[references/upstream-merge-guide.md](references/upstream-merge-guide.md)** — Detailed merge strategy, conflict resolution by file category, and verification steps - **[references/typical-changes.md](references/typical-changes.md)** — Files typically changed in each repository during an update - **[references/breaking-changes-checklist.md](references/breaking-changes-checklist.md)** — How to analyze breaking changes between milestones + +## PR Review Strategy for Large Diffs + +mono/skia PRs can have thousands of upstream commits. Guide reviewers with: + +1. **Compare PR branch against `chrome/m{TARGET}`** for non-C-API content — changes should be minimal +2. **Compare PR branch against `skiasharp`** for C API modifications (`include/c/`, `src/c/`) — this is where the real review happens +3. **Highlight the focused post-merge commits** — these contain all the actual SkiaSharp-specific adaptation work diff --git a/.github/skills/update-skia/references/breaking-changes-checklist.md b/.github/skills/update-skia/references/breaking-changes-checklist.md index 759fbf7e06a..0e6cac118b1 100644 --- a/.github/skills/update-skia/references/breaking-changes-checklist.md +++ b/.github/skills/update-skia/references/breaking-changes-checklist.md @@ -105,6 +105,48 @@ After applying fixes: 3. Build C#: `dotnet build binding/SkiaSharp/SkiaSharp.csproj` 4. Test: `dotnet test tests/SkiaSharp.Tests.Console/SkiaSharp.Tests.Console.csproj` +## Recurring Breaking Change Categories + +These categories recur across milestone bumps. Use them as a checklist when analyzing a new bump: + +### Category: Removed Static Factory Methods (HIGH Risk) + +Skia periodically removes static factory methods from classes like `SkTypeface`, `SkFontMgr`, etc. and replaces them with instance methods or free functions that require an explicit context object (e.g., a font manager). + +**C API fix pattern:** Add optional context parameter to C API wrapper (NULL = use platform default internally). +**C# fix pattern:** Add new overloads with context parameter. Follow Skia's explicit-context direction rather than hiding the change. + +### Category: Header Path Reorganization (MEDIUM Risk) + +Skia reorganizes headers into subdirectories (e.g., GPU headers into backend-specific paths under `ganesh/`, `graphite/`, etc.). + +**C API fix pattern:** Update `#include` paths in C API source files. No API surface change. + +### Category: Factory Methods Moved to Namespace Functions (HIGH Risk) + +Static methods on classes get moved to free functions in a namespace (e.g., `ClassName::Make*()` → `ClassNames::Make*()`, with new dedicated headers). + +**C API fix pattern:** Update function calls and add new `#include` directives. + +### Category: Type Renames + Struct Changes (HIGH Risk) + +Types get renamed (often into namespaces like `skgpu::`) and structs gain/lose fields. + +**C API fix pattern:** Update type references, update struct definitions (remove dead fields, add new ones). +**C# fix pattern:** Update managed struct definitions to match. Breaking but necessary — dead fields are misleading. + +### Category: Removed Files / APIs (LOW-MEDIUM Risk) + +Entire headers or individual APIs removed from upstream. + +**C API fix pattern:** Remove includes and any wrappers for removed APIs. + +### Category: New Types / Enum Values (LOW Risk, Additive) + +New color types, enum values, struct fields added. + +**C API fix pattern:** Add to enum mappings. Note: mid-enum insertions renumber all subsequent values — always regenerate bindings, never hand-edit. + ## Historical Examples ### m118 → m119 Changes Required diff --git a/.github/skills/update-skia/references/known-gotchas.md b/.github/skills/update-skia/references/known-gotchas.md index 5ac25b867af..5fed5d5d6b0 100644 --- a/.github/skills/update-skia/references/known-gotchas.md +++ b/.github/skills/update-skia/references/known-gotchas.md @@ -65,6 +65,55 @@ HarfBuzz updates are **always separate** from Skia milestone updates. When the h HarfBuzz version bumps should be done separately via the `native-dependency-update` skill, which includes writing the required delegate proxies. +## 11. Git History: Genuine Merge vs Tree-Override (CRITICAL) + +**Never** use a tree-override merge (e.g., `git merge -s ours`, or `git read-tree --reset` after `--no-commit`) when merging upstream into the mono/skia fork. This creates a merge commit that records parentage without actually combining content, which: +- Destroys `git blame` attribution — all lines attributed to the single merge commit +- Makes `git log --follow` useless for C API files +- Forces subsequent cherry-picks that further obscure history + +**Always** use a genuine conflict-resolved merge where each conflicting file is resolved individually. See [upstream-merge-guide.md](upstream-merge-guide.md). + +## 12. Prefer Native Memory Patterns + +When bridging managed and native code (e.g., stream duplication, data snapshots), prefer handing data to native Skia types (like `SKData`, `SkMemoryStream`) over managed workarounds with weak references or shared state. Native ref-counted types integrate with Skia's internal ownership model and expose fast paths (e.g., `getMemoryBase()` zero-copy) that managed wrappers cannot. + +## 13. Isolate Behavioral Changes Into Separate PRs + +If a version bump requires a significant behavioral refactor (e.g., changing stream duplication semantics), extract it into its own PR. This keeps the main bump PR focused on the mechanical version update and lets the behavioral change be reviewed, tested, and validated in isolation. It also makes bisection easier if issues arise later. + +## 14. GPU Test Coverage Is Incomplete + +GPU tests may not run in CI for all backends and platforms. WebAssembly in particular has no runtime tests beyond crash detection. When making changes that affect GPU paths, call out which platforms were actually tested and provide evidence. Don't assume CI covers it. + +## 15. GN Build Flag Churn Between Milestones + +Build flags get renamed, removed, and added frequently between milestones. Obsolete flags cause build errors. Always diff the target milestone's `BUILD.gn` against the current one to identify flag changes before building. Pay special attention to `skia_enable_fontmgr_*` flags which are particularly volatile. + +## 16. `.gitmodules` Branch Name Must Be Updated + +When the mono/skia target branch name changes for a new milestone, the `.gitmodules` file in SkiaSharp must be updated to track the new branch. This is easy to forget and causes submodule operations to fail silently or track the wrong branch. + +## 17. Fontconfig `#ifdef` Guards + +Platform font manager calls (e.g., `SkFontMgr_New_FontConfig`) must be guarded by feature-availability macros (e.g., `SK_FONTMGR_FONTCONFIG_AVAILABLE`), not platform macros (e.g., `SK_BUILD_FOR_UNIX`). When `skia_use_fontconfig=false`, platform macros leave the symbol unresolved. The `-Wl,--no-undefined` linker flag catches this at link time. + +## 18. Enum Value Renumbering + +When upstream inserts new enum values mid-sequence (e.g., new `SKColorType` entries), ALL subsequent values shift. This affects `sk_enums.cpp`, `Definitions.cs`, `EnumMappings.cs`, utility switch tables, and any test that hardcodes enum integer values. Always regenerate bindings — don't hand-edit enum values. + +## 19. Changelog Convention + +SkiaSharp uses structured changelogs at `changelogs/SkiaSharp/{version}/SkiaSharp.md`. Each version bump must add a changelog file documenting: assembly version change, new types/enums, behavioral changes, breaking changes, and known quirks. + +## 20. Reference Count Assertions May Need Relaxing + +Skia may change internal reference counting behavior between milestones. When tests fail on refcount assertions, consider using range checks (`Assert.InRange(refcount, expected, expected + 1)`) instead of exact equality. A refcount change is more likely a deliberate upstream shift than a bug in your port. + +## 21. Pixel Value Precision Changes + +Upstream Skia periodically improves color conversion precision (e.g., switching from integer to floating-point luma). This can shift expected pixel values by ±1. When pixel-exact test assertions break, check whether upstream changed the conversion — update expected values rather than treating it as a regression. + --- ## Troubleshooting @@ -74,5 +123,13 @@ HarfBuzz version bumps should be done separately via the `native-dependency-upda | `EntryPointNotFoundException` | Native lib not rebuilt after C API change | `dotnet cake --target=externals-{platform}` | | `error CS0246` missing type | Binding not regenerated | `pwsh ./utils/generate.ps1` | | Merge conflict in DEPS | Both forks updated deps independently | Keep our DEPS pins, accept upstream structure | -| `LNK2001 unresolved external` | C function name mismatch | Verify C API function names match exactly | -| Build fails after merge | Missing `#include` for moved headers | Check upstream header relocation notes | +| `LNK2001 unresolved external` | C function name mismatch or missing system lib | Verify C API function names match; check system library linkage (fontconfig, etc.) | +| Build fails after merge | Missing `#include` for moved headers | Check upstream header relocation patterns | +| `git blame` shows all lines from merge commit | Tree-override merge was used | Redo merge as genuine conflict-resolved merge (see #11) | +| `Unknown GN flag` error | Obsolete flag in build config | Remove flag; diff target BUILD.gn for current flags (see #15) | +| Disk space CI failure | Skia DEPS pulls too many dependencies | Remove unused DEPS entries | +| Submodule fetch fails / tracks wrong branch | `.gitmodules` still references old branch name | Update `.gitmodules` branch field (see #16) | +| Undefined symbol `SkFontMgr_New_FontConfig` | Wrong `#ifdef` guard | Fix guard to check `SK_FONTMGR_FONTCONFIG_AVAILABLE` (see #17) | +| Color type enum values don't match | New values inserted mid-enum | Regenerate bindings; never hand-edit (see #18) | +| Refcount test assertions fail | Skia changed internal ownership | Use `Assert.InRange` instead of `Assert.Equal` (see #20) | +| Pixel value mismatch by ±1 | Upstream precision improvement | Update expected test values (see #21) | diff --git a/.github/skills/update-skia/references/typical-changes.md b/.github/skills/update-skia/references/typical-changes.md index f5ac2cf0899..5c34c9866f7 100644 --- a/.github/skills/update-skia/references/typical-changes.md +++ b/.github/skills/update-skia/references/typical-changes.md @@ -3,16 +3,23 @@ | Repository | File | Change | |-----------|------|--------| | mono/skia | `DEPS` | Merge conflict resolution | +| mono/skia | `BUILD.gn` | Merge conflict resolution (most complex) | | mono/skia | `include/core/SkMilestone.h` | New milestone number (from upstream) | | mono/skia | `include/c/sk_types.h` | Enum/type updates | | mono/skia | `src/c/*.cpp` | C API fixes for new C++ APIs | | mono/skia | `src/c/sk_enums.cpp` | Enum mapping updates | +| mono/skia | `src/c/sk_types_priv.h` | Include path + type conversion updates | +| mono/SkiaSharp | `.gitmodules` | Submodule branch name | | mono/SkiaSharp | `externals/skia` | Submodule pointer | | mono/SkiaSharp | `scripts/VERSIONS.txt` | All version numbers | | mono/SkiaSharp | `cgmanifest.json` | Security tracking | | mono/SkiaSharp | `scripts/azure-pipelines-variables.yml` | CI config | +| mono/SkiaSharp | `scripts/cake/native-shared.cake` | Build script adjustments (e.g., emsdk skip) | +| mono/SkiaSharp | `native/*/build.cake` | Per-platform GN flag updates | | mono/SkiaSharp | `binding/SkiaSharp/SkiaApi.generated.cs` | Regenerated | -| mono/SkiaSharp | `binding/SkiaSharp/Definitions.cs` | Type definitions | +| mono/SkiaSharp | `binding/SkiaSharp/Definitions.cs` | Type definitions, new enums | | mono/SkiaSharp | `binding/SkiaSharp/EnumMappings.cs` | Enum mappings | +| mono/SkiaSharp | `binding/SkiaSharp/GRDefinitions.cs` | GPU type changes | | mono/SkiaSharp | `binding/libSkiaSharp.json` | Type config | +| mono/SkiaSharp | `changelogs/SkiaSharp/3.{TARGET}.0/SkiaSharp.md` | Changelog (new file) | | mono/SkiaSharp | `tests/Tests/SkiaSharp/*.cs` | Test updates | diff --git a/.github/skills/update-skia/references/upstream-merge-guide.md b/.github/skills/update-skia/references/upstream-merge-guide.md new file mode 100644 index 00000000000..29adb8c6aef --- /dev/null +++ b/.github/skills/update-skia/references/upstream-merge-guide.md @@ -0,0 +1,137 @@ +# Upstream Merge Guide for mono/skia + +How to properly merge a new Google Skia milestone into the mono/skia fork while preserving SkiaSharp's C API layer and full commit attribution. + +## Background + +The mono/skia fork adds a C API wrapper layer on top of upstream Google Skia: +- `include/c/*.h` (~37 headers) +- `src/c/*.cpp` (~30 implementations) +- `src/xamarin/*.cpp` (~10 utility files) +- `BUILD.gn` additions (`skiasharp_build("SkiaSharp")` target) + +These ~97 files do NOT exist in upstream Google Skia. When merging a new milestone, these must be carried forward with proper git history and blame attribution. + +## The Golden Rule: Genuine Conflict-Resolved Merge + +```bash +git merge --no-commit upstream/chrome/m{TARGET} +# Resolve each conflict individually (see per-file guide below) +git commit +``` + +### Why Not Tree-Override? + +A tree-override merge (`git merge -s ours`, or `git read-tree --reset` after `--no-commit`) records parentage without actually combining content. This causes: + +1. **`git blame` is destroyed** — every line in every file is attributed to the merge commit, not original authors +2. **`git log --follow` breaks** — can't trace file history through the merge +3. **Cherry-picks needed afterward** — C API files must be re-added as separate commits, further obscuring history +4. **Misleading history** — merge claims to include both parents but actually discarded one side entirely + +## Merge Scale (What to Expect) + +The number of conflicts scales with how many milestones are being jumped. Expect conflicts in `BUILD.gn`, `DEPS`, infrastructure files, and any upstream headers that SkiaSharp patches. C API files (`include/c/`, `src/c/`) rarely conflict since they don't exist upstream — but verify they all survive the merge. + +## Conflict Resolution by File Category + +### BUILD.gn — **Combine Both** (Most Complex) + +This is always the hardest file. Both sides modify it heavily. + +**Keep from upstream:** +- Modern build structure, updated source file lists, new modules +- Removed obsolete flags and targets + +**Keep from SkiaSharp:** +- `skiasharp_build("SkiaSharp")` target (usually near line ~3500+) +- Platform flags: `is_winrt`, `is_watchos`, `is_tvos`, `is_maccatalyst` +- `SKIA_C_DLL` define in `extra_cflags` +- All `src/c/*.cpp` and `src/xamarin/*.cpp` source listings +- All `include/c/*.h` header listings + +**Watch out for:** +- Legacy defines like `SK_DEFAULT_TYPEFACE_IS_EMPTY` and `SK_DISABLE_LEGACY_DEFAULT_TYPEFACE` — these may break SkiaSharp's C API. Comment them out if they cause compilation errors. + +### DEPS — **Keep our DEPS pins, accept upstream structure** + +SkiaSharp customizes Skia's dependency pins. Accept upstream structure, but keep SkiaSharp's DEPS pins. If SkiaSharp comments out specific entries (like emsdk), preserve those comments. + +### C API Headers (`include/c/`) — **Keep SkiaSharp** + +These files don't exist upstream. Conflicts are rare. If they occur, always keep SkiaSharp's version — the files are entirely SkiaSharp additions. + +### C API Source (`src/c/`) — **Keep SkiaSharp + Adapt Later** + +Take SkiaSharp's implementations during merge resolution. Then apply focused post-merge commits to adapt them: +- Update `#include` paths for header reorganizations +- Update C++ API calls to use current function names/namespaces +- Update struct references for type renames + +### Infrastructure Files + +| File Type | Strategy | +|-----------|----------| +| `.gitignore` | Combine both | +| CI job definitions (`infra/bots/`) | Take upstream | +| `.disabled.*` configs | Keep SkiaSharp | +| `RELEASE_NOTES.md` | Take upstream | +| Fontconfig/Tizen support | Combine | + +## Post-Merge Commits + +After the merge commit, apply targeted adaptation commits. Keep these **separate** from the merge for clear blame: + +### Commit 1: C API Adaptations + +Update C API files that need changes for the new milestone: +- Include path updates, API call updates, struct changes + +### Commit 2: Build Adjustments (if needed) + +Any remaining BUILD.gn tweaks or build configuration changes. + +## Verification Checklist + +After merge and post-merge commits: + +```bash +# 1. Verify both parents visible in history +git log --oneline --graph HEAD~5..HEAD + +# 2. Verify skiasharp commits reachable +git log --all --oneline skiasharp..HEAD | head -5 + +# 3. Verify blame attribution preserved +git blame src/c/sk_canvas.cpp | head -20 +# Should show original commit SHAs, not just the merge commit + +# 4. Count C API files (expect ~97) +find include/c src/c src/xamarin \( -name "*.h" -o -name "*.cpp" \) | wc -l + +# 5. Verify no auto-merge errors in key files +git diff HEAD -- BUILD.gn | head -50 # Sanity check + +# 6. Verify SkiaSharp build target present +grep -n "skiasharp_build" BUILD.gn +``` + +## Common Pitfalls + +1. **Auto-merged files may still be wrong**: Git's auto-merge can produce incorrect results when both sides modified the same logical section in non-overlapping lines. Review auto-merged files for correctness. + +2. **New SkiaSharp-only files**: ~103 files that only exist in SkiaSharp are auto-added during merge. Verify they're all present — Git may silently drop some during complex merges. + +3. **Deleted upstream files**: If upstream deleted files that SkiaSharp references, the merge may silently succeed but the build will fail. Check for missing includes. + +4. **Header/implementation mismatch**: After merge, verify that header declarations in `include/c/` still match implementations in `src/c/`. Custom method declarations can be silently removed by upstream changes to shared headers. + +## Typical Conflict Patterns + +These are the most common conflict types across milestone bumps: + +| File | Conflict Type | Resolution Pattern | +|------|--------------|-------------------| +| `BUILD.gn` | Both sides modified heavily | Combine: upstream structure + SkiaSharp targets/flags | +| `infra/bots/jobs.json` | Both sides modified | Take upstream | +| Platform support configs | SkiaSharp-specific platform additions | Combine: upstream base + SkiaSharp's additions |