ci: Add Windows + UCRT jobs for cross-compiling and native testing #33764

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:251102-ucrt-ci changing 4 files +145 −11
  1. hebasto commented at 3:31 pm on November 2, 2025: member

    This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.

    For more details about this migration, see:

    A few items are outside the scope of this PR and are left for follow-up work:

    1. The version of Debian’s cross-compiler is 14.2.0, which differs from version 13.3.0 used in the Guix script for release binaries. Upgrading the latter might be worth considering.
    2. MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
  2. hebasto added the label Windows on Nov 2, 2025
  3. hebasto added the label Tests on Nov 2, 2025
  4. DrahtBot commented at 3:31 pm on November 2, 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/33764.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko
    Concept NACK fanquake

    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:

    • #33775 (guix: use GCC 14.3.0 over 13.3.0 by fanquake)
    • #25573 ([POC] guix: produce a fully -static-pie bitcoind 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.

  5. in ci/test/00_setup_env_win64_ucrt.sh:9 in 7510d4519e 
    0@@ -0,0 +1,18 @@
1+#!/usr/bin/env bash
2+#
3+# Copyright (c) 2025-present The Bitcoin Core developers
4+# Distributed under the MIT software license, see the accompanying
5+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
6+
7+export LC_ALL=C.UTF-8
8+
9+export CONTAINER_NAME=ci_win64_ucrt
    maflcko commented at 10:39 am on November 3, 2025:
    nit: Could consider naming this ci_win64 (and the file), as this will be the default eventually, going forward.
    hebasto commented at 10:03 pm on November 3, 2025:
    Thanks! Reworked.
  6. maflcko commented at 10:39 am on November 3, 2025: member
    should the depends build instructions be updated to mention this?
  7. fanquake commented at 10:42 am on November 3, 2025: member

    A new -Warray-bounds compiler diagnostic should be evaluated and addressed.

    If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.

  8. maflcko commented at 10:48 am on November 3, 2025: member

    If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.

    I think it is just one of the many gcc false-positive warnings. Not sure if it makes sense to document or investigate them individually, given that they are hard to reproduce in light of changing the compiler version or the compile options even slightly.

  9. fanquake commented at 10:56 am on November 3, 2025: member

    I think it is just

    If it had been documented (in the commit or PR description), we wouldn’t need to guess. Doing that seems like the minimum that should be done, when adding new suppressions; rather than supplying no information, and claiming someone should look in a followup.

  10. hebasto commented at 11:40 am on November 3, 2025: member

    should the depends build instructions be updated to mention this?

    I’d prefer updating the docs once depends/hosts/mingw32.mk is adjusted so that compilers don’t need to be specified explicitly.

  11. hebasto commented at 11:42 am on November 3, 2025: member

    If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.

    I think it is just one of the many gcc false-positive warnings.

    That sounds plausible, though I’m not entirely convinced at this point.

  12. hebasto commented at 11:49 am on November 3, 2025: member

    From https://github.com/bitcoin/bitcoin/actions/runs/19014374041/job/54300038967:

     0In file included from /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/array:43,
 1                 from /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/span:43,
 2                 from /home/admin/actions-runner/_work/_temp/src/span.h:10,
 3                 from /home/admin/actions-runner/_work/_temp/src/uint256.h:10,
 4                 from /home/admin/actions-runner/_work/_temp/src/consensus/params.h:10,
 5                 from /home/admin/actions-runner/_work/_temp/src/kernel/chainparams.h:9,
 6                 from /home/admin/actions-runner/_work/_temp/src/chainparams.h:9,
 7                 from /home/admin/actions-runner/_work/_temp/src/test/net_tests.cpp:5:
 8In static member function ‘static constexpr _Up* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(_Tp*, _Tp*, _Up*) [with _Tp = char; _Up = char; bool _IsMove = false]’,
 9    inlined from ‘constexpr _OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = char*; _OI = char*]’ at /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/bits/stl_algobase.h:521:30,
10    inlined from ‘constexpr _OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = false; _II = char*; _OI = char*]’ at /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/bits/stl_algobase.h:548:42,
11    inlined from ‘constexpr _OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = __gnu_cxx::__normal_iterator<char*, __cxx11::basic_string<char> >; _OI = char*]’ at /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/bits/stl_algobase.h:555:31,
12    inlined from ‘constexpr _OI std::copy(_II, _II, _OI) [with _II = __gnu_cxx::__normal_iterator<char*, __cxx11::basic_string<char> >; _OI = char*]’ at /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/bits/stl_algobase.h:651:7,
13    inlined from ‘void net_tests::{anonymous}::V2TransportTester::SendMessageA(std::string, std::span<const unsigned char>)’ at /home/admin/actions-runner/_work/_temp/src/test/net_tests.cpp:1304:18:
14/usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/bits/stl_algobase.h:452:30: warning: ‘void* __builtin_memmove(void*, const void*, long long unsigned int)’ offset [0, 1] is out of the bounds [0, 0] [-Warray-bounds=]
15  452 |             __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
16      |             ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

    @sipa

    As the author of this code, would you consider this warning a false positive?

  13. fanquake commented at 12:00 pm on November 3, 2025: member

    This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.

    I’m not sure whether to Concept ACK/NACK this, as according to https://github.com/mstorsjo/llvm-mingw/issues/523, we are considering switching to using LLVM for Windows builds (has that been discussed at length somewhere)?

    If that is that case, then it’s not clear that churning a bunch of code, adding new GCC CIs, and code to Guix, is worthwhile, if it will just need to be re-written again/thrown away soon after?

  14. hebasto commented at 12:06 pm on November 3, 2025: member

    … we are considering switching to using LLVM for Windows builds (has that been discussed at length somewhere)?

    The only discussion I’m aware of is in #31388, and no conclusion has been reached so far.

    On the other hand, there seems to be agreement on #30210.

  15. ci: Rename items specific to Windows + MSVCRT 
    This is necessary to prepare for introducing the new Windows + UCRT
