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

m3dwards · 2025-05-15T13:36:46Z

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 · 2025-05-15T13:36:49Z

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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 · 2025-05-15T13:42:21Z

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.

.github/workflows/ci.yml

davidgumberg · 2025-05-16T05:53:01Z

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 · 2025-05-16T10:06:43Z

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 · 2025-05-20T12:30:56Z

Rebased and taken @davidgumberg's great suggestion.

.github/workflows/ci.yml

davidgumberg · 2025-05-23T15:05:24Z

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

.github/workflows/ci.yml

m3dwards · 2025-05-29T11:19:48Z

@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 · 2025-05-29T11:30:17Z

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

I don't mind adding that.

hodlinator · 2025-05-29T11:40:41Z

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 · 2025-05-29T18:28:32Z

.github/workflows/ci.yml

-          "MT_EXE=${sdk_dir}bin\${sdk_latest}\x64\mt.exe" >> $env:GITHUB_ENV
+          $vswherePath = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe"
+          $installationPath = & $vswherePath -latest -property installationPath
+          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.

Should be addressed

m3dwards · 2025-06-02T10:23:05Z

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

Added

davidgumberg · 2025-06-03T08:37:48Z

.github/workflows/ci.yml

+
+            Write-Host "Checking $exeName"
+            & mt.exe -nologo -inputresource:$_.FullName -validate_manifest
+            if ($LASTEXITCODE -ne 0) {


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.

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.

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

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?

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

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

This PR enables failing fast for executables in pwsh:

#32672

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

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.

DrahtBot added the Tests label May 15, 2025

fanquake requested a review from hebasto May 15, 2025 13:38

fanquake added the Windows label May 15, 2025

hebasto approved these changes May 15, 2025

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

.github/workflows/ci.yml Outdated Show resolved Hide resolved

m3dwards force-pushed the 250515-remove-js-ci branch from 2549a10 to 449cb9e Compare May 15, 2025 21:31

davidgumberg mentioned this pull request May 15, 2025

cmake: Add application manifests when cross-compiling for Windows #32396

Merged

m3dwards force-pushed the 250515-remove-js-ci branch from 449cb9e to dc403f0 Compare May 20, 2025 11:56

hebasto reviewed May 20, 2025

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

davidgumberg reviewed May 23, 2025

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

m3dwards force-pushed the 250515-remove-js-ci branch from 5bd3a34 to ff2e35f Compare May 24, 2025 12:02

hebasto approved these changes May 29, 2025

View reviewed changes

davidgumberg reviewed May 29, 2025

View reviewed changes

m3dwards force-pushed the 250515-remove-js-ci branch from ff2e35f to a453e66 Compare May 30, 2025 17:38

bitcoin deleted a comment from Dakota48423 Jun 2, 2025

davidgumberg reviewed Jun 3, 2025

View reviewed changes

m3dwards and others added 3 commits June 3, 2025 14:17

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

0402aeb

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>

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

908c27c

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>

ci: Check windows manifests for all executables

7f8393a

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.

m3dwards force-pushed the 250515-remove-js-ci branch from a453e66 to 7f8393a Compare June 3, 2025 12:46

DrahtBot mentioned this pull request Jun 3, 2025

ci: update pwsh to use custom shell that fails-fast #32672

Draft

DrahtBot mentioned this pull request Aug 1, 2025

refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags #17580

Draft

	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

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

Are you sure you want to change the base?

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

Conversation

m3dwards commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

hebasto commented May 15, 2025

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidgumberg commented May 16, 2025

Uh oh!

hebasto commented May 16, 2025

Uh oh!

m3dwards commented May 20, 2025

Uh oh!

Uh oh!

davidgumberg commented May 23, 2025

Uh oh!

Uh oh!

m3dwards commented May 29, 2025

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

m3dwards commented May 29, 2025

Uh oh!

hodlinator commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m3dwards commented Jun 2, 2025

Uh oh!

davidgumberg Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m3dwards Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

m3dwards commented May 15, 2025 •

edited

Loading

DrahtBot commented May 15, 2025 •

edited

Loading

davidgumberg Jun 3, 2025 •

edited

Loading

m3dwards Jun 3, 2025 •

edited

Loading