ci: Test cross-built Windows executables on Windows natively

hebasto commented at 12:11 pm on October 29, 2024: member

Resolves #31071.

The CI run in my personal repo: https://github.com/hebasto/bitcoin/actions/runs/12140873955.

hebasto added the label Windows on Oct 29, 2024

hebasto added the label Tests on Oct 29, 2024

DrahtBot commented at 12:11 pm on October 29, 2024: 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/31176.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	davidgumberg

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:

#31367 (ci: limit max stack size to 512 KiB by dergoegge)
#31284 (ci: Skip broken Wine64 tests by default by maflcko)
#31158 (build, ci: Fix linking bitcoin-chainstate.exe to bitcoinkernel.dll on Windows by hebasto)
#29881 (guix: use GCC 13 to build releases by fanquake)

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 commented at 12:21 pm on October 29, 2024: member

@achow101 @fanquake

This PR needs the actions/download-artifact@* and actions/upload-artifact@* actions to be allowed in the repository Settings - General - Actions - General.

davidgumberg commented at 11:21 pm on October 30, 2024: contributor

Concept ACK

#31172 @ https://github.com/bitcoin/bitcoin/pull/31172/commits/7dd0ee89a092c6ec4e305fbdb0cf3afa9e41cab6 passes CI but trivially fails to start on any Windows system because Wine lacks some undocumented Windows behavior.

in .github/workflows/ci.yml:275 in cfc624a8a5 outdated

267@@ -268,3 +268,62 @@ jobs:
268           path: ${{ env.CCACHE_DIR }}
269           # https://github.com/actions/cache/blob/main/tips-and-workarounds.md#update-a-cache
270           key: ${{ github.job }}-ccache-${{ github.run_id }}
271+
272+  windows-cross-build:
273+    runs-on: ubuntu-latest
274+    name: Cross-build to Windows
275+    container: debian:bookworm

maflcko commented at 5:12 pm on October 31, 2024:

Forgot to copy the comment?

0    container: debian:bookworm  # Check that https://packages.debian.org/bookworm/g++-mingw-w64-x86-64-posix (version 12.2, similar to guix) can cross-compile

maflcko commented at 5:13 pm on October 31, 2024:

Also, would be good to remove (in the same commit) wine from ci/test/00_setup_env_win64.sh, or at least set RUN_UNIT_TESTS=false.

hebasto commented at 11:54 am on November 26, 2024:

Reworked.

in .github/workflows/ci.yml:284 in cfc624a8a5 outdated

279+        uses: actions/checkout@v4
280+
281+      - name: Install required packages
282+        run: |
283+          apt-get update
284+          apt-get install --no-install-recommends -y build-essential pkg-config curl ca-certificates ccache python3 cmake nsis g++-mingw-w64-x86-64-posix

maflcko commented at 5:22 pm on October 31, 2024:

IIRC the Ubuntu mirrors are often down, so this would lead to intermittent issues without caching.

hebasto commented at 12:07 pm on November 25, 2024:

FWIW, we do not use the installed packages cache in the “test-each-commit” job.

maflcko commented at 12:50 pm on November 25, 2024:

Correct, but it isn’t using the Ubuntu mirror. It is using the GHA mirror. Using a GHA cache to sidestep the GHA mirror seems a no-op, no?

hebasto commented at 11:54 am on November 26, 2024:

Reworked.

in .github/workflows/ci.yml:289 in cfc624a8a5 outdated

284+          apt-get install --no-install-recommends -y build-essential pkg-config curl ca-certificates ccache python3 cmake nsis g++-mingw-w64-x86-64-posix
285+
286+      - name: Build depends
287+        working-directory: depends
288+        run: |
289+          make -j$(nproc) HOST=x86_64-w64-mingw32 LOG=1 NO_QT=1

maflcko commented at 5:23 pm on October 31, 2024:

At least with QT, this may be too slow without caching.

hebasto commented at 11:54 am on November 26, 2024:

The caching has been added.

maflcko commented at 5:26 pm on October 31, 2024: member

