ci(whisper-stt): drop the msvc-dev-cmd step the Windows build never used - #641
Open
My-Denia wants to merge 1 commit into
Open
ci(whisper-stt): drop the msvc-dev-cmd step the Windows build never used#641My-Denia wants to merge 1 commit into
My-Denia wants to merge 1 commit into
Conversation
ilammy/msvc-dev-cmd was the last node20 action in the repo, and its upstream stopped in 2024, so there is no newer major to bump to (getopenscreen#317). The issue assumed the step fed a Ninja build, but scripts/build-whisper-stt.sh calls cmake without -G: on Windows CMake picks the Visual Studio generator (the job log reads "Building for: Visual Studio 18 2026"), locates MSVC through the Visual Studio installer and builds with MSBuild, none of which reads the vcvarsall environment the action exported. vcpkg finds the compiler on its own the same way, and windows-latest ships a single VS instance, so there is nothing for vcvars to disambiguate. Remove the step instead of replacing it, and leave a comment at the spot saying why there is none, and when one would become necessary again (a move to Ninja).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Windows Whisper STT workflow removes ChangesWindows build setup
Estimated code review effort: 1 (Trivial) | ~5 minutes Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to This removes an obsolete Windows setup action without changing compiler selection or produced artifacts; the supplied builds show no merge-blocking impact. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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.
Summary
Problem.
ilammy/msvc-dev-cmd@v1inbuild-whisper-stt.ymlis the lastnode20action in the repo, and there is nothing to bump it to: upstream's last push tomasteris from 2024-03-30 and itsaction.ymlstill declaresusing: node20. Every Windows run of this workflow logs the deprecation warning, and once GitHub drops the shim the Windowswhisper-stt-serverbuild stops at that step (#317).What the step actually did. #317 lists three replacements on the assumption that the step feeds a Ninja build. It does not.
scripts/build-whisper-stt.shcallscmakewithout-G, so on Windows CMake picks its default generator, and the Windows job of the latest upstream run (branchclaude/drop-dead-autotranscribe, 2026-09-10; its workflow file and build script are identical tomain's) shows which one:The Visual Studio generator locates the instance through the Visual Studio installer and builds with MSBuild, whose VC targets set the compiler environment themselves. Nothing reads the
PATH/INCLUDE/LIBthatvcvarsallexported. The other Windows-only step, vcpkg installingspirv-headers, does its own toolchain detection too (Compiler found: .../Hostx64/x64/cl.exein its output). So the issue's option 2, building with the VS generator, is the state the workflow has been in all along, and that leaves nothing for a vcvars step to do.Fix. Remove the step. A comment at the spot says why there is none, and when one would become necessary again: a move to Ninja, at which point
scripts/msvcEnv.mjsalready has the vcvarsall discovery.Two things the vcvars environment could have influenced, checked:
VS180COMNTOOLSset (whichvcvarsalldoes), CMake uses that instance; without it, per theCMAKE_GENERATOR_INSTANCEdocs, "if more than one VS instance is installed we do not define which one is chosen by default".windows-latestships exactly one, Visual Studio Enterprise 2026 (image README), so the choice is the same either way. Should GitHub ever add a second instance, it would show up as a different compiler path in the configure log, not as a silent miscompile.Visual Studio 18 2026generator (under cmake-generators(7)): "By default this generator uses the 64-bit variant on x64 hosts".Hostx64/x64/cl.exedoes not come from the action'sarch: x64.Related issue
Fixes #317
Type of change
Release impact
Desktop impact
CI only: the Windows leg of
build-whisper-stt.yml. The produced binary is unchanged (same generator, same compiler).Screenshots / video
n/a
Testing
This workflow's
pushtrigger covers every branch and itspathsfilter includes the workflow file, so the change ran for real on my fork, at this exact commit:whisper-stt-win32-x64.tar.gz. The Windows log has noNode.js 20 is deprecatedwarning and noSetup MSVCstep. It did restore the whisper build-tree cache, though, so CMake reused a persisted instance choice there.workflow_dispatchafter deleting the fork'swhisper-stt-build-win32-x64-*caches, so CMake had to configure from scratch with no vcvars environment. Its Windows log readsCache not found for input keys: whisper-stt-build-win32-x64-…, then-- Building for: Visual Studio 18 2026andThe C compiler identification is MSVC 19.51.36256.0at the same…/VC/Tools/MSVC/14.51.36231/bin/Hostx64/x64/cl.exepath as the upstream run above, and vcpkg reports the sameCompiler found:path. Every leg green again; the Windows artifact is within a few hundred bytes of the one that upstream run produced (gzip timestamps).grep -rn msvc-dev-cmdfinds no other use of the action in the repo.Summary by CodeRabbit