Repository navigation
ci: wrap configure with eatmydata, keep the package presence test on command -v - #2
Merged
Merged
Conversation
…data build-cache/action.yml checked ccache/mold with `command -v` (a wrong-PATH false negative is harmless -- worst case a redundant apt-get install) and never checked eatmydata at all. Switch the three to dpkg -s and let eatmydata skip fsync during ./configure's small scratch writes, never make (the compiled objects are what the build-tree/ccache caches actually rely on staying byte-correct). Measured on builder02: dpkg -s x3 ~0.03s vs a bare apt-get update ~1s, so the check-before-install path avoids that network round trip on every warm job. eatmydata itself showed no measurable win on this SSD-backed runner (~3.6s either way across 3 runs) -- kept because it's a correct no-op here and a real win on slower/fsync-throttled runners, not because of a saving this box demonstrates.
dpkg -s exits 0 for a package left in rc (deinstall ok config-files) state, whose binary is gone -- verified on the build host: dpkg -s libzfs6linux exits 0 while command -v finds nothing. As the presence test for ccache/mold/eatmydata that skips the install of a package that is not actually there and fails the build downstream. The swap also bought nothing it was credited with: this step has gated apt-get behind the presence check since before eatmydata joined the list, so the not-reinstalling saving was already being realised. Comments in both files now say that plainly, and say why a real actions/cache layer over a shared system dpkg database would violate the no-stale-artifact rule.
WalkthroughThe build-cache action now installs ChangesCache-enabled build flow
Sequence Diagram(s)sequenceDiagram
participant BuildCacheAction
participant ciBuildScript
participant Configure
BuildCacheAction->>BuildCacheAction: Install missing cache tools
ciBuildScript->>ciBuildScript: Check NO_CACHE and eatmydata
alt Cache hit
ciBuildScript->>ciBuildScript: Skip configure
else Cache miss
ciBuildScript->>Configure: Run ./configure through eatmydata when enabled
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The step said its install path was for codeql.yml. codeql.yml does not use this action at all -- it skips ccache deliberately, since a cache hit that never runs cc is invisible to CodeQL's tracer. The hosted path that does exist is the fork fallback every consumer shares: runs-on resolves to ubuntu-latest when head.repo.fork is true, and to the builder02 self-hosted labels otherwise.
eatmydata is an LD_PRELOAD shim, and an ASan-instrumented binary aborts
at startup unless the ASan runtime comes first in the preload list. In
asan mode every one of configure's probe binaries is instrumented, so
they all die and configure reports the symptom instead of the cause:
==2722==ASan runtime does not come first in initial library list
./configure: error: can not detect int size
Caught by CI run 30630376885, not locally, because the first pass was
only exercised in module mode. mold is already skipped in asan mode for
a milder reason (the sanitizer runtimes want the default linker); this
one is a hard incompatibility rather than a preference.
Verified both ways on the build host: with the guard, an asan build of
nginx 1.31.3 configures and compiles clean; with it reverted and the
configure stamp cleared, configure fails with the exact error above.
This was referenced Aug 1, 2026
eilandert
added a commit
that referenced
this pull request
Aug 6, 2026
The ci/feedback/ README's rule: acting on a finding means deleting it in the same commit, because a file that outlives the thing it describes reads as a live problem forever. These should have gone with the two commits that resolved them. nginx-http-shield-module-2026-08-05.md removed entirely -- all three findings are resolved and the file's header describes findings that no longer exist: - #1 (no gate ties the runner ban to the TRIGGER) is REFUTED, not ported. The claim was that a bare [self-hosted, builder02, lxc] on pull_request: passes check_runners; it does not. TRUST_SPLITS holds full fork-ternary strings from _trust_split(), so a bare list fails the membership test. The proposed replacement is also less trigger-aware than what is here: it greps a literal pull_request trigger and would go vacuous on every workflow_call member. - #2 (nothing enforces a member carries no push:) was ported in 2026-08-06 as lint-ci-cadence.sh and left undeleted. - #3 (fuzz.dict drift) acted on in 2b0f7b2 as a PROMPT.md step 27 instruction. nginx-label-autoconf-module-2026-08-05.md keeps its record of what was verified but loses #5, acted on in 2b0f7b2. The numbering keeps the gap deliberately -- the summary table and sibling findings cite these numbers. Finding 6 stays: it is a recommendation about what step 44 should require of an adopter claiming "no request surface", and still needs an owner call rather than a fix.
eilandert
added a commit
that referenced
this pull request
Aug 6, 2026
…t instruction) (#26) * ci: content-stamp the .github/ files an adopter copies An adopting module copies this repo's .github/ tree and then edits it. Neither side could answer "is that copy of build-test.yml still the one the skeleton ships, or did the adopter edit it / did the skeleton move?" without diffing every file by hand. sync-stamp.sh puts a `# sync-sha:` line in each shared file, excluded from its own digest so it is stable across re-stamping. `--list` on two repos, diff the output, and every drifted file names itself. lint-sync-stamp.sh runs --check so a stamp cannot rot into decoration: an edit without re-stamping goes red. Ported from nginx-error-abuse-module 8cdbfb2 rather than the earlier variant in nginx-label-autoconf-module. The differences are load-bearing: it rejects a duplicate stamp line (content_sha strips every one while stamp_of reads only the first, so a second line injected by a bad merge is invisible to both and --check would pass); it materialises the target list so a find that aborts is catchable rather than reaching the loop as a plain EOF and reporting "all N current" over fewer files; it sorts and matches both .yml/.yaml spellings; and it writes back through the original file, keeping the mode without GNU-only chmod --reference. Also adds ci-cadence to lint.yml's LINT_ONLY. It was added in 2026-08-06 and had never run remotely -- the allowlist is narrower than run-all.sh's glob, so a checker present locally can be absent from every PR. Verified: --check goes red on a stale stamp, a missing stamp, a duplicate placed after the genuine one, and a discovery failure (exit 2), and stamp mode repairs a duplicate back to exactly one line. * ci: require typed per-member secrets, and gate the fuzz-dictionary instruction Two adoption-feedback decisions, from ci/feedback/. Each is deleted from its feedback file in this commit, per that directory's README: a finding that outlives the thing it describes reads as a live problem forever. label-autoconf #5 -- every workflow_call member here declares `workflow_call: {}` with no secrets: stanza. That is not "needs no secrets", it is "can never receive one": an undeclared secret is not passed, so a member that starts needing a token sees an empty string and fails downstream of the cause -- a checkout that 404s, a curl that 401s -- with nothing naming the call boundary. Of the two available stances this takes (a), typed `required: true` per member, over (b) `secrets: inherit` at the caller. (b) is less boilerplate but widens every member's blast radius to the caller's full secret set, including secrets it never reads. A member's secret surface belongs at the member. This repo declares no secrets and should not: every job is contents: read over a public tree. The member half of the check is therefore vacuously green here by design, and its value is the moment an adopter adds the first one. The caller half is live today -- it goes red on inherit and on a secret passed to a member that never declared it. shield #3 -- PROMPT.md step 27 told an adopter to "use target tokens in the dictionary" without suggesting deriving them from the source of truth or gating the drift. A hand-listed fuzz.dict goes stale silently: adding a signature and forgetting the dictionary does not fail the fuzz gate, because a merely incomplete dictionary still produces a green crash-only run. Step 27 now says generate-and-drift-gate when the tokens live in a table, and records the measurement that makes the result readable: deriving it moved cov not at all (199 both arms) while signature reach went 23 -> 35 of 645 table literals. An adopter judging it by edge coverage concludes it did nothing. Each branch of the secrets check has a fixture that dies when the branch dies. Three earlier mutants survived and each exposed a real defect rather than a missing test: - `secrets: inherit` fell through to `set(passed)` and was iterated character by character -- red, for the wrong reason. Now an explicit PolicyError. - the missing-`required`-key branch and the value branch were redundant: every spec without the key yields None from .get(), and None is not True, so deleting the first changed no verdict. The split now runs on not-a-mapping vs mapping, which .get() genuinely cannot decide. - deleting the isinstance guard made the null-spec fixture die on None.get(). An AttributeError also exits 1, so an exit-status-only control stayed green through a crash. selftest.sh grows policy_msg_ for that case: red, and red with the stated finding. selftest.sh also now cross-checks lint.yml's LINT_ONLY against the ci/linter/ glob. That allowlist is narrower than what run-all.sh discovers, so a checker can run locally and in the hook while being absent from every PR -- which is how ci-cadence shipped without ever running remotely. * ci(feedback): delete the findings acted on in 8e81254 and 2b0f7b2 The ci/feedback/ README's rule: acting on a finding means deleting it in the same commit, because a file that outlives the thing it describes reads as a live problem forever. These should have gone with the two commits that resolved them. nginx-http-shield-module-2026-08-05.md removed entirely -- all three findings are resolved and the file's header describes findings that no longer exist: - #1 (no gate ties the runner ban to the TRIGGER) is REFUTED, not ported. The claim was that a bare [self-hosted, builder02, lxc] on pull_request: passes check_runners; it does not. TRUST_SPLITS holds full fork-ternary strings from _trust_split(), so a bare list fails the membership test. The proposed replacement is also less trigger-aware than what is here: it greps a literal pull_request trigger and would go vacuous on every workflow_call member. - #2 (nothing enforces a member carries no push:) was ported in 2026-08-06 as lint-ci-cadence.sh and left undeleted. - #3 (fuzz.dict drift) acted on in 2b0f7b2 as a PROMPT.md step 27 instruction. nginx-label-autoconf-module-2026-08-05.md keeps its record of what was verified but loses #5, acted on in 2b0f7b2. The numbering keeps the gap deliberately -- the summary table and sibling findings cite these numbers. Finding 6 stays: it is a recommendation about what step 44 should require of an adopter claiming "no request surface", and still needs an owner call rather than a fix. * ci(secrets): catch a required secret that no caller wires Self-review of the check found it green in the mirror direction of the case it was written for: a member declaring `required: true` with no caller passing it. Both halves read as correct alone -- the declaration is the endorsed shape, and a bare `uses:` with no secrets block is right for every other member here. GitHub does refuse to start that call, so this fails loudly rather than silently. That is what `required: true` buys. But it fails on the first run after merge, and the caller/member pairing is checkable at review time, which is the point of having the linter at all. `secrets-required-not-wired` fixture added as the red twin of `secrets-undeclared`; verified by mutation (disarming the branch turns it green). Also drops a dead `member` reassignment and computes `call` after the guard that validates it rather than before. * docs(prompt): correct the linting steps for the checkers added in this PR Answering "is there a step for zizmor and C/nginx linting": yes, steps 32-35 install the gate, 46 re-proves every checker still bites, 47 audits the drift classes. The C and nginx checkers have their own hazard covered at step 12 -- they pass green on an EMPTY selection, so the probe plants malloc/strcpy beside the module's real C and requires exit 1. What was wrong rather than missing: - Step 34 quoted the reference's LINT_ONLY as a literal string, which this PR had just invalidated. It now says to read the current value and warns that a string quoted in a document (including that one) is the thing that goes stale. - Step 34 and 47 both said nothing cross-checks LINT_ONLY against the scripts that exist. A selftest.sh case does now, so 34 says to port the case rather than the string and 47 says to re-run it. - "The three repo-policy checks" and the linter README's "last four" both miscounted after cadence and secrets landed. Replaced the positional wording with the names, so the next addition does not silently falsify it. Also adds the transfer conditions for the checks this PR introduced, since both have adoption caveats an adopter would otherwise hit blind: ci-secrets is vacuously green on a target with no secrets and only its caller half is live immediately; sync-stamp's value is in the reference-to-adopter direction, so an adopter running it in isolation gets a stamp file and no signal. * ci(secrets): reject `secrets: inherit` on external reusable workflows The caller loop filtered to local `./.github/workflows/` members before it judged `secrets: inherit`, so a call to `owner/repo/.github/workflows/x.yml@ref` was skipped entirely. That inverted the severity the check exists to enforce: a local `inherit` overshares within one repository, while the skipped case hands the caller's whole secret set to code maintained elsewhere, at a ref that can move. Judge `inherit` on the call site alone, before the local filter, and keep the rest of the loop local-only -- everything below compares against `declared`, which only knows local members. The new fixture is a real control, not a restatement of secrets-inherit: restoring the old ordering leaves secrets-inherit red and turns secrets-inherit-external green, so the local fixture alone could never have gated this path.
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.
Phase 5 lists six caching layers and this repo had four. The two missing ones turn out to be a real change and a documented non-change.
eatmydata now wraps
./configureinci/tools/ci-build.sh, in every mode except asan. configure spends its time on tiny writes and fsyncs (probe files,autoconf.err, Makefile fragments) that ccache cannot touch, andobjs/is scratch that gets rebuilt fromsrc/on any failure, so losing it to a host crash costs one rerun.makeis deliberately left unwrapped: those objects are what the build-tree and ccache layers hand to later jobs, and durability there is the whole point.asan mode is excluded because eatmydata is an
LD_PRELOADshim and an ASan-instrumented binary aborts at startup unless the ASan runtime is first in the preload list. Every probe binary configure builds is instrumented in that mode, so they all die and configure reports the symptom rather than the cause:ASan runtime does not come first in initial library list, then./configure: error: can not detect int size. mold is already skipped under asan for the milder reason that the sanitizer runtimes want the default linker.The apt layer stays uncached, on purpose.
actions/cacheover/var/lib/dpkgwould be system-wide, shared by every job on a persistent runner, and a stale restore desyncs dpkg's own state, which is exactly the stale-artifact-into-a-green-result failure the caching rules forbid. What is there instead is a presence check that skipsapt-getentirely, which this step has done since before eatmydata joined its list. For an ephemeral runner the answer is a pre-baked image, not a cache bolted onto this script.That check keeps using
command -vrather thandpkg -s.dpkg -sexits 0 for a package sitting inrc(deinstall ok config-files) state, whose binary is long gone: on the build hostdpkg -s libzfs6linuxexits 0 whilecommand -v libzfs6linuxfinds nothing. As a presence test that skips installing something that is not there and fails the build several steps later.Testing
The asan exclusion is verified both ways on builder02. With the guard,
ci/tools/ci-build.sh nginx 1.31.3 asanconfigures and compiles clean. With it reverted andobjs/.conf-stampcleared, configure dies on the preload error above. The first pass of this PR only exercised module mode, which is why CI found this and the local run did not.Module-mode timings at loadavg 0.89, which matters because this box also runs the six CI slots and numbers taken under load are noise: cold build 5.18s, warm rerun 0.03s on the configure-skip stamp.
The honest number for eatmydata here is nothing: three plain configure runs against three wrapped ones both land at 3.6-3.8s. That is an SSD-backed incus container where fsync was already cheap. It is kept for the runner that is not, and the comment in the file says so rather than claiming a win.
ci/linter/run-all.shclean,ci/linter/selftest.shcontrols all held.