feat: support vs2022 #2533
feat: support vs2022 #2533
Conversation
|
ping |
| run: | | ||
| npm install --no-progress | ||
| - name: Set Windows environment | ||
| if: matrix.os == 'windows-latest' |
There was a problem hiding this comment.
Given that Win22 is still beta and is not windows-latest https://github.com/actions/virtual-environments this setup code is not going to be run. Is that a problem? Do we want this setup run on all Windows tests?
if: startsWith(matrix.os, 'windows')
There was a problem hiding this comment.
I assume this is just copypasta from tests.yml and it's not needed here
but why do we need this separate workflow? can't we add windows-2022 to tests.yml? What's this one doing that's unique?
There was a problem hiding this comment.
I assume this is just copypasta from tests.yml and it's not needed here
Yeap.
but why do we need this separate workflow? can't we add windows-2022 to tests.yml? What's this one doing that's unique?
The matrix is 3*3, adding another os will get 9 extra CI pipeline.
https://github.com/nodejs/node-gyp/blob/master/.github/workflows/tests.yml#L10
There was a problem hiding this comment.
| if (ret.versionMajor === 17) { | ||
| ret.versionYear = 2022 | ||
| return ret | ||
| } |
There was a problem hiding this comment.
Sorry to keep commenting on a merged PR but… wouldn’t it be easier to make future changes to:
let versionYears = {
15: 2017,
16: 2019,
17: 2022,
}
ret.versionYear = versionYears[ret.versionMajor]And similar around line 305?
closes #2520.
Since github action now provide vs2022, so I also add vs2022 to CI.
Verified on win10.