Improve update-skia skill from m133 bump analysis#3658
Merged
Conversation
Add three mandatory verification checks to Phase 2 breaking change analysis: - Struct size audit: check all static_assert(sizeof) in sk_structs.cpp - Deleted file audit: search target branch for relocated files before removing refs - Removal verification: confirm symbols are truly gone, not just reordered in diffs Update validation prompt with new checks (5→8 steps): - Shared GPU header consumer tracing (Ganesh vs Graphite) - Struct size assertion audit - Deleted file relocation search - Symbol removal confirmation on target branch Add gotchas #11-13 from m132→m133 experience: - #11: Struct size static_assert failures (SkPngEncoder::Options) - #12: Deleted files are usually relocated (SkJSON.h → modules/jsonreader/) - #13: Reordered fields ≠ removed fields (GrContextOptions fSuppressPrints) Add m132→m133 historical example to breaking-changes-checklist.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Incorporate learnings from PR #3644 and our m133 analysis, focusing on quality over quantity (-50 lines net). Key changes: - breaking-changes-checklist.md: Merge Steps 4a/4b/4c into a single 'Verify the Analysis' section (was duplicating gotchas 11-13). Add 'Recurring patterns' table consolidating Common C API Patterns and PR #3644's 'Recurring Breaking Change Categories' into one reference - known-gotchas.md: Reorganize into themed sections (C API layer, build system, dependencies, diff reading, merge strategy, testing). Merge HarfBuzz gotchas 7+10 into one. Add merge strategy (from PR #3644), fontconfig guards, enum renumbering, pixel precision, GN flag churn - SKILL.md Phase 2: Replace verbose 'three mandatory checks' with single reference to checklist. Phase 4: Add --no-commit merge, per-file conflict resolution table, git blame verification gate (from PR #3644) - typical-changes.md: Add BUILD.gn, .gitmodules, sk_types_priv.h, native/*/build.cake, GRDefinitions.cs (from PR #3644) - Remove 'MANDATORY' headers that diluted the signal — everything in the skill is mandatory by default Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Gotcha #6 (BUILD.gn legacy flags): Replace 'comment these out' with a proper investigation flow: check what the flag removes, update the C API to use the replacement, only comment out as a tracked short-term bridge with a TODO - Gotcha #3 (custom patches): Add concrete steps for when declarations diverge — check for upstream replacement, consider moving custom methods into C API layer, never leave mismatched header/impl - Phase 5 error table: Flag removed enum values as C# breaking changes requiring [Obsolete] or documentation in Phase 8. Add legacy flag row pointing to gotcha #6 - Fix duplicate step 4/5 in Phase 2 (copy-paste from earlier edit) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📦 Try the packages from this PRWarning 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 -- 3658PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 3658"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-3658/packages --name skiasharp-pr-3658More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-3658 |
Contributor
|
📖 Documentation Preview The documentation for this PR has been deployed and is available at: 🔗 View Staging Site This preview will be updated automatically when you push new commits to this PR. This comment is automatically updated by the documentation staging workflow. |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improves the
update-skiaskill based on lessons learned during the m132→m133 breaking change analysis, incorporating useful findings from PR #3644 (community contribution).Net effect: better quality, less duplication (-50 lines in the consolidation commit).
Changes
Commit 1: Lessons from m133 analysis
Commit 2: Consolidate and reduce duplication
git blamegate from PR update skia skill improvements #3644Commit 3: Fix guidance that hides issues
[Obsolete]Motivation
During the m133 analysis, three errors were made that the skill should have prevented:
modules/jsonreader/)SkPngEncoder::Optionsstruct size change (caught by validation agent, not analysis)fSuppressPrintsas removed when it was reordered withinGrContextOptions.hAdditionally, the skill had guidance that masked problems rather than solving them (commenting out legacy flags, verifying without action steps, silently removing enum values).