feat: add support for newer Windows SDKs #72

stevapple · 2023-09-26T02:07:45Z

Supports Windows 11 SDK correctly;
Bumps the recommended version to Windows 10 SDK (10.0.19041);
Build dist/index.js;
Fixes typo.

This requires a SemVer minor release.

codecov · 2023-09-26T06:58:52Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (5dfc518) 91.00% compared to head (1ceff75) 91.02%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
+ Coverage   91.00%   91.02%   +0.02%     
==========================================
  Files          30       30              
  Lines         900      903       +3     
  Branches      122      125       +3     
==========================================
+ Hits          819      822       +3     
  Misses         80       80              
  Partials        1        1

Files	Coverage Δ
src/installer/windows/index.ts	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

stevapple · 2023-09-26T07:35:22Z

~~Unfortunately this path cannot be easily covered because GitHub runner is already Windows 11. Any hint?~~

The runner is Windows 10. Logic fixed.

stevapple · 2023-09-26T12:30:03Z

@soumyamahunt Can you help to build and push dist/index.js so that end-to-end tests will use the updated version? It seems we need CI/CD for auto-building dist/index.js, or at least block E2E tests if it’s outdated.

stevapple · 2023-09-26T12:31:11Z

And is it necessary & how to test the Windows SDK version logic?

soumyamahunt · 2023-09-26T12:35:39Z

@soumyamahunt Can you help to build and push dist/index.js so that end-to-end tests will use the updated version?

You can run npm run all, this will generate dist/index.js.

It seems we need CI/CD for auto-building dist/index.js, or at least block E2E tests if it’s outdated.

yes for E2E test generated dist/index.js needs to be present in the branch, in future we can explore how to block if dist/index.js is not up-to-date.

soumyamahunt · 2023-09-26T06:30:34Z

src/installer/windows/index.ts

@@ -11,17 +11,18 @@ import {Installation} from './installation'

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


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.

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.

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.

soumyamahunt · 2023-09-26T06:31:44Z

src/installer/windows/index.ts

-    const version = semver.gte(current, reccommended) ? current : reccommended
-    const winsdk = semver.patch(version)
+    const version = semver.gte(current, recommended) ? current : recommended
+    const winsdkMajor = semver.gte(version, '10.0.22000') ? semver.major(version) : 11


There could be better way of writing this that works for future release of Windows, i.e. Windows 12

Basically no one knows what version logic MS will use for Windows 12🤷 Maybe they’re going to stick to 10 as major version….

soumyamahunt · 2023-09-26T06:33:39Z

src/installer/windows/index.ts

@@ -66,6 +67,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+


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

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

src/installer/windows/index.ts

stevapple · 2023-09-26T12:38:03Z

You can run npm run all, this will generate dist/index.js.

I cannot access my computer in ~2hrs and now I’m using an iPad🤦

yes for E2E test generated dist/index.js needs to be present in the branch, in future we can explore how to block if dist/index.js is not up-to-date.

Is it possible to run npm run all before E2E tests?

soumyamahunt · 2023-09-26T12:58:37Z

Is it possible to run npm run all before E2E tests?

Integration tests use npm run build && npm run package to generate latest dist/index.js and then test.

The reason E2E works differently is because I wanted to add test for real-world usage. Unlike other GitHub actions, this action accesses files from swiftorg submodule, which varies based on action is cloned via git or downloaded.

If we go approach of generating dist/index.js, then it needs to be committed first for E2E test. I would rather enforce generation of dist/index.js before committing than complicating CI with auto-commit.

I cannot access my computer in ~2hrs and now I’m using an iPad🤦

No need to hurry, anyway action works fine on current windows runners.

And is it necessary & how to test the Windows SDK version logic?

For now you can add unit-tests with mocking the OS, there is no way to test properly until GitHub adds Windows 11 runner.

stevapple · 2023-09-26T16:52:37Z

For now you can add unit-tests with mocking the OS, there is no way to test properly until GitHub adds Windows 11 runner.

It turns out that there's no reliable way to spyOn an ES module (os), see microsoft/TypeScript#43081.

Give up after literally trying all possible "safe" solutions.

soumyamahunt · 2023-09-26T18:06:51Z

For now you can add unit-tests with mocking the OS, there is no way to test properly until GitHub adds Windows 11 runner.

It turns out that there's no reliable way to spyOn an ES module (os), see microsoft/TypeScript#43081.

Give up after literally trying all possible "safe" solutions.

Not sure what you have tried, there are already tests that mock os, please refer here for example.

Also please add windows-2019 to integration test, let's see if changes impact older versions:

setup-swift/.github/workflows/main.yml

Line 236 in 5dfc518

os: [ubuntu-latest, macos-latest, windows-latest]

setup-swift/.github/workflows/main.yml

Lines 249 to 251 in 5dfc518

    
           - os: windows-latest 
        
             swift: '5.3' 
        
             development: false

stevapple · 2023-09-26T18:35:44Z

Not sure what you have tried, there are already tests that mock os, please refer here for example.

Mocking os in this way requires changing import * as os from "os" into import os from "os", which fundamentally changes os from all exported declarations to the only default one. However it worked, just slightly… uglier, I would say, than before.

