ci: remove 3rd party js from windows dll gha job

m3dwards commented at 1:36 pm on May 15, 2025: contributor

We only need to find MSBuild.exe for the job to function and can do that ourselves using vswhere.exe and so can remove the dependency on ilammy/msvc-dev-cmd.

Fixes: #32508

DrahtBot commented at 1:36 pm on May 15, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32513.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on May 15, 2025

fanquake requested review from hebasto on May 15, 2025

fanquake added the label Windows on May 15, 2025

hebasto commented at 1:42 pm on May 15, 2025: member

Concept ACK.

Thank you for taking this!

in .github/workflows/ci.yml:194 in 2549a10755 outdated

194+        shell: pwsh
195+        run: |
196+          $vswherePath = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe"
197+          $msbuildPath = & "$vswherePath" -latest -requires Microsoft.Component.MSBuild -find MSBuild\**\Bin\MSBuild.exe
198+          if (-not $msbuildPath) {
199+            throw "MSBuild not found"

hebasto commented at 2:50 pm on May 15, 2025:

Why use an exception? Wouldn’t exiting with an error code suffice?

m3dwards commented at 4:13 pm on May 15, 2025:

I spose I’d ask the question the other way round, why not the exception? It would be an exceptional circumstance if MSBuild could not be found.

hebasto commented at 4:20 pm on May 15, 2025:

I mean, did GHA document handling exceptions in PowerShell scripts?

m3dwards commented at 4:45 pm on May 15, 2025:

No, this doc just says use any exit code other than 0 (which throw does do anyway).

I’ve tested throw works here: https://github.com/m3dwards/bitcoin/actions/runs/15050358923/job/42303402427

All that said, I’m really not wedded to it and happy to change to a simple exit 1 if you prefer, throw just felt more powershelly.

hebasto commented at 5:04 pm on May 15, 2025:

I have no preferences either.

in .github/workflows/ci.yml:198 in 2549a10755 outdated

198+          if (-not $msbuildPath) {
199+            throw "MSBuild not found"
200+          }
201+          $msbuildDir = Split-Path $msbuildPath -Parent
202+          Write-Host "MSBuild found at $msbuildPath"
203+          "$msbuildDir" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append

hebasto commented at 2:51 pm on May 15, 2025:

GHA docs do not suggest -Encoding utf8. Is it necessary?

m3dwards commented at 4:13 pm on May 15, 2025:

Will try without.

m3dwards commented at 9:32 pm on May 15, 2025:

Worked, force pushed 449cb9e

hebasto approved

hebasto commented at 2:56 pm on May 15, 2025: member

ACK 2549a1075521202ee0d9fe069ba1062563ee6387, tested locally on Windows 11. Additionally tested in my personal repo with invalidated vcpkg binary cache.

m3dwards force-pushed on May 15, 2025

davidgumberg commented at 5:53 am on May 16, 2025: contributor

It might be better to use a more general solution for setting up VS prompts, this would also work once #32396 is merged.

 0      - name: Set up VS Developer Prompt
 1        shell: pwsh
 2        run: |
 3          $vswherePath = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe"
 4          $installationPath = & 'C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe' -latest -property installationPath
 5          if ($installationPath -and (test-path "$installationPath\Common7\Tools\vsdevcmd.bat")) {
 6            & "${env:COMSPEC}" /s /c "`"$installationPath\Common7\Tools\vsdevcmd.bat`" -no_logo && set" | foreach-object {
 7              $name, $value = $_ -split '=', 2
 8              echo "$name=$value" >> $env:GITHUB_ENV
 9            }
10          }

(https://github.com/davidgumberg/bitcoin/commit/83c53bfb58bf5451dccee753ace1c7464ea4ac5f)

This snippet was mostly taken from this vswhere wiki article: https://github.com/microsoft/vswhere/wiki/Start-Developer-Command-Prompt#using-powershell

hebasto commented at 10:06 am on May 16, 2025: member

It might be better to use a more general solution for setting up VS prompts, this would also work once #32396 is merged. @m3dwards

Could you please rebase this PR on top of #32396?

m3dwards force-pushed on May 20, 2025

m3dwards commented at 12:30 pm on May 20, 2025: contributor

Rebased and taken @davidgumberg’s great suggestion.

in .github/workflows/ci.yml:196 in dc403f0c13 outdated

196+          $vswherePath = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe"
197+          $installationPath = & 'C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe' -latest -property installationPath
198+          if ($installationPath -and (test-path "$installationPath\Common7\Tools\vsdevcmd.bat")) {
199+            & "${env:COMSPEC}" /s /c "`"$installationPath\Common7\Tools\vsdevcmd.bat`" -no_logo && set" | foreach-object {
200+              $name, $value = $_ -split '=', 2
201+              echo "$name=$value" >> $env:GITHUB_ENV

hebasto commented at 1:30 pm on May 20, 2025:

This part seems too invasive to me. Please compare the list of environment variables in the “Get tool information” step on the master branch with those in this PR.

m3dwards commented at 3:42 pm on May 20, 2025:

The only tool we need is msbuild.exe which is what my original version did? Shall I just put it back?

hebasto commented at 3:49 pm on May 20, 2025:

The only tool we need is msbuild.exe…

We also use now mt.exe: https://github.com/bitcoin/bitcoin/blob/dc403f0c13e1ceee3d4651b4056ff2bad47d0315/.github/workflows/ci.yml#L246-L253

hebasto commented at 4:14 pm on May 20, 2025:

Shall I just put it back?

Perhaps updating only the Path variable will be sufficient?

hebasto commented at 11:17 am on May 22, 2025:

The only tool we need is msbuild.exe which is what my original version did?

And we do use it—along with the VCToolsVersion environment variable—but only to determine whether the vcpkg binary cache needs to be invalidated. CMake is smart enough to inspect the build environment and locate the required tools on its own.

We might be able to use other indicators to invalidate the cache at the right time. Perhaps the top commit hash of vcpkg?

m3dwards commented at 12:02 pm on May 22, 2025:

@hebasto thanks, I see. I will look at using the top commit hash for cache invalidation. If I’ve understood you correct the current cache key is:

hashFiles('cmake_version', 'msbuild_version', 'toolset_version', 'vcpkg.json')

we think this will be sufficient:

hashFiles('vcpkg_commit_hash', 'vcpkg.json')

Which then means we can drop the vswhere tool finding.

hebasto commented at 12:06 pm on May 22, 2025:

I will look at using the top commit hash for cache invalidation. If I’ve understood you correct the current cache key is:

hashFiles('cmake_version', 'msbuild_version', 'toolset_version', 'vcpkg.json')

we think this will be sufficient:

hashFiles('vcpkg_commit_hash', 'vcpkg.json')

Yes, I thin so.

Which then means we can drop the vswhere tool finding.

Right. However, we still need to locate mt.exe.

UPD. Only a path can be added though:

0"${sdk_dir}bin\${sdk_latest}\x64" | Out-File -FilePath "$env:GITHUB_PATH" -Append

davidgumberg commented at 3:50 pm on May 22, 2025:

I think this approach is no less invasive then what is done currently using an external script, setting up an environment that behaves the same as the VS Powershell utility. The advantage of this approach is that it makes use of Visual Studio utilities and commands zero maintenance cost, manually setting variables voids that, and we are subject to being broken by MS upstream, or wrestling with strange misconfigurations when some command that works on a local VS powershell environment is broken on CI because some environment variable is not set.

hebasto commented at 3:54 pm on May 22, 2025:

The advantage of this approach is that it makes use of Visual Studio utilities and commands zero maintenance cost, manually setting variables voids that, and we are subject to being broken by MS upstream, or wrestling with strange misconfigurations when some command that works on a local VS powershell environment is broken on CI because some environment variable is not set.

That sounds compelling.

m3dwards commented at 1:37 pm on May 23, 2025:

@hebasto compelling enough to ACK as is?

davidgumberg commented at 3:05 pm on May 23, 2025: contributor

This approach here should also be applied to replace the Find mt.exe tool step in the Windows, test cross-built job.

in .github/workflows/ci.yml:369 in 5bd3a34430 outdated

368         run: |
369-          $sdk_dir = (Get-ItemProperty 'HKLM:\SOFTWARE\Wow6432Node\Microsoft\Windows Kits\Installed Roots' -Name KitsRoot10).KitsRoot10
370-          $sdk_latest = (Get-ChildItem "$sdk_dir\bin" -Directory | Where-Object { $_.Name -match '^\d+\.\d+\.\d+\.\d+$' } | Sort-Object Name -Descending | Select-Object -First 1).Name
371-          "MT_EXE=${sdk_dir}bin\${sdk_latest}\x64\mt.exe" >> $env:GITHUB_ENV
372+          $vswherePath = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe"
373+          $installationPath = & 'C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe' -latest -property installationPath

davidgumberg commented at 6:37 pm on May 23, 2025:

nit for here and above:

0          $installationPath = & $vswherePath -latest -property installationPath

or could drop the line where $vswherePath is defined, whatever your preference is.

m3dwards commented at 12:56 pm on May 24, 2025:

Done, thanks for the spot!

ci: remove 3rd party js from windows dll gha job

We can use vswhere.exe directly to create a vs developer
prompt and so can remove this third party dependency.

Co-authored-by: David Gumberg <davidzgumberg@gmail.com>

ffd25e7191

ci: use a more generic way of finding mt.exe

This sets up a vs developer command prompt and should hopefully should
be more resilient to upstream changes

Co-authored-by: David Gumberg <davidzgumberg@gmail.com>

ff2e35f631

m3dwards force-pushed on May 24, 2025

ci: remove 3rd party js from windows dll gha job #32513

Code Coverage & Benchmarks

Reviews