Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support vs2022 #2533

Merged
merged 1 commit into from
Oct 29, 2021
Merged

feat: support vs2022 #2533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .github/workflows/visual-studio.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
name: Tests on Windows
on: [push, pull_request]
jobs:
Tests:
strategy:
fail-fast: false
max-parallel: 15
matrix:
os: [windows-2022]
runs-on: ${{ matrix.os }}
steps:
- name: Checkout Repository
uses: actions/checkout@v2
- name: Install Dependencies
run: |
npm install --no-progress
- name: Set Windows environment
if: matrix.os == 'windows-latest'
Copy link
Contributor

@cclauss cclauss Oct 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#example-excluding-configurations-from-a-matrix

run: |
echo 'GYP_MSVS_VERSION=2015' >> $Env:GITHUB_ENV
echo 'GYP_MSVS_OVERRIDE_PATH=C:\\Dummy' >> $Env:GITHUB_ENV
- name: Environment Information
run: npx envinfo
- name: Run Node tests
run: npm test
14 changes: 13 additions & 1 deletion lib/find-visualstudio.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const log = require('npmlog')
const execFile = require('child_process').execFile
const fs = require('fs')
const path = require('path').win32
const logWithPrefix = require('./util').logWithPrefix
const regSearchKeys = require('./util').regSearchKeys
Expand Down Expand Up @@ -257,22 +258,31 @@ VisualStudioFinder.prototype = {
ret.versionYear = 2019
return ret
}
if (ret.versionMajor === 17) {
ret.versionYear = 2022
return ret
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign in to view

this.log.silly('- unsupported version:', ret.versionMajor)
return {}
},

// Helper - process MSBuild information
getMSBuild: function getMSBuild (info, versionYear) {
const pkg = 'Microsoft.VisualStudio.VC.MSBuild.Base'
const msbuildPath = path.join(info.path, 'MSBuild', 'Current', 'Bin', 'MSBuild.exe')
if (info.packages.indexOf(pkg) !== -1) {
this.log.silly('- found VC.MSBuild.Base')
if (versionYear === 2017) {
return path.join(info.path, 'MSBuild', '15.0', 'Bin', 'MSBuild.exe')
}
if (versionYear === 2019) {
return path.join(info.path, 'MSBuild', 'Current', 'Bin', 'MSBuild.exe')
return msbuildPath
}
}
// visual studio 2022 don't has msbuild pkg
if (fs.existsSync(msbuildPath)) {
return msbuildPath
}
return null
},

Expand All @@ -293,6 +303,8 @@ VisualStudioFinder.prototype = {
return 'v141'
} else if (versionYear === 2019) {
return 'v142'
} else if (versionYear === 2022) {
return 'v143'
}
this.log.silly('- invalid versionYear:', versionYear)
return null
Expand Down