Fix Windows CI against Visual Studio 2026 - #1001
Open
X-Guardian wants to merge 1 commit into
Open
X-Guardian wants to merge 1 commit into
X-Guardian wants to merge 1 commit into
Conversation
|
OX Security reviewed this pull request — nothing to fix.
Branch |
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.
Closes #998
Changes
Drop the hardcoded
--msvs_version=2022fromtest/run.util.jsand let node-gyp detect the toolchain. Each runner image carries exactly one Visual Studio, so there is nothing to disambiguate: the flag only ever asserted a year that happened to match. It was not deliberate either: it began as--msvs_version=2015with the comment "since that is more edge case than 2013", and the2022value arrived via a mechanical bump in #707, needing an edit on every image change since.Add
windows-2022to both matrices. VS 2022 remains widely used and is the image GitHub directs users to when they need it, so it is worth covering alongside VS 2026 rather than testing only the newest toolchain.Add
scripts/upgrade-node-gyp.js, run on Windows vianpm run upgrade-node-gyp. It reads the version of the node-gyp that node-pre-gyp will actually invoke and installs a newer one only when that version predates VS 2026 support.Keying off the resolved node-gyp rather than the Node version matters because the matrix uses floating aliases.
lts/-1is Node 22 today and becomes Node 24 when Node 26 enters LTS, at which point the script finds node-gyp 12 and does nothing, with no annual edit and no comment that goes stale.lib/util/compile.jsexportswhich_node_gypso the script resolves node-gyp exactly as the build does.Result
15 jobs in
ci.ymland 12 ins3-bucket.yml, with Windows covered on both toolchains:lts/-1lts/*latestNon-Windows jobs are unaffected. The upgrade step is guarded by
runner.os == 'Windows', and--msvs_versionwas only ever passed under awin32check.Other Changes
s3-bucket.ymlalso picks up a fix for${{ matrix.node }}references left behind by the matrix key rename in chore: replace the end-of-life Node 20 in the CI test matrices #999. Its tests are skipped whenS3_BUCKETis unset, so the Windows changes there are untested in forks.