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

[m3dwards]

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

[DrahtBot]

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
ACK	davidgumberg, hebasto

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #33562 (DRAFT: add a freebsd job using systemlibs by willcl-ark)
• #32672 (ci: update pwsh to use custom shell that fails-fast by m3dwards)

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.

[hebasto]

Concept ACK.

Thank you for taking this!

[hebasto]

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

[davidgumberg]

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

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

(davidgumberg@83c53bf)

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

[hebasto]

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]

Rebased and taken @davidgumberg's great suggestion.

[hebasto]

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]

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

[hebasto]

The only tool we need is msbuild.exe...

We also use now mt.exe:

bitcoin/.github/workflows/ci.yml

Lines 246 to 253 in dc403f0

	- name: Get bitcoind manifest
	if: matrix.job-type == 'standard'
	working-directory: build
	run: |
	mt.exe -nologo -inputresource:bin/Release/bitcoind.exe -out:bitcoind.manifest
	cat bitcoind.manifest
	echo
	mt.exe -nologo -inputresource:bin/Release/bitcoind.exe -validate_manifest

[hebasto]

Shall I just put it back?

Perhaps updating only the Path variable will be sufficient?

[hebasto]

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]

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 think 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:

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

[davidgumberg]

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]

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]

@hebasto compelling enough to ACK as is?

[hebasto]

@davidgumberg

I think this approach is no less invasive then what is done currently using an external script...

I disagree. I've checked c44fb4f. The env: sections in the CI logs are indeed more bloated compared to the master branch. I'd prefer to avoid this.

[davidgumberg]

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

[m3dwards]

@hebasto @davidgumberg I believe all comments have been addressed?

[hebasto]

ACK ff2e35f.

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.

[m3dwards]

While touching this code, it seems reasonable to also address the other @davidgumberg's comment:

I don't mind adding that.

[hodlinator]

I suspect checking for manifests will currently fail for bench_bitcoin.exe as it doesn't yet have a manifest. Not sure about other binaries. Wouldn't be against adding missing manifests unless it slows down the builds noticeably.

[davidgumberg]

(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.

[m3dwards]

Don't worry, I'm reworking it to do more manifest checks anyway so changes at this point are fine.

Will look at this.

[m3dwards]

Should be addressed

[m3dwards]

While touching this code, it seems reasonable to also address the other @davidgumberg's comment:

Added

[davidgumberg]

If I'm not mistaken, this if branch will never be reached since the CI script will exit if the line above has a non-zero exit code.

[m3dwards]

Default behaviour in powershell is to continue on error (unless it's a terminating error like throw). When calling a powershell cmdlet you can set $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.

[m3dwards]

Which now worries me about elsewhere we have used powershell, if it will exit like we think it should.

[fanquake]

Default behaviour in powershell is to continue on error

Do we have that turned off for all of our CIs?

[m3dwards]

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.

[m3dwards]

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.

[hebasto]

Default behaviour in powershell is to continue on error

Do we have that turned off for all of our CIs?

We did:

bitcoin/.github/workflows/ci.yml

Lines 24 to 28 in e872a56

	defaults:
	run:
	# Enforce fail-fast behavior for all platforms.
	# See: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference
	shell: bash

[m3dwards]

@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: 4bf1031

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.

[m3dwards]

This PR enables failing fast for executables in pwsh:

#32672

[m3dwards]

@davidgumberg force pushed a change so that it behaves how you expected it to: https://github.com/bitcoin/bitcoin/compare/a453e66c827f260da597bf70273bcd0da2313774..7f8393ad745e6bc44dc9481428ea5979ed31a3c9

[hebasto]

@m3dwards

Could you please rebase this PR to refresh the CI?

[fanquake]

Backported to 30.x in #34283.

[fanquake]

Backported to 29.x in #34446.

[fanquake]

Backported to 28.x in #34463.