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: add support for newer Windows SDKs #72

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

feat: add support for newer Windows SDKs #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ffd9b03
feat: support Windows 11 SDK
stevapple Sep 26, 2023
5565d3f
feat: update Visual Studio and Windows SDK
stevapple Sep 26, 2023
f4343a9
fix: fixme for support file setup
stevapple Sep 26, 2023
63eac44
fix: run linter
stevapple Sep 26, 2023
b48af66
fix: update test to use Visual Studio 2022
stevapple Sep 26, 2023
1d3c38f
fix: Windows version judgement was accidentally inverted
stevapple Sep 26, 2023
e2b56f4
fix: revert some Visual Studio requirement changes
stevapple Sep 26, 2023
d1e443e
build: update `dist/index.js`
stevapple Sep 26, 2023
4ea245a
test: mock Windows 10 and 11 for SDK resolution
stevapple Sep 26, 2023
1ceff75
chore: integration test for Windows Server 2019
stevapple Sep 26, 2023
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
3 changes: 3 additions & 0 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,9 @@ jobs:
- os: windows-latest
swift: '5.3'
development: false
- os: windows-2019
Copy link
Contributor

Choose a reason for hiding this comment

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

please add windows-2019 with latest swift as well

swift: '5.3'
development: false

steps:
- name: Checkout repository
Expand Down
26 changes: 22 additions & 4 deletions __tests__/installer/windows.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import * as os from 'os'
import * as path from 'path'
import {promises as fs} from 'fs'
import * as core from '@actions/core'
import * as exec from '@actions/exec'
import * as cache from '@actions/cache'
import * as toolCache from '@actions/tool-cache'
import os from 'os'
import {coerce as parseSemVer} from 'semver'
import {WindowsToolchainInstaller} from '../../src/installer/windows'
import {VisualStudio} from '../../src/utils/visual_studio'
Expand All @@ -23,15 +23,15 @@ describe('windows toolchain installation verification', () => {
}
const visualStudio = VisualStudio.createFromJSON({
installationPath: path.join('C:', 'Visual Studio'),
installationVersion: '16',
catalog: {productDisplayVersion: '16'},
installationVersion: '17',
catalog: {productDisplayVersion: '17'},
properties: {
setupEngineFilePath: path.join('C:', 'Visual Studio', 'setup.exe')
}
})
const vsEnvs = [
`UniversalCRTSdkDir=${path.join('C:', 'Windows Kits')}`,
`UCRTVersion=10.0.17063`,
`UCRTVersion=10.0.22000`,
`VCToolsInstallDir=${path.join('C:', 'Visual Studio', 'Tools')}`
]

Expand All @@ -58,6 +58,24 @@ describe('windows toolchain installation verification', () => {
])
})

it('tests setting up on Windows 10', async () => {
jest.spyOn(os, 'release').mockReturnValue('10.0.17063')
const installer = new WindowsToolchainInstaller(toolchain)
expect(installer['vsRequirement'].components).toStrictEqual([
'Microsoft.VisualStudio.Component.VC.Tools.x86.x64',
'Microsoft.VisualStudio.Component.Windows10SDK.19041'
])
})

it('tests setting up on Windows 11', async () => {
jest.spyOn(os, 'release').mockReturnValue('10.0.22621')
const installer = new WindowsToolchainInstaller(toolchain)
expect(installer['vsRequirement'].components).toStrictEqual([
'Microsoft.VisualStudio.Component.VC.Tools.x86.x64',
'Microsoft.VisualStudio.Component.Windows11SDK.22621'
])
})

it('tests download without caching', async () => {
const installer = new WindowsToolchainInstaller(toolchain)
expect(installer['version']).toStrictEqual(parseSemVer('5.8'))
Expand Down
4 changes: 2 additions & 2 deletions __tests__/utils/visual_studio/setup.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ describe('visual studio setup validation', () => {
const env = process.env
const visualStudio = VisualStudio.createFromJSON({
installationPath: path.join('C:', 'Visual Studio'),
installationVersion: '16',
catalog: {productDisplayVersion: '16'},
installationVersion: '17',
catalog: {productDisplayVersion: '17'},
properties: {
setupEngineFilePath: path.join('C:', 'Visual Studio', 'setup.exe')
}
Expand Down
26 changes: 18 additions & 8 deletions dist/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 8 additions & 4 deletions src/installer/windows/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,20 @@ import {Installation} from './installation'

export class WindowsToolchainInstaller extends VerifyingToolchainInstaller<WindowsToolchainSnapshot> {
private get vsRequirement() {
const reccommended = '10.0.17763'
const recommended = '10.0.19041'
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for this change? Swift.org mentions 10.0.17763 is the lowest version supported and I would like the action have maximum compatibility.

Copy link
Contributor Author

@stevapple stevapple Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IIRC Windows 10 SDK (10.0.17763) is the default of Visual Studio 2017, which is long outdated and has compatibility issues with even VS 2019. Official Swift toolchains and SDKs are actually built against the newest setup, which means Windows 11 SDK (10.0.22621) for Swift 5.9.

I actually think Windows 11 SDK (10.0.22000) is better since it’s the default of Visual Studio 2022, but 19041 is new enough for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current logic uses the grater SDK version between current platform and 10.0.17763. So for Windows 11 version 10.0.22621, that version SDK will be chosen but if action is used on a Windows version lesser than 10.0.17763 then SDK of 10.0.17763 will be used.

This variable should be the lowest SDK version supported by Swift, not the latest one. Otherwise action will download the SDK unnecessarily, when it can reuse pre-installed SDK version.

const current = os.release()
const version = semver.gte(current, reccommended) ? current : reccommended
const winsdk = semver.patch(version)
const version = semver.gte(current, recommended) ? current : recommended
const winsdkMajor = semver.lt(version, '10.0.22000')
? semver.major(version)
: 11
const winsdkMinor = semver.patch(version)
Copy link
Contributor

Choose a reason for hiding this comment

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

please extract the windows major version logic to a separate function, it will be easier to update in future.

const componentsStr = core.getInput('visual-studio-components')
const providedComponents = componentsStr ? componentsStr.split(';') : []
return {
version: '16',
components: [
'Microsoft.VisualStudio.Component.VC.Tools.x86.x64',
`Microsoft.VisualStudio.Component.Windows10SDK.${winsdk}`,
`Microsoft.VisualStudio.Component.Windows${winsdkMajor}SDK.${winsdkMinor}`,
...providedComponents
]
}
Expand Down Expand Up @@ -71,6 +74,7 @@ export class WindowsToolchainInstaller extends VerifyingToolchainInstaller<Windo
}
core.debug(`Swift installed at "${swiftPath}"`)
const visualStudio = await VisualStudio.setup(this.vsRequirement)
// FIXME(stevapple): This is no longer required for Swift 5.9+
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to address this in current PR or in a future PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A future PR I think. Needs to figure out how to pass the version around, so it might be a big fix.

await visualStudio.update(installation.sdkroot)
const swiftFlags = `-sdk %SDKROOT% -I %SDKROOT%/usr/lib/swift -L %SDKROOT%/usr/lib/swift/windows`
core.exportVariable('SWIFTFLAGS', swiftFlags)
Expand Down