Windows 10 SDK install tests #153

mokagio · 2025-02-19T03:59:09Z

Builds on top of #144. Initially, this was a dedicated PR because I expected a lot of Git noise by trial and error in CI. But as I kept refining the implementation for the Windows 10 SDK version file validation, I also introduced what I hope are other improvements to the Windows scripts.

Adds validation of the .windows-10-sdk-version content before attempting to use said content as the version to install, including comparing against an allow-list of valid versions
Replaces all Write-Host with Write-Output, because Write-Host is meant for messages such as user prompts
Adds a -DryRun option to the Windows 10 SDK install script, mostly so it can be tested without doing a full install
Adds a SkipWindows10SDKInstallation to the host setup script, just in case

Together with #144, I think this should be enough for now to ship a new plugin (major) version and begin DRYing the Electron apps.

Next steps

Take a look at the CI setup time and consider moving the tests into a single script if the overhead is considerable
Keep working on R&D - Error handling from Bash and PS1 in Buildkite/Windows #133 to make sure the script properly handle errors

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

dangermattic · 2025-02-19T03:59:33Z

	1 Warning
⚠️	Please add an entry in the `CHANGELOG.md` file to describe the changes made by this PR

Generated by 🚫 Danger

bin/install_windows_10_sdk.ps1

AliSoftware · 2025-02-19T10:34:16Z

bin/install_windows_10_sdk.ps1

