Skip to content

Improve update-skia skill from m133 bump analysis #3658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattleibow merged 3 commits into from
Apr 10, 2026
Merged

Improve update-skia skill from m133 bump analysis #3658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a96fe1
Improve update-skia skill with lessons from m132→m133 analysis
mattleibow Apr 10, 2026
abe277f
Consolidate update-skia skill: less duplication, better structure
mattleibow Apr 10, 2026
9ab7799
Fix skill guidance that hides issues instead of addressing them
mattleibow Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 24 additions & 18 deletions .github/skills/update-skia/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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

Expand All @@ -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
Expand Down
110 changes: 66 additions & 44 deletions .github/skills/update-skia/references/breaking-changes-checklist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand All @@ -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`
Expand All @@ -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 |
Expand All @@ -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 |
Expand All @@ -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+ |
Expand Down
Loading
Loading