soumyamahunt · 2023-09-27T04:39:36Z

.github/workflows/main.yml

@@ -249,6 +249,9 @@ jobs:
        - os: windows-latest
          swift: '5.3'
          development: false
+        - os: windows-2019


please add windows-2019 with latest swift as well

soumyamahunt · 2023-09-27T04:44:06Z

src/installer/windows/index.ts

+    const winsdkMajor = semver.lt(version, '10.0.22000')
+      ? semver.major(version)
+      : 11
+    const winsdkMinor = semver.patch(version)


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

soumyamahunt · 2023-09-27T04:52:27Z

src/installer/windows/index.ts

@@ -11,17 +11,18 @@ import {Installation} from './installation'

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


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.

soumyamahunt · 2024-04-04T12:50:01Z

@stevapple can you rebase this PR, I am thinking of adding this as part of next release.

stevapple · 2024-04-04T13:05:47Z

@stevapple can you rebase this PR, I am thinking of adding this as part of next release.

I may need some more time to look at previous discussions and try to handle these more elegantly. Thanks for getting this back to me😂

stevapple force-pushed the vs-sdk branch from 908afdb to 95a8be2 Compare September 26, 2023 07:35

stevapple added 5 commits September 26, 2023 20:05

feat: support Windows 11 SDK

ffd9b03

feat: update Visual Studio and Windows SDK

5565d3f

fix: fixme for support file setup

f4343a9

fix: run linter

63eac44

fix: update test to use Visual Studio 2022

b48af66

stevapple force-pushed the vs-sdk branch from 95a8be2 to b48af66 Compare September 26, 2023 12:05

fix: Windows version judgement was accidentally inverted

1d3c38f

stevapple force-pushed the vs-sdk branch from 5b56e50 to 1d3c38f Compare September 26, 2023 12:21

soumyamahunt reviewed Sep 26, 2023

View reviewed changes

stevapple added 2 commits September 27, 2023 01:01

fix: revert some Visual Studio requirement changes

e2b56f4

build: update dist/index.js

d1e443e

stevapple force-pushed the vs-sdk branch from cd0d36c to d1e443e Compare September 26, 2023 17:01

stevapple changed the title ~~feat: add support for newer Visual Studio and Windows SDKs~~ feat: add support for newer Windows SDKs Sep 26, 2023

stevapple added 2 commits September 27, 2023 02:28

test: mock Windows 10 and 11 for SDK resolution

4ea245a

chore: integration test for Windows Server 2019

1ceff75

soumyamahunt reviewed Sep 27, 2023

View reviewed changes

stevapple mentioned this pull request Sep 27, 2023

feat: expand SWIFTFLAGS with full paths #74

Merged

soumyamahunt mentioned this pull request Nov 8, 2024

feat: support Windows 11 SDK #334

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for newer Windows SDKs #72

feat: add support for newer Windows SDKs #72

stevapple commented Sep 26, 2023 •

edited

Loading

codecov bot commented Sep 26, 2023 •

edited

Loading

stevapple commented Sep 26, 2023 •

edited

Loading

stevapple commented Sep 26, 2023

stevapple commented Sep 26, 2023

soumyamahunt commented Sep 26, 2023

soumyamahunt Sep 26, 2023

stevapple Sep 26, 2023 •

edited

Loading

soumyamahunt Sep 27, 2023

soumyamahunt Sep 26, 2023

stevapple Sep 26, 2023 •

edited

Loading

soumyamahunt Sep 26, 2023

stevapple Sep 26, 2023

stevapple commented Sep 26, 2023

soumyamahunt commented Sep 26, 2023

stevapple commented Sep 26, 2023 •

edited

Loading

soumyamahunt commented Sep 26, 2023

stevapple commented Sep 26, 2023

soumyamahunt Sep 27, 2023

soumyamahunt Sep 27, 2023

soumyamahunt Sep 27, 2023

soumyamahunt commented Apr 4, 2024

stevapple commented Apr 4, 2024

feat: add support for newer Windows SDKs #72

Are you sure you want to change the base?

feat: add support for newer Windows SDKs #72

Conversation

stevapple commented Sep 26, 2023 • edited Loading

codecov bot commented Sep 26, 2023 • edited Loading

Codecov Report

stevapple commented Sep 26, 2023 • edited Loading

stevapple commented Sep 26, 2023

stevapple commented Sep 26, 2023

soumyamahunt commented Sep 26, 2023

Choose a reason for hiding this comment

stevapple Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevapple Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevapple commented Sep 26, 2023

soumyamahunt commented Sep 26, 2023

stevapple commented Sep 26, 2023 • edited Loading

soumyamahunt commented Sep 26, 2023

stevapple commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyamahunt commented Apr 4, 2024

stevapple commented Apr 4, 2024

stevapple commented Sep 26, 2023 •

edited

Loading

codecov bot commented Sep 26, 2023 •

edited

Loading

stevapple commented Sep 26, 2023 •

edited

Loading

stevapple Sep 26, 2023 •

edited

Loading

stevapple Sep 26, 2023 •

edited

Loading

stevapple commented Sep 26, 2023 •

edited

Loading