Windows: Install Win 10 SDK, clearer name, no Node.js setup #144

mokagio · 2025-02-04T01:17:37Z

Tested via Automattic/studio#882 and https://github.com/beeper/beeper-desktop-new/pull/1970

After realizing that nvm-buildkite-plugin does a better job at setting up Node.js and the custom logic we had in the prepare_windows_host_for_node.ps1 script, I remove said logic from the script
Also renamed the script to prepare_windows_host_for_app_distribution.ps1 which I find better explains the script purpose. Besides, now that it no longer sets up Node.js, the name would have been inappropriate.
New command to install Windows 10 SDK, reading from the .windows-10-sdk-version file in the repo — Extracted from Studio and currently duplicated in other apps
prepare_windows_host_for_app_distribution.ps1 automatically runs the Windows 10 install script if needed, so clients don't have to call two commands

I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

Clients should use the dedicated nvm plugin. See https://github.com/Automattic/nvm-buildkite-plugin

CHANGELOG.md

mokagio · 2025-02-17T05:24:09Z

bin/prepare_windows_host_for_app_distribution.ps1

+Write-Host "--- :windows: Checking whether to install Windows 10 SDK..."
+
+# When using Electron Forge and electron2appx, building Appx requires the Windows 10 SDK
+#
+# See https://github.com/hermit99/electron-windows-store/tree/v2.1.2?tab=readme-ov-file#usage
+
+$windowsSDKVersionFile = ".windows-10-sdk-version"
+if (Test-Path $windowsSDKVersionFile) {
+    Write-Host "Found $windowsSDKVersionFile file, installing Windows 10 SDK..."
+    & "$PSScriptRoot\install_windows_10_sdk.ps1"
+    If ($LastExitCode -ne 0) { Exit $LastExitCode }
+} else {
+    Write-Host "No $windowsSDKVersionFile file found, skipping Windows 10 SDK installation."
+}


Example of this finding the SDK file

Example of this not finding the SDK file

One thing that I'm unsure of is whether we should provide a way to bypass or modify this behavior.

Example:

Something might be broken in the plugin and a client might need to install the SDK manually; or

For whichever reason, it's not possible to use the default file name for the SDK version

I don't think we need to stress much about implementing this flexibility from the get go, so if you agree I'd leave it as a followup, but the code feels incomplete or suboptimal without it.

Nice!

I wouldn't mind adding an escape hatch, that might be a good idea indeed (though one could argue that'd be YAGNI…)

If it's easy enough to implement and doesn't complicate the script logic too much (which I wouldn't expect to), I'd say go for it.

bin/prepare_windows_host_for_node.ps1

Ported from beeper/beeper-desktop-new#1970

bin/path_aware_refreshenv.ps1

bin/prepare_windows_host_for_node.ps1

AliSoftware · 2025-02-18T12:09:28Z

bin/prepare_windows_host_for_app_distribution.ps1

+Write-Host "--- :windows: Checking whether to install Windows 10 SDK..."
+
+# When using Electron Forge and electron2appx, building Appx requires the Windows 10 SDK
+#
+# See https://github.com/hermit99/electron-windows-store/tree/v2.1.2?tab=readme-ov-file#usage
+
+$windowsSDKVersionFile = ".windows-10-sdk-version"
+if (Test-Path $windowsSDKVersionFile) {
+    Write-Host "Found $windowsSDKVersionFile file, installing Windows 10 SDK..."
+    & "$PSScriptRoot\install_windows_10_sdk.ps1"
+    If ($LastExitCode -ne 0) { Exit $LastExitCode }
+} else {
+    Write-Host "No $windowsSDKVersionFile file found, skipping Windows 10 SDK installation."
+}


Nice!

I wouldn't mind adding an escape hatch, that might be a good idea indeed (though one could argue that'd be YAGNI…)

