ci: check the csp override wherever its fixes live - #9
Merged
Merged
Conversation
The patch check assumes every group is .patch files in servo-patches/, and hard-fails on an empty glob. #2 moves the content-security-policy fixes to commits on a fork and deletes csp-0001 accordingly, so merging it as-is turns the check red on main with "no patches matched csp-*.patch". Confirmed by merging the two locally. Rather than special-casing the crate, check whichever arrangement the README documents. A `git`+`rev` override means the fixes are commits on a fork and there is nothing to apply — what can rot there is the pin, so verify the rev still fetches and that the crate version at it matches what Cargo.lock resolves. That is exactly the condition cargo requires to accept a [patch] entry, and exactly what breaks when the crate publishes a new version and the fork is left behind: silent in the manifest, loud at build time. Anything else keeps the old behaviour of applying csp-*.patch to the revision behind the published crate, so this is green both before and after #2 lands and needs no merge ordering. Two things fell out of writing it. The discriminator cannot be "is there an override line" — main already has one, naming a path, which means the reader applied the patch files locally. It has to be whether the line names a git rev. And a git source with no rev now fails outright: that pins a branch, the fork branch is rebased whenever upstream master moves, so a source-expression matcher would change behaviour under `cargo update`. That is the reasoning #2 gives for pinning a rev, enforced rather than just written down. `checkout` now returns instead of exiting so the fork path can report its own failure. Verified both ways: green on main as it stands, applying csp 1/1 to 05528760; and green merged with #2, reporting the pin at fb5fd0f1 with crate 0.8.2 against a lockfile resolving 0.8.2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The closing advice only described regenerating .patch files, which is the wrong instruction when what broke is a fork pin no longer satisfying the version the lockfile resolves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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.
#4's check assumes every group in the series is
.patchfiles underservo-patches/, and hard-fails on an empty glob. #2 moves thecontent-security-policy fixes to commits on a fork and deletes
csp-0001accordingly, so merging it as-is turns the check red on
main:Confirmed by merging the two branches locally. This adapts the check so the
two PRs are independent — it is green both before and after #2 lands, so
neither has to wait for the other.
What it checks now
Whichever arrangement the README documents, rather than special-casing the
crate:
git+revoverride means the fixes are commits on a fork and thereis nothing to apply. What can rot there is the pin, so verify the rev still
fetches and that the crate version at it matches what
Cargo.lockresolves.That is exactly the condition cargo requires to accept a
[patch]entry, andexactly what breaks when the crate publishes a new version and the fork is
left behind — silent in the manifest, loud at build time.
csp-*.patchto therevision behind the published crate.
The fork branch is rebased whenever upstream master moves — which is why the
override pins a rev — so it is deliberately not checked against upstream's
current tip.
Two things that fell out of writing it
The discriminator cannot be "is there an override line".
mainalready hasone, and it names a
path— meaning the reader applied the patch fileslocally. It has to be whether the line names a git rev. My first attempt got
this wrong and the check caught it.
A git source with no rev now fails outright. That pins a branch, the fork
branch is rebased, so what a CSP source-expression matcher does would change
under
cargo update. That is the reasoning #2 gives for pinning a rev —enforced rather than only written down.
Verification
mainas it stands (path override, csp patch file)csp: 1/1 appliedat05528760pinned to fb5fd0f1 … (crate 0.8.2)this pin no longer satisfies itA branch pin changes meaning whenever the fork is rebasedThe failure hint now covers both arrangements; previously it only described
regenerating
.patchfiles, which is the wrong instruction for a stale pin.🤖 Generated with Claude Code