fix(ci): read the runner image instead of interpolating an empty string - #601
Conversation
`${{ env.ImageOS }}` and `${{ env.ImageVersion }}` have expanded to nothing
since the line was written. The `env` expression context holds only what a
workflow, job or step `env:` block put there; the runner sets these two into
its own process environment, where `run:` steps see them and `${{ env.… }}`
does not. The repository's own cache list shows the result — three consecutive
hyphens where the image should be:
whisper-stt-build-darwin-arm64---2ca5d2c75aac59ca…
So the cache was keyed on the tag and the CMakeLists hash alone, and the exact
failure the comment above it predicts — a restored tree with dead absolute SDK
paths, "No rule to make target …libz.tbd" — was never actually guarded against.
A shell step reads the values and both the key and the restore-keys prefix use
its output. `shell: bash` is not decorative: this matrix includes
windows-latest, whose default shell has no `${VAR:-default}`.
Second, unrelated to the key but proven by the same cache list's neighbours:
`on.push` had `paths:` and no `branches:`, so a tag push matched. Publishing
`v0.0.0-onnxruntime-1.27.1` started a four-platform whisper build for a tag
that touches none of these files. `branches: ['**']` excludes tags while
keeping every branch, which is how this workflow is used.
Closes #596
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe workflow now limits push-triggered builds to branches. It reads ChangesWhisper STT workflow cache correction
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The workflow now avoids tag-triggered builds and scopes whisper.cpp caches to the runner image, preventing stale build-tree reuse after SDK or Xcode changes. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Verified on a real run, not argued. Pushing the branch triggered run 33894430230 — four legs, all green, 8 minutes — and these are the keys it actually wrote: Against the Two things the keys settle beyond the fix itself:
|
1 participant
Closes #596.
build-whisper-stt.ymlkeys its build cache on the runner image so a Xcode/SDK roll busts it, and carries a comment explaining precisely why that matters. The mechanism has never once worked.${{ env.ImageOS }}and${{ env.ImageVersion }}both expand to the empty string: theenvexpression context holds only what a workflow, job or stepenv:block put there, while the runner sets these two into its own process environment, visible torun:steps and to nothing else.The repository's own cache list is the evidence — three consecutive hyphens where the image belongs:
So the cache has been keyed on
matrix.tag+ the CMakeLists hash alone, and the failure the comment predicts — a restored tree full of dead absolute SDK paths,No rule to make target …libz.tbd— has been unguarded the whole time. Nothing broke yet because no roll happened to land badly; that is luck, not design.The fix
A shell step reads the values, and both
key:andrestore-keys:use its output — the same shape #595 uses.shell: bashis load-bearing, not decorative. The issue's suggested snippet would have broken on this workflow: the matrix includeswindows-latest, where the default shell is PowerShell and${VAR:-default}is not syntax. GitHub ships bash on the Windows image, so pinning the shell is what makes one step work across all four legs.Second fix, same file
on.pushhadpaths:and nobranches:, so a tag push matched it. This is not hypothetical — it is in the run list:Publishing that ONNX artifact started a four-platform whisper build for a tag touching none of these files. Fixed with
branches: ['**'], which excludes tags while keeping every branch — deliberately notbranches: [main]as in the ONNX workflow, because contributors push branches here specifically to get binaries built, and narrowing that would be a behaviour change nobody asked for.Verification
Pushing this branch triggers the workflow, which is the point: the proof is what lands in the cache list afterwards. The keys must carry a real image and must not contain
---. I will post the actual keys before asking anyone to merge — an assertion that cannot fail is worth nothing here, and this is a fix whose entire failure mode was looking correct while doing nothing.What a reviewer should contest
branches: ['**']is the permissive choice. If the preference is that this workflow only build frommainand PRs, say so; it is a one-line change and a product-of-workflow call rather than a technical one.:-unknownfallbacks would silently degrade on a self-hosted runner that sets neither variable — the key would readunknown-unknownand stop busting on toolchain rolls again. Every runner here is GitHub-hosted, so it does not bite today; failing loudly instead is defensible if you would rather.Summary by CodeRabbit