If it's easy enough to implement and doesn't complicate the script logic too much (which I wouldn't expect to), I'd say go for it.

bin/prepare_windows_host_for_app_distribution.ps1

AliSoftware · 2025-02-18T12:29:40Z

bin/install_windows_10_sdk.ps1

+# Install the Windows SDK and other required components
+Write-Output "~~~ Installing Visual Studio Build Tools..."
+Start-Process `
+  -FilePath $buildToolsPath `
+  -ArgumentList "--quiet --wait --add Microsoft.VisualStudio.Component.Windows10SDK.$windows10SDKVersion" `
+  -NoNewWindow `
+  -Wait
+
+# Check if the installation was successful in file system
+$windowsSDKsRoot = "C:\Program Files (x86)\Windows Kits\10\bin"
+$sdkPath = "$windowsSDKsRoot\10.0.$windows10SDKVersion.0\x64"
+If (-not (Test-Path $sdkPath)) {
+  Write-Output "[!] Failed to install Windows 10 SDK: Could not find SDK at $sdkPath."
+  If (-not (Test-Path $windowsSDKsRoot)) {
+    Write-Output "[!] Expected $windowsSDKsRoot to exist, but it does not."
+  } else {
+    Write-Output "    Found:"
+    Get-ChildItem -Path $windowsSDKsRoot | ForEach-Object { Write-Output "    - $windowsSDKsRoot\$_" }
+  }
+  Exit 1
+}


Is there an easy way to check if $windows10SDKVersion is valid (i.e. an existing version, as opposed to if someone tried to use latest or none or whatever invalid text in the version file) and detect it early?

In particular, I'm wondering if the vs_buildtools.exe would fail with an explicit and clear error if that -add … argument we pass to it was invalid (i.e. if it didn't find the component), allowing us to print a specific error message in that case (like "Check the value you set in $windowsSDKVersionFile"), as opposed to the installer happily continuing with the installation of all the other default components—without complaining about this one not existing in the options—and us only finding out the component was not installed after the fact, thanks to your test on lines 42–51… (and even in that case, would it help to provide in the error messages a more explicit suggestion to check the syntax/version used in the version file?)

I'm wondering if the vs_buildtools.exe would fail with an explicit and clear error

I've done a few experiments in my VM. It fails silently, with exit code 1.

I'll duplicated what I wrote in #153 (comment)

This all makes me wonder whether we might be better off using a default SDK version in the installer, and only falling back to the version file and/or a parameter if the user provides one. After all, I implemented the version file because other tools use it and because the app I was working on had a check for the SDK being installed and so I wanted a single source of truth. But I'd argue now that our tooling takes care of the installing, that check is redundant: the build script should trust the SDK is available.

I think the conversation should continue there, given that's the PR that is editing the script.

I also like the idea of having a list of valid numbers to pick from, given it's published at https://learn.microsoft.com/en-us/visualstudio/install/workload-component-id-vs-build-tools?view=vs-2022 but I think that would be a good additional option, while we should still default to installing the latest valid version for the user ourselves.

Maybe if we end up implementing the behavior of defaulting to install the latest valid version, we could allow latest to be a special valid content for .windows-10-sdk-version, that way if the file is not present it won't install the SDK at all, but if it's present with latest it will install the latest (and if it's present with a specific version it will deterministically install that specific version, acting as a lockfile) 🤔

That way the implicit auto-install from prepare_windows_host_for_app_distibution if file is present would still work for that default-version fallback… 🤔

AliSoftware · 2025-02-18T12:32:23Z

bin/install_windows_10_sdk.ps1

+  exit 1
+}
+
+$windows10SDKVersion = Get-Content $windowsSDKVersionFile


Question: how does this Get-Content handles possible newlines?

Especially, if the version file ends with a newline at end of file, is that trimmed?
Or maybe it's not, but that doesn't cause any issues because when you then reference $windows10SDKVersion in other commands like --add Microsoft.VisualStudio.Component.Windows10SDK.$windows10SDKVersion that's when the value is trimmed as usage/substitution site?
What if the file contains 2 newlines at end of file? 🤔

Great question! We clearly need to add something to strip them:

https://buildkite.com/automattic/ci-toolkit-buildkite-plugin/builds/1007#01951c5c-320d-4346-8d45-3e9bda56ad1e

I think the -TotalCount 1 parameter of Get-Content can help?
Though even if that allows to limit only reading the first line of the file, not 100% sure if it'd properly trim the \n at the end of that read line, in that case you might need to also use Trim() or TrimEnd().

Tentative suggestion (not tested):

Suggested change

$windows10SDKVersion = Get-Content $windowsSDKVersionFile

$windows10SDKVersion = (Get-Content -TotalCount 1 $windowsSDKVersionFile).Trim()

Spent more time than I care to admit on this but got a bunch of tests for the version parsing

I'll clean up tomorrow #153

Co-authored-by: Olivier Halligon <[email protected]>

CHANGELOG.md

bin/install_windows_10_sdk.ps1

bin/prepare_windows_host_for_app_distribution.ps1

Co-authored-by: Ian G. Maia <[email protected]>

mokagio · 2025-02-21T00:51:43Z

Thanks @iangmaia for the missing ps1 suggestions.

Incidentally, yesterday I looked into whether it was possible to remove the extension from the scripts to keep them consistent with the other bash ones. As far as I could see, it was not possible short of wrapping them in a cmdlet thus adding two files for each command. I think the "overhead" of adding the .ps1 extension is acceptable in this case.

Co-authored-by: Ian G. Maia <[email protected]>

dangermattic · 2025-02-26T00:51:26Z

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

AliSoftware

Looking good, left some nitpicks but none being blockers

AliSoftware · 2025-02-27T12:26:52Z

.buildkite/pipeline.yml

          - github_commit_status:
              context: "pr_changed_files Tests: Edge Cases"
+
+  - group: ":windows: install_windows_10_sdk Tests"


💡 For future PR:

Suggested change

- group: ":windows: install_windows_10_sdk Tests"

- group: ":windows: install_windows_10_sdk Tests"

if: build.env('INSTALL_WINDOWS_10_SDK_SCRIPT_CHANGED') == 'true'

