The windows job uses the external dependency ilammy/msvc-dev-cmd which runs javascript. We use this to put various tools on the path such as MSBuild.exe and mt.exe. We can remove this dependency and use vswhere.exe directly to find these tools and create a “Developer command prompt” as someone would on their dev machine.
While in this area of the code, this PR also runs some additional manifest checks on the windows binaries.
Fixes: #32508
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32513.
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.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Concept ACK.
Thank you for taking this!
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"
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.
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
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
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
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
Shall I just put it back?
Perhaps updating only the Path variable will be sufficient?
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?
@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.
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
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.
Find mt.exe tool step in the Windows, test cross-built job.
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
nit for here and above:
0 $installationPath = & $vswherePath -latest -property installationPath
or could drop the line where $vswherePath is defined, whatever your preference is.
ACK ff2e35f6315cc6b912f68fe85103575d751bd2bc.
While touching this code, it seems reasonable to also address the other @davidgumberg’s comment:
Might be nice to have a CI check like davidgumberg@1b98160 to prevent future regressions, but OK for a separate PR if out of scope.
While touching this code, it seems reasonable to also address the other @davidgumberg’s comment:
I don’t mind adding that.
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 = & $vswherePath -latest -property installationPath
374+ if ($installationPath -and (test-path "$installationPath\Common7\Tools\vsdevcmd.bat")) {
(Sorry for the churn here)
I think we actually want to drop this if guard here and above, the CI job should fail right here if the path to vsdevcmd.bat breaks, so that the cause/fix is obvious.
Don’t worry, I’m reworking it to do more manifest checks anyway so changes at this point are fine.
Will look at this.
While touching this code, it seems reasonable to also address the other @davidgumberg’s comment:
Added
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>
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>
The other executables have manifests and these should be checked in
addition to bitcoind. Skipping fuzz.exe, bench_bitcoin.exe and
test_bitcoin-qt.exe as they do not have manifests.
403+ return
404+ }
405+
406+ Write-Host "Checking $exeName"
407+ & mt.exe -nologo -inputresource:$_.FullName -validate_manifest
408+ if ($LASTEXITCODE -ne 0) {
if branch will never be reached since the CI script will exit if the line above has a non-zero exit code.
$ErrorActionPreference = 'Stop' but being as this is a call to an external process I don’t believe that approach works here, hence why checking the exit code.
Default behaviour in powershell is to continue on error
Do we have that turned off for all of our CIs?
Doesn’t appear to be turned off on our CI and I think it should be.
In Powershell 7.4 and up there is now a setting PSNativeCommandUseErrorActionPreference which should make calling native commands respect the ErrorActionPreference so we should set both of those, globally if possible. I will look into it.
I’m wrong about powershell cmdlets: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference
Github sets ErrorActionPreference to stop but it doesn’t appear that they set PSNativeCommandUseErrorActionPreference which is why calling native executables (like mt.exe) can still fail silently.
Default behaviour in powershell is to continue on error
Do we have that turned off for all of our CIs?
@hebasto but there are a few places where we use pwsh, which will still fail fast if using exclusively cmdlets but doesn’t appear to fail when calling native executables.
Here is a commit to demonstrate it won’t always failfast: https://github.com/bitcoin/bitcoin/commit/4bf10310fb4ec4d6c2f479f008c487cca1f90c27
and the job run that didn’t fail: https://github.com/m3dwards/bitcoin/actions/runs/15415032095/job/43375709475
Suggest we set PSNativeCommandUseErrorActionPreference to true everywhere we use pwsh.
This PR enables failing fast for executables in pwsh: