feat: support vs2022#2533
Conversation
|
ping |
rvagg
left a comment
rvagg
left a comment
There was a problem hiding this comment.
tbh this is entirely rubber stamp from me, the existence check for msbuildPath seems like a bit of a concern, but 🤷 if it works?
| 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.
| 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?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
4 participants
closes #2520.
Since github action now provide vs2022, so I also add vs2022 to CI.
Verified on win10.