in shared-pipelines-vars (that we'll need to introduce in this repo, apparently):

export INSTALL_WINDOWS_10_SDK_SCRIPT_CHANGED=$(bin/pr_changed_files --stdout --any-match 'bin/install_windows_10_sdk.ps1' 'tests/test-install-windows-10-sdk.ps1')

Which we'd be able to do in this repo despite the Uploader agent not loading the a8c-ci-toolkit plugin (which is what prevents us from doing that in other repos)… because in the particular case of this repo being the plugin itself, bin/pr_changed_files should be available directly after checkout 😉

(PS: I thought I mentioned that idea somewhere already somewhere but couldn't find it, maybe it was in another PR… so I figured I'd make it appear here too anyway)

bin/install_windows_10_sdk.ps1

AliSoftware · 2025-02-27T12:32:17Z

bin/install_windows_10_sdk.ps1

+# Install the Windows SDK and other required components
+Write-Output "~~~ Installing Visual Studio Build Tools..."
+Start-Process `
+  -FilePath $buildToolsPath `
+  -ArgumentList "--quiet --wait --add Microsoft.VisualStudio.Component.Windows10SDK.$windows10SDKVersion" `
+  -NoNewWindow `
+  -Wait
+
+# Check if the installation was successful in file system
+$windowsSDKsRoot = "C:\Program Files (x86)\Windows Kits\10\bin"
+$sdkPath = "$windowsSDKsRoot\10.0.$windows10SDKVersion.0\x64"
+If (-not (Test-Path $sdkPath)) {
+  Write-Output "[!] Failed to install Windows 10 SDK: Could not find SDK at $sdkPath."
+  If (-not (Test-Path $windowsSDKsRoot)) {
+    Write-Output "[!] Expected $windowsSDKsRoot to exist, but it does not."
+  } else {
+    Write-Output "    Found:"
+    Get-ChildItem -Path $windowsSDKsRoot | ForEach-Object { Write-Output "    - $windowsSDKsRoot\$_" }
+  }
+  Exit 1
+}


Maybe if we end up implementing the behavior of defaulting to install the latest valid version, we could allow latest to be a special valid content for .windows-10-sdk-version, that way if the file is not present it won't install the SDK at all, but if it's present with latest it will install the latest (and if it's present with a specific version it will deterministically install that specific version, acting as a lockfile) 🤔

That way the implicit auto-install from prepare_windows_host_for_app_distibution if file is present would still work for that default-version fallback… 🤔

AliSoftware · 2025-02-27T12:33:52Z

bin/prepare_windows_host_for_app_distribution.ps1

+# Stop script execution when a non-terminating error occurs
+$ErrorActionPreference = "Stop"
+
+Write-Output "--- :windows: Setting up Windows for app distribution"


Nitpick: I wonder if all those Write-Output (used in that script and the others) shouldn't be Write-Host, given they're intended to print informational logs (rather than passing results to a follow-up process down the pipeline) 🤔

Co-authored-by: Olivier Halligon <[email protected]>

mokagio mentioned this pull request Feb 4, 2025

Move script to install Windows 10 SDK to CI toolkit #132

Closed

mokagio added 2 commits February 14, 2025 17:35

Print PWD to help debugging in prepare_windows_host_for_node.ps1

a066096

Print path at which certificate.pfx is saved

ac494f5

mokagio force-pushed the mokagio/more-windows-utils branch from 7d9f5fc to 76bff10 Compare February 14, 2025 06:36

Track command to install Windows 10 SDK

cb3e3ff

mokagio force-pushed the mokagio/more-windows-utils branch from 76bff10 to cb3e3ff Compare February 14, 2025 06:58

mokagio added 3 commits February 14, 2025 18:52

Add entry for command to install Windows 10 SDK to CHANGELOG

51b682b

Remove logic to install Node.js from "prepare" PS1 command

2a3539b

Clients should use the dedicated nvm plugin. See https://github.com/Automattic/nvm-buildkite-plugin

Deprecate prepare_windows_host_for_node in favor of script with new name

d707ff5

mokagio force-pushed the mokagio/more-windows-utils branch from f57b840 to d707ff5 Compare February 17, 2025 03:49

mokagio added 6 commits February 17, 2025 15:23

DRY new PS1 name in deprecated script

a7e8550

Remove a leftover debug log in Windows setup script

8dfadff

Load cert as last action in Windows setup

b3cbf3f

Make app distribution setup script install Windows 10 SDK

abf5498

Update CHANGELOG with more of the Windows changes

2d93f61

DRY certificate.bin and certificate.pfx definitions

41262e8

mokagio commented Feb 17, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mokagio commented Feb 17, 2025

View reviewed changes

bin/prepare_windows_host_for_node.ps1 Outdated Show resolved Hide resolved

mokagio changed the title ~~Add command to install Windows 10 SDK~~ Windows: Install Win 10 SDK, clearer name, no Node.js setup Feb 17, 2025

mokagio added 2 commits February 18, 2025 09:52

Add path_aware_refreshenv.ps1

5bb49fb

Ported from beeper/beeper-desktop-new#1970

Add changelog entry regarding path_aware_refreshenv.ps1

c84c8ca

mokagio requested review from a team, AliSoftware and iangmaia February 17, 2025 23:15

mokagio marked this pull request as ready for review February 17, 2025 23:15

AliSoftware reviewed Feb 18, 2025

View reviewed changes

mokagio and others added 2 commits February 19, 2025 14:37

Completely remove prepare_windows_host_for_node.ps1

2656bfa

Add header doc for prepare_windows_host_for_app_distribution

4561513

Co-authored-by: Olivier Halligon <[email protected]>