ci: Use previous releases in tests on Windows #32363

pull GarmashAlex wants to merge 12 commits into bitcoin:master from GarmashAlex:hpp1 changing 3 files +80 −10
  1. GarmashAlex commented at 1:51 pm on April 28, 2025: none

    Currently, none of the CI jobs running on Windows (Windows native and Windows test cross-built) use previous releases for testing. This is causing problems with tests like wallet_migration.py which require previous releases to be available.

    This PR fixes issue #32192 by implementing the following changes:

    1. Added Windows binaries to the SHA256_SUMS dictionary in get_previous_releases.py
    2. Modified the check_host function to recognize Windows hosts and map them to the “win64” platform
    3. Enhanced the download_binary function to handle Windows zip files differently from Linux/macOS tar.gz files
    4. Updated the wallet_migration.py test to be Windows-compatible by handling file copying in a platform-specific way
    5. Added DOWNLOAD_PREVIOUS_RELEASES=“true” to the Windows jobs in the CI configuration
    6. Added a step to download previous releases for Windows in the CI workflow
    7. Added a specific step to run the wallet_migration.py test with –previous-releases flag
    8. Excluded wallet_migration.py from the main functional tests to avoid running it twice

    With these changes, the CI jobs running on Windows will now download and use previous releases, allowing tests like wallet_migration.py to run successfully.

    Fixes #32192

  2. test: Add Windows binaries support to previous releases downloader 4fcab2957d
  3. test: Fix wallet_migration.py for Windows platform 819140ef81
  4. ci: Enable previous releases tests on Windows CI 60f9f1f023
  5. DrahtBot commented at 1:51 pm on April 28, 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/32363.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31410 (qa: Fix wallet_multiwallet.py by hebasto)

    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.

  6. DrahtBot added the label Tests on Apr 28, 2025
  7. DrahtBot commented at 1:51 pm on April 28, 2025: contributor
    ♻️ Automatically closing for now based on heuristics. Please leave a comment, if this was erroneous. Generally, please focus on creating high-quality, original content that demonstrates a clear understanding of the project’s requirements and goals.
  8. DrahtBot closed this on Apr 28, 2025
  9. hebasto reopened this on Apr 28, 2025
  10. in .github/workflows/ci.yml:395 in 60f9f1f023 outdated 
    391       - name: Run functional tests
392         env:
393           # TODO: Fix the excluded test and re-enable it.
394-          EXCLUDE: '--exclude wallet_multiwallet.py'
395+          EXCLUDE: '--exclude wallet_multiwallet.py --exclude wallet_migration.py'
396+          PYTHONIOENCODING: utf-8
    maflcko commented at 2:10 pm on April 28, 2025:
    not sure about running the test separately and then excluding it separately below. Just remove the exclusion?
  11. in test/functional/wallet_migration.py:119 in 60f9f1f023 outdated 
    115+            src_path = self.old_node.wallets_path / "wallet.dat"
116+            dst_path = self.master_node.wallets_path / "wallet.dat"
117+            # On Windows, we need to close all open file handles before copying
118+            if is_windows:
119+                import gc
120+                gc.collect()  # Try to release any file handles
    maflcko commented at 2:12 pm on April 28, 2025:
    I don’t think any wallet files are opened in this test, let alone without a context manager. Why would this be needed?
  12. in test/get_previous_releases.py:144 in 60f9f1f023 outdated 
    139@@ -134,8 +140,13 @@ def download_binary(tag, args) -> int:
140     platform = args.platform
141     if tag < "v23" and platform in ["x86_64-apple-darwin", "arm64-apple-darwin"]:
142         platform = "osx64"
143+    if tag < "v23" and platform == "win64":
144+        platform = "win64"  # Windows platform names have been consistent
    maflcko commented at 2:14 pm on April 28, 2025:
    no-op?
  13. remove the exclusion c866e5e725
  14. delete unnecessary code 7e6a0fe7e6
  15. delete useless code 6b3920f6ab
  16. DrahtBot added the label CI failed on Apr 28, 2025
  17. DrahtBot commented at 2:40 pm on April 28, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/41281497857 LLM reason (✨ experimental): The CI failure is caused by errors detected by the Python linter ‘ruff,’ specifically an unused variable warning in wallet_migration.py, which has not been fixed.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  18. correct lint aa31d6ba22
  19. GarmashAlex commented at 2:43 pm on April 28, 2025: none
    @maflcko I’ve made some corrections
  20. GarmashAlex requested review from maflcko on Apr 28, 2025
  21. maflcko commented at 2:47 pm on April 28, 2025: member

    Please don’t ask for review before CI is passing

    Please link to a passing CI in your project.

    Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

  22. Update get_previous_releases.py 7987f07e93
  23. Update wallet_migration.py bcd1d6ba64
  24. DrahtBot removed the label CI failed on Apr 28, 2025
  25. in test/get_previous_releases.py:114 in bcd1d6ba64 outdated 
    105@@ -106,6 +106,12 @@