Thanks for the POC!

Without caching I wonder how useful this is. However, caching may be the hardest part to get right.

Taking a step back, I wonder what was the last real issue found by Wine, which wasn’t found by the native Windows task? If there are none, or not too many, I think this could also be made a nightly CI task, only run on master after a merge (without caching), or in a completely separate repo. The same is done for other stuff, like the valgrind tasks, or the s390x tasks.

hebasto commented at 12:25 pm on November 1, 2024: member

@maflcko

I’ll address your feedback once new actions are enabled.

maflcko commented at 12:30 pm on November 1, 2024: member

Well, if it is made a nightly task in another repo, it won’t need new actions enabled.

maflcko commented at 11:47 am on November 22, 2024: member

I think this is useful, and while it is waiting for more review (and for the existing review to be addressed), it could make sense to schedule it to run regularly in a repo. For example, in one of your repos, or in a new repo, or in an existing one like https://github.com/maflcko/b-c-nightly/ .

hebasto renamed this:
~~[POC] ci: Test cross-built Windows executables on Windows natively~~
ci: Test cross-built Windows executables on Windows natively
on Nov 25, 2024

hebasto force-pushed on Nov 25, 2024

DrahtBot added the label CI failed on Nov 25, 2024

hebasto force-pushed on Nov 26, 2024

hebasto marked this as ready for review on Nov 26, 2024

hebasto commented at 11:53 am on November 26, 2024: member

Reworked. The GHA job now re-uses ci/test/00_setup_env_win64.sh.

Added caching for depends/built and Ccache.

DrahtBot removed the label CI failed on Nov 26, 2024

in ci/test/00_setup_env_win64.sh:13 in 6e4bd9f57d outdated

11@@ -12,7 +12,8 @@ export HOST=x86_64-w64-mingw32
12 export DPKG_ADD_ARCH="i386"
13 export PACKAGES="nsis g++-mingw-w64-x86-64-posix wine-binfmt wine64 wine32 file"

maflcko commented at 12:42 pm on November 26, 2024:

remove wine?

hebasto commented at 4:26 pm on December 2, 2024:

Thanks! Done.

in .github/workflows/ci.yml:304 in 6e4bd9f57d outdated

299+        run: |
300+          .\bench_bitcoin.exe -sanity-check -priority-level=high
301+
302+      - name: Run tests
303+        run: |
304+          .\test_bitcoin.exe

maflcko commented at 12:43 pm on November 26, 2024:

Would be nice to run the util and functional tests as well

hebasto commented at 4:27 pm on December 2, 2024:

Thanks! Done.

However, a few functional tests fail…

maflcko commented at 7:51 pm on December 2, 2024:

IIRC chmod does not work on Windows, so a workaround may be needed for the test failure.

Not sure about the wallet_migration.py failure. Another workaround may be needed there.

Also, the wallet_migration test won’t be running after #31248, so the previous releases tests should probably be run as well here.

maflcko commented at 7:52 pm on December 2, 2024:

(Functional tests could be handled in a follow-up, if you want)

hebasto commented at 1:41 pm on December 3, 2024:

(Functional tests could be handled in a follow-up, if you want)

The problematic tests have been disabled and documented.

maflcko approved

maflcko commented at 12:43 pm on November 26, 2024: member

lgtm

DrahtBot added the label Needs rebase on Nov 26, 2024

hebasto force-pushed on Dec 2, 2024

hebasto commented at 4:26 pm on December 2, 2024: member

Rebased. @maflcko’s comments have been addressed.

DrahtBot removed the label Needs rebase on Dec 2, 2024

ci: Move "Windows cross" job from Cirrus CI to GHA CI

This change is required for testing cross-built executables on Windows
natively.

cdec27ce8f

ci: Test cross-built Windows executables on Windows natively 9fb3ce1fa1

hebasto force-pushed on Dec 3, 2024

ci: Test cross-built Windows executables on Windows natively #31176

Code Coverage & Benchmarks

Reviews

Conflicts