diff --git a/.github/skills/update-skia/SKILL.md b/.github/skills/update-skia/SKILL.md index a947549f1ab..8c51ac288b3 100644 --- a/.github/skills/update-skia/SKILL.md +++ b/.github/skills/update-skia/SKILL.md @@ -147,7 +147,7 @@ The workflow follows this shape: git diff upstream/chrome/m{CURRENT}..upstream/chrome/m{TARGET} -- include/core/ include/gpu/ganesh/ ``` -👉 See [references/breaking-changes-checklist.md](references/breaking-changes-checklist.md) for detailed analysis template. +👉 See [references/breaking-changes-checklist.md](references/breaking-changes-checklist.md) for the full analysis template, including verification steps for struct sizes, moved files, and diff-reading traps. > 🛑 **GATE**: Present full breaking change analysis to user. Get approval before proceeding. @@ -175,38 +175,43 @@ 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. **Resolve conflicts** — each conflict must be resolved individually. + Never use `git merge -s ours` or `git read-tree --reset` — this destroys `git blame` attribution. -4. **Verify our C API files survived the merge**: + | File Category | Strategy | + |--------------|----------| + | `BUILD.gn` | **Combine both** — keep upstream structure AND SkiaSharp's platform flags + `skiasharp_build` target | + | `DEPS` | **Combine** — keep our dependency pins, accept upstream structure | + | `RELEASE_NOTES.md`, `infra/bots/` | **Take upstream** | + | C API (`include/c/`, `src/c/`) | **Keep SkiaSharp** — adapt includes/API calls in post-merge commits | + +4. **Commit the merge**: + ```bash + git commit # Creates proper two-parent merge + ``` + +5. **Verify our C API files survived the merge**: ```bash ls src/c/*.cpp include/c/*.h # All files should still exist ``` -5. **Source file verification** — Check for added/deleted upstream files: +6. **Source file verification** — Check for added/deleted upstream files: ```bash git diff upstream/chrome/m{CURRENT}..upstream/chrome/m{TARGET} --diff-filter=AD --name-only -- src/ include/ ``` Cross-reference against `BUILD.gn` — new source files may need to be added. -> 🛑 **GATE**: Merge complete, conflicts resolved. Run ALL of these verification commands -> (from inside `externals/skia`): +> 🛑 **GATE**: Merge complete, conflicts resolved. Verify: > ```bash -> cd externals/skia > ls src/c/*.cpp include/c/*.h # C API files intact -> 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 diff --check # Zero conflict markers +> git blame src/c/sk_canvas.cpp | head -20 # Attribution shows original commits, not just merge > ``` -> All three must produce expected output before proceeding. ### Phase 5: Fix C API Shim Layer @@ -224,9 +229,10 @@ must be updated when the underlying C++ APIs change. |-----------|-------------| | Missing type | Add/update typedef in `sk_types.h` | | Renamed function | Update call in `*.cpp` | - | Removed enum value | Remove from `sk_enums.cpp`, update `sk_types.h` | + | Removed enum value | Remove from `sk_enums.cpp` + `sk_types.h`. Flag as a C# breaking change — Phase 8 must add `[Obsolete]` or document removal | | Changed signature | Update C wrapper function signature | | New header required | Add `#include` in the relevant `.cpp` | + | Legacy flag breaks C API | Update C API to use replacement API (see gotcha #6). Do not just comment out the flag without a plan | 3. **Update `sk_types.h`** for any new enums or type changes: - **Reset `SK_C_INCREMENT` to `0`** in `externals/skia/include/c/sk_types.h` for the new milestone diff --git a/.github/skills/update-skia/references/breaking-changes-checklist.md b/.github/skills/update-skia/references/breaking-changes-checklist.md index 759fbf7e06a..38ed6f88b3f 100644 --- a/.github/skills/update-skia/references/breaking-changes-checklist.md +++ b/.github/skills/update-skia/references/breaking-changes-checklist.md @@ -2,8 +2,9 @@ ## How to Use This Document -When updating Skia to a new milestone, use this checklist to systematically identify -and categorize all breaking changes that affect SkiaSharp. +When updating Skia to a new milestone, follow these steps in order. Each step builds +on the previous one. The goal is to identify every change that affects our C API shim +layer **before** you start merging. ## Step 1: Gather Release Notes @@ -28,7 +29,18 @@ SkiaSharp uses **Ganesh** (not Graphite). Filter changes: | `SkPath`, `SkPaint` | ✅ Yes | Drawing APIs — always check | | `Dawn*`, `wgpu::` | ❌ No | Dawn/WebGPU — skip | | `SkSL`, `SkRuntimeEffect` | ⚠️ Maybe | Only if C API exposes runtime effects | -| `SkCodec` | ⚠️ Maybe | Only if C API exposes codec APIs | +| `SkCodec`, `SkEncoder` | ⚠️ Maybe | Only if C API exposes codec/encoder APIs | + +**Two common misclassification traps:** + +1. **Shared GPU headers** (`include/gpu/GpuTypes.h`, `include/gpu/*.h`): Types here are + shared across Ganesh and Graphite. Before skipping as "Graphite-only", check if + `include/gpu/ganesh/` consumes them — e.g., `GrFlushInfo` uses types from `GpuTypes.h`. + +2. **Struct field changes in asserted types**: `sk_structs.cpp` has `static_assert(sizeof(...))` + for every C API struct mapped via `reinterpret_cast`. A field addition to any of these + C++ structs (encoders, lattice, frame info, etc.) is a **build break** even if the + release notes don't mention it prominently. ## Step 3: Categorize Each Change @@ -42,62 +54,59 @@ Create a table for each change: | 3 | New `SkVertices::Builder` SK_API | 🟢 LOW | None | None | Optional: wrap later | ``` -## Step 4: C API Impact Analysis +## Step 4: Verify the Analysis -For each HIGH/MEDIUM risk change, check the C API: +The release notes don't cover everything. These checks catch what they miss. +**Check struct sizes:** ```bash -cd externals/skia - -# Search for affected symbols in C API -grep -rn "SYMBOL_NAME" src/c/ include/c/ - -# Show the C API file that wraps the affected C++ class -# Example: For SkImage changes, check sk_image.cpp -cat src/c/sk_image.cpp | grep -A5 "FUNCTION_NAME" +# List all asserted structs, then compare each against the target milestone +grep "static_assert.*sizeof" src/c/sk_structs.cpp +git show upstream/chrome/m{TARGET}:include/encode/SkPngEncoder.h | grep -A30 "struct Options" ``` -### Common C API Patterns - -**Enum removed from C++:** -```cpp -// Before (sk_enums.cpp) -case ENUM_VALUE_FOO: return CppEnum::kFoo; - -// After: Remove the line and the corresponding sk_types.h entry +**Check for moved files** (Skia relocates, it rarely deletes): +```bash +git diff upstream/chrome/m{CURRENT}..upstream/chrome/m{TARGET} --diff-filter=D --name-only +# For each deleted file our C API references: +git ls-tree -r upstream/chrome/m{TARGET} --name-only | grep -i "FILENAME_STEM" ``` -**Function signature changed:** -```cpp -// Before (sk_image.cpp) -return ToImage(SkImage::MakeOld(args).release()); - -// After: Update to new API -return ToImage(SkImages::MakeNew(args).release()); +**Confirm removals on the target branch** (diff `-` lines can be reorders, not deletions): +```bash +# Don't trust the diff — verify directly +git show upstream/chrome/m{TARGET}:include/gpu/ganesh/GrContextOptions.h | grep "fSuppressPrints" ``` -**Header moved:** -```cpp -// Before -#include "include/gpu/GrOldHeader.h" +## Step 5: C API Impact Analysis -// After: Update include path -#include "include/gpu/ganesh/GrNewHeader.h" +For each HIGH/MEDIUM risk change, check the C API: + +```bash +cd externals/skia +grep -rn "SYMBOL_NAME" src/c/ include/c/ ``` -## Step 5: C# Impact Analysis +### Recurring patterns across milestone bumps + +| Pattern | Risk | C API Fix | C# Fix | +|---------|------|-----------|--------| +| **Removed static factory** → replaced with context-taking free function | 🔴 HIGH | Update call + add `#include` for new header | Add new overload or update existing | +| **Header path reorganization** (e.g., `include/gpu/` → `include/gpu/ganesh/`) | 🟡 MED | Update `#include` paths | None | +| **Factory moved to namespace** (`ClassName::Make*` → `ClassNames::Make*`) | 🔴 HIGH | Update function call + add `#include` | None (auto-generated) | +| **Type renamed into namespace** (e.g., `GrVkAlloc` → `skgpu::VulkanAlloc`) | 🔴 HIGH | Update type refs in `sk_types_priv.h` | Update managed struct names | +| **Struct field added/removed** (breaks `static_assert`) | 🔴 HIGH | Update `sk_types.h` struct definition | Update managed struct | +| **Enum value inserted mid-sequence** (renumbers everything after) | 🟡 MED | Regenerate bindings — never hand-edit | Regenerate + update mappings | +| **File moved to new module** (e.g., `src/utils/` → `modules/`) | 🟡 MED | Update `#include` path | None | -For each C API change, check the C# side: +## Step 6: C# Impact Analysis ```bash -# Search C# wrappers for affected types grep -rn "ENUM_NAME\|FUNCTION_NAME" binding/SkiaSharp/ - -# Check generated bindings grep -rn "SYMBOL" binding/SkiaSharp/SkiaApi.generated.cs ``` -## Step 6: Build & Verify +## Step 7: Build & Verify After applying fixes: 1. Build native: `dotnet cake --target=externals-macos --arch=arm64` @@ -107,6 +116,23 @@ After applying fixes: ## Historical Examples +### m132 → m133 Changes Required + +| Change | C API Fix | C# Fix | +|--------|-----------|--------| +| `src/utils/SkJSON.h` moved to `modules/jsonreader/` | Updated `#include` in `sk_linker.cpp` | None | +| `SkPngEncoder::Options` gained `fGainmap` + `fGainmapInfo` | Added fields to `sk_pngencoder_options_t` | None (null pointers) | +| `SkColorSpace::MakeCICP` + CICP enums added | New C API function + enum types | New `CreateCicp` factory + enums | +| `GrContextOptions` fields reordered (no add/remove) | None (field-by-field copy) | None | +| `SkMaskFilter::approximateFilteredBounds` removed | None (not wrapped) | None | +| Shared GPU stats types added (`GpuStatsFlags`, `GpuStats`) | None (additive, safe defaults) | None (callback-based, deferred) | + +**Lessons learned:** +- `SkJSON.h` deletion was a **relocation** to `modules/jsonreader/` — always search for moves +- `SkPngEncoder::Options` field addition broke `static_assert` — always audit asserted structs +- `fSuppressPrints` appeared "removed" in diff but was reordered — verify on target branch +- `GpuTypes.h` changes looked Graphite-only but `GrFlushInfo` (Ganesh) uses them — trace consumers + ### m118 → m119 Changes Required | Change | C API Fix | C# Fix | @@ -116,8 +142,6 @@ After applying fixes: | `SkImages::MakeWithFilter` API change | Updated `sk_image.cpp` call | None (auto-generated) | | `GrDirectContext` API updates | Updated `gr_context.cpp` | Updated `GRDefinitions.cs` | -**Files changed:** 8 in C API, 12 total in SkiaSharp PR - ### m117 → m118 Changes Required | Change | C API Fix | C# Fix | @@ -127,8 +151,6 @@ After applying fixes: ## Risk Assessment Template -Before proceeding with a milestone update, score the risk: - | Factor | Low Risk | Medium Risk | High Risk | |--------|----------|-------------|-----------| | Milestones skipped | 1 | 2-3 | 4+ | diff --git a/.github/skills/update-skia/references/known-gotchas.md b/.github/skills/update-skia/references/known-gotchas.md index 5ac25b867af..32b933b658b 100644 --- a/.github/skills/update-skia/references/known-gotchas.md +++ b/.github/skills/update-skia/references/known-gotchas.md @@ -2,68 +2,118 @@ Hard-won findings from past Skia milestone updates. Check these proactively — they will save hours of debugging. -## 1. `DEF_STRUCT_MAP` vs Type Aliases +## C++ ↔ C API Layer + +### 1. `DEF_STRUCT_MAP` vs Type Aliases When upstream changes a C++ type from a `struct` to a `using` alias (e.g., `GrVkYcbcrConversionInfo` → `using VulkanYcbcrConversionInfo`), the `DEF_STRUCT_MAP` macro in `sk_types_priv.h` forward-declares `struct X`, which conflicts with the alias. Fix by switching to `DEF_MAP_WITH_NS(namespace, ActualType, CType)` and wrapping in the appropriate platform guard (e.g., `#if SK_VULKAN`). -## 2. `git-sync-deps` emsdk Failure +### 2. Struct Size `static_assert` Failures + +`sk_structs.cpp` asserts `sizeof(our_c_type) == sizeof(CppType)` for every struct mapped via `reinterpret_cast`. When upstream adds fields (e.g., `SkPngEncoder::Options` gaining gainmap pointers in m133), the assert fires. During analysis, proactively check every asserted struct against the target milestone — don't wait for the build to catch it. + +### 3. Custom Patches May Partially Survive Merges + +SkiaSharp adds custom methods to upstream headers (e.g., `SkTypeface::RefDefault()`). After merge, implementations in `.cpp` files may survive but header declarations can be silently removed by upstream changes. -Upstream m121+ added an `activate-emsdk` call in `tools/git-sync-deps`. Since SkiaSharp comments out emsdk in DEPS, this call fails. Set the environment variable `GIT_SYNC_DEPS_SKIP_EMSDK=1` in `scripts/cake/native-shared.cake` to prevent build failures during dependency sync. +When header declarations are lost: +1. **Check if upstream provides a replacement API** — if so, update the C API to use it +2. **If no replacement exists**, re-add the declaration. But consider moving custom methods out of upstream headers and into our C API layer (`src/c/`) to avoid this recurring conflict +3. **Never leave a mismatched header/implementation** — it compiles but crashes at runtime on some platforms -## 3. `BUILD.gn` Legacy Flags +### 4. Fontconfig `#ifdef` Guards -Upstream may introduce defines like `SK_DEFAULT_TYPEFACE_IS_EMPTY` and `SK_DISABLE_LEGACY_DEFAULT_TYPEFACE` that break SkiaSharp's C API, which still relies on legacy typeface/fontmgr APIs. Comment these defines out in `BUILD.gn` when they cause compilation errors in the C API shim layer. +Platform font manager calls (e.g., `SkFontMgr_New_FontConfig`) must be guarded by feature-availability macros (`SK_FONTMGR_FONTCONFIG_AVAILABLE`), not platform macros (`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. -## 4. Custom Patches May Partially Survive Merges +## Build System -SkiaSharp adds custom methods to upstream headers (e.g., `SkTypeface::RefDefault()`, `SkTypeface::UniqueID()`, `SkFontMgr::MakeDefault()`). After an upstream merge, implementations in `.cpp` files may survive but header declarations can be silently removed by upstream changes. Always verify that header declarations in `include/` still match the implementations in `src/`. +### 5. `git-sync-deps` emsdk Failure -## 5. Version Compatibility Errors Mean You Missed a Step +Upstream m121+ added an `activate-emsdk` call in `tools/git-sync-deps`. Since SkiaSharp comments out emsdk in DEPS, this call fails. Set `GIT_SYNC_DEPS_SKIP_EMSDK=1` in `scripts/cake/native-shared.cake`. -If you get `InvalidOperationException: The version of the native libSkiaSharp library (X) is incompatible`, this means the native milestone and C# expected milestone don't match. This is always caused by an incomplete Phase 6 (VERSIONS.txt not fully updated) or a stale build. Go back and fix the root cause — do NOT work around it with `--no-incremental` or by manually copying native libraries. +### 6. `BUILD.gn` Legacy Flags -## 6. Test Runner +Upstream progressively deprecates legacy APIs behind flags (e.g., `SK_DEFAULT_TYPEFACE_IS_EMPTY`, `SK_DISABLE_LEGACY_DEFAULT_TYPEFACE`). When these flags break SkiaSharp's C API: -Tests use runtime `Skip.If()` calls to self-skip on unsupported platforms. Run all tests -with `dotnet test tests/SkiaSharp.Tests.Console.sln` — this runs core, Vulkan, and Direct3D -test projects. Backend-specific tests self-skip when hardware isn't available. CI handles -WASM, Android, and iOS testing separately. +1. **Check what the flag removes** — read the upstream commit that added it +2. **If the C API uses the removed behavior**, update the C API to use the replacement API. This is the real fix — upstream is signaling that the old API will be removed entirely in a future milestone. +3. **Only as a short-term bridge** (with a TODO comment and tracking issue), you may comment out the flag to unblock the build while you work on the proper fix. Never leave a commented-out flag without a plan to address it. -## 7. HarfBuzz Binding Generation Failures +Also watch for renamed/removed GN flags between milestones — obsolete flags cause `Unknown GN flag` errors. Always diff the target `BUILD.gn` against the current one. -HarfBuzz generated bindings may fail due to system header issues (`inttypes.h` not found). This is independent of SkiaSharp bindings — if it happens, restore the file from git with `git checkout -- binding/HarfBuzzSharp/HarfBuzzApi.generated.cs` and continue. SkiaSharp bindings generate independently. +### 7. `.gitmodules` Branch Name -## 8. New C API Functions From Upstream +When the mono/skia target branch name changes, `.gitmodules` must be updated to track the new branch. Easy to forget; causes silent submodule tracking failures. -Upstream may add new C API functions (e.g., `sk_surface_draw_with_sampling`) that weren't in the previous milestone. The regeneration step (`pwsh ./utils/generate.ps1`) picks these up automatically. Always review the diff of `*.generated.cs` files for new functions that may need corresponding C# wrappers in `binding/SkiaSharp/`. +## Dependencies & Bindings -## 9. DEPS: Fork-Customized Dependencies +### 8. DEPS: Fork-Customized Dependencies -SkiaSharp's fork often has **newer** dependency versions than upstream Skia (from custom security/bug-fix updates via the `native-dependency-update` skill). When merging upstream and resolving DEPS, do NOT blindly update all hashes to the upstream milestone's versions — you may **downgrade** dependencies and break the build. +SkiaSharp's fork often has **newer** dependency versions than upstream. When resolving DEPS conflicts, do NOT blindly take upstream's hashes — you may downgrade and break the build. -**Check the skiasharp branch commit log** for custom dependency updates: ```bash git log --oneline skiasharp | grep -i "update\|bump\|libpng\|zlib\|expat\|brotli\|webp\|harfbuzz\|vulkan" ``` -For each dep with a custom update, **keep the fork's hash**. Only update deps that the fork hasn't customized. Common fork-customized deps: libwebp, brotli, expat, libpng, zlib, vulkanmemoryallocator, **harfbuzz**. +Keep fork's hash for customized deps. Common ones: libwebp, brotli, expat, libpng, zlib, vulkanmemoryallocator, **harfbuzz**. -> ⚠️ **HarfBuzz is ALWAYS a fork-customized dep.** HarfBuzz updates require hand-written C# delegate -> proxies and must be done as a separate task via the `native-dependency-update` skill. During a Skia -> milestone update, ALWAYS keep the fork's harfbuzz hash in DEPS and ALWAYS revert any changes to -> `binding/HarfBuzzSharp/HarfBuzzApi.generated.cs`. +### 9. HarfBuzz — ALWAYS Separate -**Symptoms of getting this wrong**: Build failures referencing missing source files (e.g., `palette.c` in libwebp), or HarfBuzz generated binding errors from a version mismatch. +HarfBuzz updates require hand-written C# delegate proxies and must be done via the `native-dependency-update` skill. During a milestone update, ALWAYS: +1. Keep the fork's harfbuzz hash in DEPS +2. Revert any generated HarfBuzz binding changes: `git checkout HEAD -- binding/HarfBuzzSharp/HarfBuzzApi.generated.cs` -## 10. HarfBuzz Generated Bindings — ALWAYS Revert +### 10. Enum Value Renumbering -HarfBuzz updates are **always separate** from Skia milestone updates. When the harfbuzz DEPS version changes (even accidentally during a merge), the code generator picks up new APIs (paint/draw/colorline callbacks) that require hand-written delegate proxy implementations in `binding/HarfBuzzSharp/DelegateProxies.*.cs`. This causes `CS8795` errors for missing partial method implementations. +When upstream inserts new enum values mid-sequence, ALL subsequent values shift. This affects `sk_enums.cpp`, `Definitions.cs`, `EnumMappings.cs`, and any test hardcoding enum integers. Always regenerate bindings — never hand-edit enum values. -**During a Skia milestone update, ALWAYS:** -1. Keep the fork's harfbuzz hash in DEPS (do not accept upstream's version) -2. Revert any generated HarfBuzz binding changes: `git checkout HEAD -- binding/HarfBuzzSharp/HarfBuzzApi.generated.cs` +## Diff Reading Traps + +### 11. Deleted Files ≠ Deleted Functionality + +Skia relocates files, it rarely removes them. Example: `src/utils/SkJSON.h` → `modules/jsonreader/SkJSONReader.h` in m133. Always search the target branch for where content moved before removing references: + +```bash +git ls-tree -r upstream/chrome/m{TARGET} --name-only | grep -i "FILENAME_STEM" +``` + +### 12. Reordered Fields ≠ Removed Fields + +A symbol on a diff `-` line may have been moved within the same file, not removed. Always confirm on the target branch: + +```bash +git show upstream/chrome/m{TARGET}:FILEPATH | grep "SYMBOL" +``` + +## Merge Strategy + +### 13. Genuine Merge Required + +Never use a tree-override merge (`git merge -s ours`, `git read-tree --reset`). This destroys `git blame` attribution for all C API files. Always resolve each conflict individually. Use `git merge --no-commit` for manual control. + +### 14. Conflict Resolution by File Category + +| File Category | Strategy | +|--------------|----------| +| `BUILD.gn` | **Combine both** — most complex; keep upstream structure AND SkiaSharp's platform flags/targets | +| `DEPS` | **Combine** — keep our dependency pins, accept upstream structure | +| `RELEASE_NOTES.md`, `infra/bots/` | **Take upstream** | +| C API headers (`include/c/`) | **Keep SkiaSharp** — these don't exist upstream | +| C API source (`src/c/`) | **Keep SkiaSharp + adapt** — fix includes and API calls in post-merge commits | + +## Testing + +### 15. Version Compatibility Errors + +`InvalidOperationException: The version of the native libSkiaSharp library (X) is incompatible` means VERSIONS.txt wasn't fully updated. Fix the root cause — do NOT work around it. + +### 16. Pixel Value Precision + +Upstream periodically improves color conversion precision, shifting expected pixel values by ±1. When pixel-exact test assertions break, check if upstream changed the conversion and update expected values. + +### 17. Test Runner -HarfBuzz version bumps should be done separately via the `native-dependency-update` skill, which includes writing the required delegate proxies. +Tests use `Skip.If()` for unsupported platforms. Run `dotnet test tests/SkiaSharp.Tests.Console.sln` for the full suite. Backend-specific tests self-skip when hardware isn't available. --- @@ -73,6 +123,11 @@ 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 | +| `static_assert` sizeof failure | Upstream struct gained/lost fields | Update C API struct in `sk_types.h` | +| `#include` file not found | Upstream moved file to new path | Search target branch, update path | +| `LNK2001 unresolved external` | C function name mismatch or missing lib | Verify names; check system library linkage | +| `Unknown GN flag` error | Obsolete build flag | Remove flag; diff target BUILD.gn | +| `git blame` all from merge commit | Tree-override merge was used | Redo as genuine conflict-resolved merge | +| Merge conflict in DEPS | Both forks updated deps | Keep our pins, accept upstream structure | +| Enum values don't match | Mid-sequence insertion | Regenerate bindings — never hand-edit | +| Pixel mismatch by ±1 | Upstream precision change | Update expected test values | diff --git a/.github/skills/update-skia/references/typical-changes.md b/.github/skills/update-skia/references/typical-changes.md index f5ac2cf0899..b5a05ca43f4 100644 --- a/.github/skills/update-skia/references/typical-changes.md +++ b/.github/skills/update-skia/references/typical-changes.md @@ -2,17 +2,22 @@ | Repository | File | Change | |-----------|------|--------| +| mono/skia | `BUILD.gn` | Merge conflict resolution (most complex) | | mono/skia | `DEPS` | Merge conflict resolution | | mono/skia | `include/core/SkMilestone.h` | New milestone number (from upstream) | -| mono/skia | `include/c/sk_types.h` | Enum/type updates | +| mono/skia | `include/c/sk_types.h` | Enum/type updates, `SK_C_INCREMENT` reset | | 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 | `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 | `tests/Tests/SkiaSharp/*.cs` | Test updates | diff --git a/.github/skills/update-skia/references/validation-prompt.md b/.github/skills/update-skia/references/validation-prompt.md index d4fb3992f51..bb9af68de8c 100644 --- a/.github/skills/update-skia/references/validation-prompt.md +++ b/.github/skills/update-skia/references/validation-prompt.md @@ -12,8 +12,20 @@ Please validate by (run from externals/skia): 1. Run: git diff upstream/chrome/m{CURRENT}..upstream/chrome/m{TARGET} --stat -- src/ include/ Count the files changed and compare to my analysis — did I miss any? 2. For each HIGH/MEDIUM item I identified, verify the C API impact by grepping src/c/ include/c/ -3. Check for changes I may have filtered as "Graphite-only" that actually affect Ganesh +3. Check for changes I may have filtered as "Graphite-only" that actually affect Ganesh: + - Search shared GPU headers (include/gpu/GpuTypes.h, include/gpu/*.h outside ganesh/ and graphite/) + - For each new type, grep include/gpu/ganesh/ to see if Ganesh consumes it 4. Check for removed/moved headers that our C API includes: grep -rh '#include' src/c/*.cpp | sort -u Then verify each included header still exists at upstream/chrome/m{TARGET} -5. Report: missed items, incorrect classifications, and confirmed items +5. **Struct size audit**: Check every `static_assert(sizeof(...))` in src/c/sk_structs.cpp. + For each asserted C++ struct, compare the target milestone's definition against our + C API struct in include/c/sk_types.h. Flag any struct that gained or lost fields. +6. **Deleted file audit**: For each file deleted between milestones + (git diff --diff-filter=D --name-only), check if our C API references it (#include + or uses its types). For referenced deletions, search the target branch for where the + content moved (git ls-tree -r upstream/chrome/m{TARGET} --name-only | grep STEM). +7. **Removal verification**: For any symbol claimed "removed" in the analysis, verify it + is truly absent from the target branch (not just moved within the file): + git show upstream/chrome/m{TARGET}:PATH | grep SYMBOL +8. Report: missed items, incorrect classifications, and confirmed items