106     "6ee1a520b638132a16725020146abea045db418ce91c02493f02f541cd53062a": {"tag": "v28.0", "tarball": "bitcoin-28.0-riscv64-linux-gnu.tar.gz"},
107     "77e931bbaaf47771a10c376230bf53223f5380864bad3568efc7f4d02e40a0f7": {"tag": "v28.0", "tarball": "bitcoin-28.0-x86_64-apple-darwin.tar.gz"},
108     "7fe294b02b25b51acb8e8e0a0eb5af6bbafa7cd0c5b0e5fcbb61263104a82fbc": {"tag": "v28.0", "tarball": "bitcoin-28.0-x86_64-linux-gnu.tar.gz"},
109+
110+    "bff531650dcf859c27d8428dc5f98f3f93d9b6d54e4c1401e0ea9651f1edd7a3": {"tag": "v22.0", "tarball": "bitcoin-22.0-win64.zip"},
111+    "e16fdbdc4ee953969df1ce8f82380e9c61658b1a64801bd21e2e87063e93bc6a": {"tag": "v23.0", "tarball": "bitcoin-23.0-win64.zip"},
112+    "97de6e2a4d91531c057537e83bb7e72b1cf9ab777601eb481f3f4a6d9e7b9c67": {"tag": "v24.0.1", "tarball": "bitcoin-24.0.1-win64.zip"},
113+    "c0d22e9d37d0238215676af1c4e358c8af3a43fe7a25c57499b4c66ca80381da": {"tag": "v25.0", "tarball": "bitcoin-25.0-win64.zip"},
114+    "f2974a7df505cff14ca92dd7a23ba7e47f1b97ae7e7a12a6fc2f5f5c0a66ca10": {"tag": "v28.0", "tarball": "bitcoin-28.0-win64.zip"},
    achow101 commented at 8:34 pm on April 28, 2025:

    Please put these with their respective versions above.

    If we’re going to do Windows testing as well, then we should be downloading all of the versions above, not just this small subset. This subset doesn’t even make sense, there’s no single test that uses just these versions. wallet_migration.py uses only 28.0, the rest require the older versions too.

  26. in test/get_previous_releases.py:145 in bcd1d6ba64 outdated 
    141@@ -136,6 +142,9 @@ def download_binary(tag, args) -> int:
142         platform = "osx64"
143     tarball = 'bitcoin-{tag}-{platform}.tar.gz'.format(
144         tag=tag[1:], platform=platform)
145+    if platform == "win64" and tag >= "v22.0":
    achow101 commented at 8:36 pm on April 28, 2025:
    Why is this restricted to 22.0 and above? The naming scheme for the Windows zips hasn’t changed.
  27. in test/get_previous_releases.py:172 in bcd1d6ba64 outdated 
    173-                          '--strip-components=1',
174-                          'bitcoin-{tag}'.format(tag=tag[1:])]).returncode
175-    if ret != 0:
176-        print(f"Failed to extract the {tag} tarball")
177-        return ret
178+    if platform == "win64" and tag >= "v22.0":
    achow101 commented at 8:38 pm on April 28, 2025:
    Why restrict to 22.0 and above?
  28. in test/get_previous_releases.py:174 in bcd1d6ba64 outdated 
    175-    if ret != 0:
176-        print(f"Failed to extract the {tag} tarball")
177-        return ret
178+    if platform == "win64" and tag >= "v22.0":
179+        # Handle zip files for Windows
180+        import zipfile
    achow101 commented at 8:38 pm on April 28, 2025:
    imports go at the top of the file.
  29. in test/get_previous_releases.py:288 in bcd1d6ba64 outdated 
    283@@ -258,6 +284,8 @@ def check_host(args) -> int:
284             'x86_64-*-linux*': 'x86_64-linux-gnu',
285             'x86_64-apple-darwin*': 'x86_64-apple-darwin',
286             'aarch64-apple-darwin*': 'arm64-apple-darwin',
287+            'x86_64-w64-mingw32': 'win64',
288+            'x86_64-*-win*': 'win64',
    achow101 commented at 8:41 pm on April 28, 2025:
    What is the purpose of this host triple? We don’t make Windows binaries with any other host triple.
  30. in test/functional/wallet_migration.py:128 in bcd1d6ba64 outdated 
    125+                d = os.path.join(dst_path, item)
126+                if os.path.isfile(s):
127+                    shutil.copy2(s, d)
128+                else:
129+                    shutil.copytree(s, d, dirs_exist_ok=True)
130+
    achow101 commented at 8:42 pm on April 28, 2025:
    What is the purpose of this change? It seems entirely unnecessary and unrelated to the purpose of this PR.
  31. in .github/workflows/ci.yml:390 in bcd1d6ba64 outdated 
    385+      - name: Run wallet_migration.py test
386+        if: env.DOWNLOAD_PREVIOUS_RELEASES == 'true'
387+        env:
388+          PYTHONIOENCODING: utf-8
389+        run: py -3 test/functional/wallet_migration.py --previous-releases
390+
    achow101 commented at 8:45 pm on April 28, 2025:
    Why does wallet_migration.py need to be run separately, and none of the other tests that require previous releases? These can all be run at the same time from the test_runner.py invocation if the PREVIOUS_RELEASES_DIR environment variable is set.
  32. Update ci.yml 2b168a9a55
  33. Update ci.yml ffa0d89099
  34. Update get_previous_releases.py 99b86a125c
  35. in .github/workflows/ci.yml:395 in bcd1d6ba64 outdated 
    390+
391       - name: Run functional tests
392         env:
393           # TODO: Fix the excluded test and re-enable it.
394           EXCLUDE: '--exclude wallet_multiwallet.py'
395+          PYTHONIOENCODING: utf-8
    achow101 commented at 8:45 pm on April 28, 2025:
    Unnecessary change?
  36. fanquake commented at 1:37 pm on April 29, 2025: member
    This is complete AI slop. Please stop opening these kinds of PRs against this repo.
  37. fanquake closed this on Apr 29, 2025


GarmashAlex DrahtBot maflcko achow101 fanquake


maflcko

Labels
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-05-05 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me