script and jobs.
    4247d8a845
  16. ci: Add Windows + UCRT jobs for cross-compiling and native testing ac30f544d2
  17. test, refactor: Fix `-Warray-bounds` warning 3b135a8fc4
  18. hebasto force-pushed on Nov 3, 2025
  19. hebasto commented at 10:03 pm on November 3, 2025: member
    The feedback has been addressed.
  20. hebasto commented at 10:10 pm on November 3, 2025: member

    I’m not sure whether to Concept ACK/NACK this…

    Since this PR is only a subtask of the broader effort to resolve #30210, it seems more appropriate to have a conceptual UCRT-related discussion there on in #33593.

    This one simply addresses your comments here and here.

  21. maflcko commented at 10:25 am on November 4, 2025: member

    This looks like a nice and small CI-only/test-only change to flatten the way for #30210. There is already a pull open for the guix changes in #33593, which can then pick up any follow-ups from here and also conclude by removing the msvcrt CI config and msvcrt workarounds in the code.

    review ACK 3b135a8fc4451c93b3ea50b3f4621e0d19f35daf 🐂

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 3b135a8fc4451c93b3ea50b3f4621e0d19f35daf 🐂
3GGNiE1hD13QtTLvcrvA92ZP1OD3ly24HSGZbRQceL4YLLAM21XtAoV9N81XNmrIwRlFV/bq+Wa7ZbWQPllyIAA==
  22. in .github/workflows/ci.yml:440 in 3b135a8fc4 
    436@@ -445,6 +437,122 @@ jobs:
437           TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended' || '' }}
438         run: py -3 test/functional/test_runner.py --jobs $NUMBER_OF_PROCESSORS --ci --quiet --tmpdirprefix="$RUNNER_TEMP" --combinedlogslen=99999999 --timeout-factor=$TEST_RUNNER_TIMEOUT_FACTOR $EXCLUDE $TEST_RUNNER_EXTRA
439 
440+  windows-ucrt-cross:
    fanquake commented at 10:29 am on November 4, 2025:
    Is there a way to do this without copy-pasting/duplicating > 100 lines of CI config (where both copies need to be kept in sync).
    maflcko commented at 10:35 am on November 4, 2025:
    I guess yaml anchors could be used, but this would increase the diff and review effort again when it is deleted. An alternative would be to use a matrix, but I guess this requires depending both cross-test tasks on both cross-compile tasks? I’d say the current version is fine, but no strong opinion.
    hebasto commented at 11:19 am on November 4, 2025:

    I guess yaml anchors could be used, but this would increase the diff and review effort again when it is deleted. An alternative would be to use a matrix, but I guess this requires depending both cross-test tasks on both cross-compile tasks? I’d say the current version is fine, but no strong opinion.

    You’re right. I considered both of these options while working on this PR and came to the same conclusions.

  23. fanquake commented at 10:40 am on November 4, 2025: member

    This one simply addresses your comments #33593 (comment) and #33593#pullrequestreview-3398698978.

    Somewhat, but if this is the approach, it means that not only the compiler, but also the version of the headers being tested isn’t going to match Guix? (I’m wondering if internalising the headers might be a better approach, as that would be more flexible, and, if there is some plan to migrate to Clang, would likely be needed in any case?).

  24. hebasto commented at 12:22 pm on November 4, 2025: member

    This one simply addresses your comments #33593 (comment) and #33593 (review).

    Somewhat, but if this is the approach, it means that not only the compiler, but also the version of the headers being tested isn’t going to match Guix? (I’m wondering if internalising the headers might be a better approach, as that would be more flexible, and, if there is some plan to migrate to Clang, would likely be needed in any case?).

    I’ve gathered the available compiler and header versions here: https://gist.github.com/hebasto/dee3c918da49ee767ccf5eea43276407.

    It seems feasible to align Guix and CI.

    As mentioned in the PR description, upgrading the compiler in Guix could be considered a step toward matching tool versions.

  25. maflcko commented at 4:38 pm on November 4, 2025: member
    I think we’ve never seen a header-version specific bug in the windows-cross compilation, so trying to fit the major versions in the CI on a best-effort basis seems sufficient for now? If more specific matching is needed for better testing a specific scenario, or for llvm builds, it seems sufficient to do as a follow-up?


hebasto DrahtBot maflcko fanquake

Labels
Windows Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-11-06 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me