+
+if ($windows10SDKVersion -notmatch '^\d+$') {
+  Write-Output "[!] Invalid version file format."
+  Write-Output "Expected an integer, got: '$windows10SDKVersion'"


I'm not well versed in how Windows10 SDK versioning works, so completely open question: are we sure it's guaranteed to be an integer? As opposed to versions like 13b4 or 12.7 or 123a also being a possibility?

As far as I understand, yes. That's because the integer used in the file is then passed to the SDK downloader and used to identify the version according to the spec in the screenshot below, sourced from here:

Having said that, when I tried 18362, nothing happened:

I wonder whether that's because the version is 10.0.18362.1, there's something wrong with that version, or my command.

For the record, I tried passing 18362.1 and it still did not result in the SDK being installed.

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 like the version file to enforce a specific version instead of always installing the latest though, especially for build reproducibility and idempotence (ie similar to a lockfile ensuring every rerun of a build on a same commits days later will still use the same libs and tools)

Ah, good point 👍

Given the timezone schedule that basically leaves me with a whole workday before the next round of feedback, I'll work on adding an allow-list for the valid versions. Otherwise, I would have suggested to merge this as is and ship the allow-list as a followup improvement.

Tested it by running on my Windows VM first this time.

If one tries to change the position, the script will fail

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

`Write-Host` is for user interaction.

mokagio · 2025-02-24T03:14:26Z

bin/install_windows_10_sdk.ps1

+# The expected .windows-10-sdk-version format is a integer representing a valid SDK component id.
+# The list of valid component ids can be found at
+# https://learn.microsoft.com/en-us/visualstudio/install/workload-component-id-vs-build-tools?view=vs-2022
+#
+# Example:
+#
+#   20348
+


See #144 (comment) by @iangmaia

I added the documentation here rather than in the prepare script as per @iangmaia 's comment because it seemed like a better place where to explain the expectation.

Also, I am aware we have two PRs that add docs here, but it felt best to add a manual version in this context to get it over the line while waiting for the LLM-generated doc approach to be discussed, see #159 #160

mokagio · 2025-02-24T03:15:53Z

@iangmaia @AliSoftware I'd appreciate a second (third, fourth?) look after the latest followup, so ideally we can merge in #144 and keep going.

Thanks! 🙇‍♂️

AliSoftware · 2025-02-24T13:54:59Z

.buildkite/pipeline.yml

@@ -118,3 +118,98 @@ steps:
        notify:
          - github_commit_status:
              context: "pr_changed_files Tests: Edge Cases"
+
+  - group: ":windows: install_windows_10_sdk Tests"


What about making use of our new pr_changed_files to run this group conditionally?

The nice thing is that this a8c-ci-toolkit repo is the only one for now where it'd actually be easy to use that helper in shared-pipeline-vars to set up an env var you could then use as an if: build.env('HAS_INSTALL_WINDOWS_10_SDK_CHANGES') == 1.

This is because unlike all other repos for which the a8c-ci-toolkit is not loaded on the uploader agent when the main step that does pipeline upload is run on it, our default steps don't include it as a plugin… in this specific repo we don't even need to load it as a plugin because it's already a binary in the working copy of the repo it's trying to run CI on 😛

Which means you should be able to use bin/pr_change_files in this repo directly (instead of having the plugin loaded and having its bin/ folder added to the $PATH) to export HAS_INSTALL_WINDOWS_10_SDK_CHANGES=$(bin/pr_changed_files --stdout --any-match './bin/install_windows_10_sdk.ps1' './tests/test-install-windows-sdk.ps1') 😉

Same for the other one btw.

That would be neat and great dogfooding.

AliSoftware · 2025-02-24T13:56:22Z

.buildkite/pipeline.yml

+      - label: ":windows: install_windows_10_sdk Tests - Version file with valid format and version"
+        command: |
+          "20348" | Out-File .windows-10-sdk-version
+          .\tests\test-install-windows-10-sdk.ps1 -ExpectedExitCode 0


I just realized the inconsistency we have here between snake_case used for bin/* scripts and kebab-case used for tests/* 🤔

I noticed that too, but didn't take the time to write it down or address it. Sounds like an easy follow up PR for me to tackle.

AliSoftware · 2025-02-24T14:01:14Z

bin/prepare_windows_host_for_app_distribution.ps1

@@ -8,24 +8,28 @@
 #  - Install the Windows 10 SDK if it detected a `.windows-10-sdk-version` file(2)
 #
 # (1) The certificate it installs is stored in our AWS SecretsManager storage (`windows-code-signing-certificate` secret ID)
-# (2) You can skip the Win10 install even if `.windows-10-sdk-version` file is present by using the `SKIP_WINDOWS_10_SDK_INSTALL=1` env var before calling this script
+# (2) You can skip the Windows 10 SDK installation regardless of whether `.windows-10-sdk-version` is present by calling the script with `-SkipWindows10SDKInstallation`.


mokagio changed the base branch from mokagio/windows-utils to mokagio/more-windows-utils February 19, 2025 03:59

mokagio force-pushed the mokagio/windows-10-experiments branch 2 times, most recently from 67f168e to 4701b45 Compare February 19, 2025 05:11

mokagio changed the title ~~Print PWD to help debugging in prepare_windows_host_for_node.ps1~~ Windows 10 SDK install tests Feb 19, 2025

mokagio mentioned this pull request Feb 19, 2025

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

Open

1 task

AliSoftware reviewed Feb 19, 2025

View reviewed changes

mokagio marked this pull request as ready for review February 20, 2025 02:49

mokagio requested a review from AliSoftware February 20, 2025 02:49

mokagio and others added 21 commits February 21, 2025 12:01

Add tests for install Windows 10 SDK command

30cb3dd

Add logic to handle new lines and non integers to Win 10 SDK

30de013

Add test for Win 10 SDK with string

f4f8a64

Add dry run flag to install Win 10 SDK script

1c308cb

Update win 10 sdk tests to capture and inspect output

4521151

DRY win 10 SDK test logic in test script

8771501

Refined test for Windows 10 SDK

eb9c63d

Tested it by running on my Windows VM first this time.

Restore tests creating .windows-10-sdk where needed

7e305bf

Fix emoji rendering

16af116

Test fix attempt

bc562b6

Use | Out-File over echo > to maintain new lines

135509a

Use Write-Output over Write-Host to allow for capturing

c2892e8

Update tests because the script handles new lines -.-'

73de2d3

Remove statement to prevent script error from failing test early

ef282e0

Use a text check that is okay with new lines

54e9aef

Remove note about param required position in PS1 script

b04ea32

If one tries to change the position, the script will fail

Simplify version file parsing logic in Win 10 SDK install

7764a28

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

Add test for Win 10 version with trailing whitespaces

44ed08c

Add test for Win 10 SDK with leading whitespaces

1748736

Remove unnecessary line length check

b34cac1

Add header doc for install_windows_10_sdk.ps1

34ae304

mokagio added 3 commits February 21, 2025 12:01

Add allow list for Windows versions

320d84c

Add parameter to bypass Windows 10 SDK installation in host setup script

12e82ba

Replace all Write-Host with Write-Output

8216d5e

`Write-Host` is for user interaction.

mokagio force-pushed the mokagio/windows-10-experiments branch from b78effb to 8216d5e Compare February 21, 2025 01:07

Add test for skip windows 10 installation behavior

31db443

mokagio force-pushed the mokagio/windows-10-experiments branch from 02c7ccf to 31db443 Compare February 21, 2025 01:38

mokagio requested a review from iangmaia February 21, 2025 03:54

This was referenced Feb 21, 2025

Add .editorconfig #158

Open

Experiment with Cursor-generated docs for PowerShell scripts #159

Draft

Add valid .windows-10-sdk-version example in doc

de4505e

mokagio commented Feb 24, 2025

View reviewed changes

AliSoftware approved these changes Feb 24, 2025

View reviewed changes

mokagio merged commit 356fe9b into mokagio/more-windows-utils Feb 26, 2025
25 checks passed

mokagio deleted the mokagio/windows-10-experiments branch February 26, 2025 00:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows 10 SDK install tests #153

Windows 10 SDK install tests #153

mokagio commented Feb 19, 2025 •

edited

Loading

dangermattic commented Feb 19, 2025 •

edited

Loading

AliSoftware Feb 19, 2025

mokagio Feb 20, 2025 •

edited

Loading

AliSoftware Feb 20, 2025

mokagio Feb 21, 2025

mokagio Feb 24, 2025

mokagio commented Feb 24, 2025 •

edited

Loading

AliSoftware Feb 24, 2025

mokagio Feb 26, 2025

AliSoftware Feb 24, 2025

mokagio Feb 26, 2025

AliSoftware Feb 24, 2025

Windows 10 SDK install tests #153

Windows 10 SDK install tests #153

Conversation

mokagio commented Feb 19, 2025 • edited Loading

Next steps

dangermattic commented Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

mokagio Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Feb 19, 2025 •

edited

Loading

dangermattic commented Feb 19, 2025 •

edited

Loading

mokagio Feb 20, 2025 •

edited

Loading

mokagio commented Feb 24, 2025 •

edited

Loading