Coverage CI: cache OPAM env, rewatch build, and coverage dune state#8434
Coverage CI: cache OPAM env, rewatch build, and coverage dune state#8434JonoPrest wants to merge 3 commits into
Conversation
The OPAM install step on a cold runner often takes 10+ minutes and periodically times out / fails. This mirrors the cache strategy used by the main CI workflow: store the OPAM tree (incl. dev-setup deps like bisect_ppx), the rewatch cargo target, and the instrumented dune build state. PRs restore the latest master cache; only master pushes write to it, so PRs can't poison it. The OPAM cache key is distinct from the main CI workflow's because coverage installs --with-dev-setup (for bisect_ppx) while CI only uses --with-test.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84b166265b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Save OPAM environment | ||
| if: steps.cache-opam-env.outputs.cache-hit != 'true' | ||
| uses: actions/cache/save@v5 |
There was a problem hiding this comment.
This save step runs on every cache miss, including pull_request runs, so it will upload PR-scoped caches. Per GitHub cache semantics, cache misses auto-create a cache, and caches created on pull_request are tied to refs/pull/.../merge and are not restorable by master or other PRs; that means repeated PR misses for a new key will create isolated entries that add storage pressure and can evict the shared branch cache, reducing hit rate and increasing future OPAM install time. Gate this save to push on refs/heads/master (as done for coverage build state) to keep the cache reusable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3a99217 — added github.event_name == 'push' && github.ref == 'refs/heads/master' to the save condition, matching the gating already used on the coverage-build-state cache. PRs still restore the latest master cache on hit; they just don't write new PR-scoped entries.
actions/cache/save@v5 on cache miss runs on every event including pull_request, which creates PR-scoped cache entries tied to refs/pull/<n>/merge. Those entries aren't restorable by master or other PRs, so they consume storage and can evict the shared master-saved entry, reducing hit rate. Restrict the save to push events on refs/heads/master, matching the pattern already used for the coverage-build-state cache. PRs still benefit from full restore via the latest master cache; they just don't write new entries. Flagged by Codex review on rescript-lang#8434.
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
1 participant
The OPAM install step on a cold runner often takes 10+ minutes and periodically times out / fails. This mirrors the cache strategy used by the main CI workflow: store the OPAM tree (incl. dev-setup deps like bisect_ppx), the rewatch cargo target, and the instrumented dune build state. PRs restore the latest master cache; only master pushes write to it, so PRs can't poison it.
The OPAM cache key is distinct from the main CI workflow's because coverage installs --with-dev-setup (for bisect_ppx) while CI only uses --with-test.