Support release-branch targeting in Skia upstream sync (+ Opus 4.7, libc++ build fix)#4239
Merged
Merged
Conversation
Let the Skia sync workflow and update-skia skill target a release/<major>.<milestone>.x line (in both mono/SkiaSharp and mono/skia) instead of only main, derived automatically from the resolved milestone. - auto-skia-sync.md: pre-activation detects the release line from the target milestone, exposes is_release/base_branch/skia_base_branch/ head_branch, fails fast if the mono/skia release branch is missing, and adapts the skip-check; host submodule-align step and prompt body use the resolved base/head branches. - skia-sync-push-prs.sh: consume HEAD_BRANCH/BASE_BRANCH/SKIA_BASE_BRANCH/ IS_RELEASE and target the right base branches with release-aware titles. - update-skia skill: document release-target mode (Phase 1 base detection, parametric Phase 4 + Phase 11, fail-if-missing mono/skia branch). Creation of the matching mono/skia release branch is owned by the release process and will be handled in a separate change to the release-branch skill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the milestone/branch-line resolution out of the inline workflow YAML and into two committed, testable scripts: - skia-sync-detect.sh: the single source of truth for mode/target/base-branch resolution. Emits key=value to $SKIA_SYNC_OUT (default $GITHUB_OUTPUT); --gate adds the upstream-exists / already-merged checks. Run by both the pre_activation step (--gate, writes job outputs) and the agent align step (sourced, since that job can't read pre_activation outputs). - skia-sync-align-submodule.sh: the submodule alignment, driven by the sourced base_branch / skia_base_branch. The pre_activation job now sparse-checks-out .github/scripts so it can call the script. This collapses the two large inline bash blocks to ~1 and ~5 lines and removes the duplicated branch-resolution logic. Behaviour is preserved for the main line and the single-match release path. The extraction also folds in two release-resolution fixes from review that were not yet in the previous commit: the strictly-older check is now a numeric compare (`-lt` instead of `!=`), and a milestone matching multiple release branches now hard-fails instead of silently picking the first via `head -1`. 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 -- 4239PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4239"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4239/packages --name skiasharp-pr-4239More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4239 |
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. |
Add an engine: block pinning model claude-opus-4.6 (overriding the default claude-sonnet-4.6). Upstream merge/conflict resolution and native build-fix reasoning are hard, so the stronger model is worth the higher AI-credit cost for this workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Opus 4.7 carries the same 5x AI-credit multiplier as 4.6, so move to the newer model for the upstream merge/conflict-resolution reasoning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
native/linux/build.cake builds libSkiaSharp/libHarfBuzzSharp with -stdlib=libc++. On the real CI that runtime comes from the .NET cross-compilation image, but this workflow builds on a bare Ubuntu host where the agent cannot apt-install anything (no apt/sudo in the AWF chroot, and the firewall blocks the OS package mirrors). Install libc++-dev + libc++abi-dev in the host pre-agent step so the unmodified native build compiles, and tell the agent not to patch native build files or compiler flags to work around a missing dependency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mattleibow
changed the title
The cron→mode mapping (7 AM → current, 5 PM → latest, else → next) was the only workflow-coupled knowledge inside skia-sync-detect.sh — it hardcoded the literal cron strings and read mode/milestone/schedule from INPUT_MODE, INPUT_MILESTONE and SCHEDULE env vars. Move that mapping into the workflow, next to where the crons are declared, as a single GitHub Actions expression. The detector now takes real --mode/--milestone arguments and no longer knows anything about cron, so it is self-contained and unit-testable (pass --mode latest instead of faking SCHEDULE='0 17 * * *'). mode is staged into an env var (not interpolated straight into run:) so the free-form milestone dispatch input cannot inject shell — the same anti-injection safety the previous env-based wiring had. Behavior is unchanged: validated that --mode current/next/latest, the default, an explicit --milestone override, an empty --milestone, and an unknown arg all resolve exactly as before. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Final review pass over the skia-sync scripts: - Harden skia-sync-detect.sh arg parsing: a value-less --mode/--milestone now reports a clear error and exits 2 instead of tripping set -e on a double shift. Matters now that the detector is meant to be run by hand for testing. - Flatten the multi-match release-branch list with paste instead of the $(echo $VAR) word-splitting idiom (SC2116/SC2086). - Mark the runtime-generated skia-sync-env.sh source with a shellcheck source=/dev/null directive (SC1091). All three scripts are now shellcheck-clean at style level; behavior verified unchanged for every valid invocation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mattleibow
added a commit
that referenced
this pull request
Jun 29, 2026
…ne skill, honest run signal (#4258) Harden Skia upstream-sync: conflict policy, durable gn args, standalone skill, honest run signal (#4258) Follow-up to the mode=main tip work (#4254) and release-line targeting (#4239). The first real tip runs exposed three rough edges in the auto-skia-sync automation, plus that key Skia-update knowledge lived only in the workflow prompt where the update-skia skill couldn't reach it. ~~ Verify-upstream-or-reapply conflict resolution ~~ A tip merge runs ~291 commits ahead of our submodule base and conflicts on every file carrying an [M1xx] fork cherry-pick. The old "keep ours / resolve what you reasonably can" guidance risked silently dropping a not-yet- upstreamed patch (--theirs) or freezing a stale form of one upstream had since refined (--ours; e.g. our SDF-LCD patch used getMaxScale() while upstream relanded it as sk_ieee_float_divide(1.f, getMinScale())). Replace it with an explicit, auditable policy: classify each fork patch on a conflicted file as upstreamed (take upstream's form, record the SHA) or not upstreamed (re-apply ours on top) -- never a blanket --theirs/--ours, never a silent drop. A mandatory before-merge fork-patch snapshot is cross-checked so every patch lands in the resolution table as upstreamed or re-applied; a patch that is neither is a lost patch -> stop and fix. Applied across gotcha #15, SKILL.md Phase 5, and the workflow tip note. ~~ Allow a required new gn arg through build.cake ~~ The tip build needed skia_use_partition_alloc=false because upstream now defaults it to is_clang while our DEPS deliberately omits partition_alloc; Skia ships an official zero-overhead noop raw_ptr for exactly this case, so =false is the supported embedder path, not a hack. The rule banned all build.cake edits, forcing a one-off --gnArgs CLI flag and leaving durable config non-durable. Split the rule into two tiers: still forbid changing compiler/linker flags (or scripts/infra/native/**) to silence a host build error, but allow adding a genuinely required new upstream gn arg to the affected platforms' native/**/build.cake gn-args lists, flagged in both PR summaries for cross- platform review. Sequencing is honored: the toggle doesn't exist in m150, so the relaxed rule lets the next milestone merge add it in-context rather than breaking the current build. Recommendation for the eventual bump (gotcha #23): keep partition_alloc disabled via the supported noop, not by vendoring the Chromium allocator. ~~ Make the skill standalone; reduce the workflow to CI overrides ~~ main/tip mode and the required-new-gn-arg rule were documented only in the workflow prompt, so the update-skia skill couldn't drive them locally. Move both into the skill: tip mode becomes a first-class third option in Phase 1 (new {UPSTREAM_REF} variable, merge-base..upstream/main milestone diffs, generalized Phase 5 merge command), and the gn-arg rule lands in Phase 7 backed by gotcha #23. Trim the workflow's duplicated build.cake/mode prose to short pointers, keeping only genuinely CI-specific guardrails (can't apt- install in the sandbox; a missing host dep is a workflow bug, not a flag hack; Linux-only-build cross-platform review). These are prompt-body edits, so the lock stays byte-identical. ~~ Stop mislabeling a real sync as a no-op ~~ A sync that merged upstream, built, tested, and opened both PRs was reported by the gh-aw conclusion job as a no-op, which then failed trying to file a no-op issue (the workflow has no issues: write). Cause: safe-outputs staged mode blocks the agent from creating anything directly -- the real PRs are opened by the host post-step skia-sync-push-prs.sh with the autobump token -- so noop was the agent's only completion signal and every run, real or not, signalled noop. Give the agent an honest signal: declare create-pull-request in safe-outputs, kept staged (preview-only; the safe_outputs job is ubuntu-slim with permissions:{} and never touches the workspace) so a real sync registers as a pull-request output, while real PRs still come only from the post-step. Set noop: report-as-issue: false so noop is reserved for genuine no-work runs and no longer files an uncreatable issue; drop the unused create_issue offer; and add a "Completion signal" section to the prompt. This is the one frontmatter edit in the PR, so auto-skia-sync.lock.yml is regenerated. Validated end-to-end by run 28269474099 (mode=main, real 2-commit merge): the agent emitted a staged create_pull_request, the conclusion logged no noop with no failed issue step, and the post-step still pushed skia-sync/main to both repos and updated the real PRs (#4263 + #269). Rebased on main to pick up the #4260 workflow rename (lock recompiled to a 0-diff) and confirm consistency with the #4267 support-status model. Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
What
Extends the daily Skia upstream sync automation (
auto-skia-sync) so it can target a maintenance line —release/{major}.{milestone}.x— instead of onlymain, refactors the milestone/branch resolution out of inline workflow YAML into committed, testable scripts, runs the agent on Claude Opus 4.7, and fixes a native-build regression that broke the sync's Linux build.Example: dispatching the workflow with milestone
148now opens PRs againstrelease/4.148.xin both mono/SkiaSharp and mono/skia, rather thanmain/skiasharp.Why
Release lines need ongoing bug-fix syncs from upstream Skia, but the sync workflow only knew how to target
main. The branch-resolution logic also lived as two large duplicated inline bash blocks in the workflow source, which was hard to read, review, and test.How it works
Branch-line resolution (
skia-sync-detect.sh, the single source of truth):current/next/latestcron, or an explicitmilestonedispatch input).main's milestone (the newest line is always served bymain).release/{major}.{milestone}.x);main⇄ mono/skiaskiasharp.Submodule alignment (
skia-sync-align-submodule.sh): pointsexternals/skiaat the SHA the base branch records before the agent runs, since the agent checks out the workflow branch (usuallymain).Refactor
The milestone/branch logic was extracted from two inline YAML bash blocks into committed scripts, collapsing the workflow
run:blocks to ~1 and ~5 lines and removing the duplicated derivation:pre_activation(--gate) runs the detector and writes job outputs; it now sparse-checks-out.github/scriptsso it can call the script.--gate) and sources its output — necessary because the agent job can't readpre_activationoutputs in gh-aw.The extraction also folds in two release-resolution review fixes: numeric
-lt(was!=) and hard-fail on multi-match (was silenthead -1).A follow-up moved the cron→mode mapping out of the detector and into the workflow: the 7 AM / 12 PM / 5 PM →
current/next/latestdecision is now a single GitHub Actions expression next to where the crons are declared, andskia-sync-detect.shtakes real--mode/--milestonearguments instead of readingINPUT_MODE/INPUT_MILESTONE/SCHEDULEenv vars. The script no longer knows anything about cron, so it is self-contained and unit-testable (--mode latestinstead of fakingSCHEDULE='0 17 * * *'). The free-formmilestoneinput is still staged via env (not interpolated intorun:) to avoid shell injection.Model: Claude Opus 4.7
The workflow now pins the agent engine to
claude-opus-4.7via theengine.modelblock (was the gh-aw default Sonnet). Opus is materially more reliable on this multi-repo, submodule-heavy task — across validation it produced cleaner runs with no failed commands, retries, or self-introduced build hacks. Opus 4.6 and 4.7 carry the same AI-credit multiplier, so 4.7 is a free upgrade to the latest snapshot.Native build fix (libc++)
The sync's pre-agent "Install native build dependencies" step now also installs
libc++-dev libc++abi-dev.Root cause: PR #4062 ("Migrate Linux Docker builds to .NET official cross-compilation images") moved the Linux native build into the
mcr.microsoft.com/dotnet-buildtools/prereqscross images, which ship Clang + libc++.native/linux/build.cakeconsequently hardcodes-stdlib=libc++in itsextra_cflags/extra_ldflags. The sync workflow builds on a bareubuntu-latesthost (not inside that Docker image), so libc++ was absent and the agent'sdotnet cake --target=externals-linuxfailed to link. The agent could not self-remediate — there is no apt/sudo inside the AWF chroot, and the firewall blocks OS package mirrors — so it got stuck. Installing libc++ on the host (visible to the chroot via the shared filesystem) resolves it.The agent prompt was also hardened to forbid patching native build files or compiler/linker flags and attempting package installs; if a host dependency is genuinely missing it must STOP and report it under "items needing human attention" rather than work around it.
Files
.github/scripts/skia-sync-detect.sh(new) — milestone/branch-line resolution + gate checks; takes--mode/--milestoneargs.github/scripts/skia-sync-align-submodule.sh(new) — submodule alignment.github/scripts/skia-sync-push-prs.sh— release-aware--basetargeting and PR titles.github/workflows/auto-skia-sync.md/.lock.yml— slimmed steps, new outputs, sparse checkout, cron→mode expression,engine.model: claude-opus-4.7, libc++ install, hardened prompt.agents/skills/update-skia/SKILL.md— documents release-target modeValidation
Static / unit:
gh aw compile auto-skia-sync→ 0 errors (4 pre-existing warnings); lock verified to reflect the source (model in all 4 places, libc++ in the apt line,--mode/--milestoneat both call sites, no staleINPUT_*/SCHEDULE).bash -nclean on all scripts.skia-sync-detect.shexercised against stubbedgh/git:--mode current/next/latest, the default, explicit--milestoneoverride, empty--milestone, and unknown-arg rejection all resolve as before; plus main-line vs release-line resolution, mono/skia-branch-missing fail, ambiguous-match fail, and missing-upstream fail.set -euo pipefailhazards confirmed sound.Live end-to-end (dispatched against
release/4.148.x, real upstream, real native build + tests):milestone=150mode=latestmilestone=148(already synced)milestone=151(nochrome/m151yet)milestone=148, 2 commits behindrelease/4.148.xrelease/4.148.xin both reposThe full-sync run (
28203686018) transcript was audited line-by-line: 0 failed commands, 0 retries, 0 self-introduced build hacks — the agent built, regenerated bindings (no-op), ran the full test suite, and committed onlycgmanifest.json+ the submodule pointer. Validation PRs were closed and their branches deleted after verification.Co-authored-by: Matthew Leibowitz mattleibow@live.com
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com