ci: Split out native fuzz jobs for macOS and windows (take 2) #31221

pull dergoegge wants to merge 1 commits into bitcoin:master from dergoegge:2024-10-native-fuzz changing 2 files +69 −8
  1. dergoegge commented at 2:45 pm on November 5, 2024: member

    Split out two new CI jobs (for native macOS and windows) that run the fuzz tests on the qa-assets input corpora.

    In both jobs the fuzz binary is built with -DBUILD_FOR_FUZZING to enable Assume assertions as well as FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION.

  2. DrahtBot commented at 2:46 pm on November 5, 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/31221.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko
    Concept ACK marcofleon, 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:

    • #31158 (build, ci: Fix linking bitcoin-chainstate.exe to bitcoinkernel.dll on Windows 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.

  3. DrahtBot added the label Tests on Nov 5, 2024
  4. dergoegge commented at 2:46 pm on November 5, 2024: member
    cc @maflcko couldn’t reopen the other PR after force pushing
  5. dergoegge marked this as a draft on Nov 5, 2024
  6. dergoegge commented at 3:47 pm on November 5, 2024: member
    Looks like using the matrix feature works for this
  7. dergoegge marked this as ready for review on Nov 5, 2024
  8. dergoegge marked this as a draft on Nov 5, 2024
  9. dergoegge force-pushed on Nov 6, 2024
  10. dergoegge marked this as ready for review on Nov 6, 2024
  11. dergoegge commented at 1:56 pm on November 6, 2024: member
    Now it works, ready for review!
  12. in .github/workflows/ci.yml:78 in 441a8b450b outdated
    73@@ -74,20 +74,27 @@ jobs:
    74           # Use clang++, because it is a bit faster and uses less memory than g++
    75           git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='-Wno-error=unused-member-function' && cmake --build build -j $(nproc) && ctest --output-on-failure --test-dir build -j $(nproc) && ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 ))" ${{ env.TEST_BASE }}
    76 
    77-  macos-native-arm64:
    78-    name: 'macOS 14 native, arm64, no depends, sqlite only, gui'
    79-    # Use latest image, but hardcode version to avoid silent upgrades (and breaks).
    80-    # See: https://github.com/actions/runner-images#available-images.
    81+  macos-native:
    82+    name: 'macOS 14 native'
    


    hebasto commented at 2:18 pm on November 6, 2024:

    You might want to use ${{ matrix.job_name }}.

    Same for the Windows job.


    dergoegge commented at 2:48 pm on November 6, 2024:
    Doesn’t look like this works

    dergoegge commented at 2:51 pm on November 6, 2024:
    It had to be job-name not job_name
  13. in .github/workflows/ci.yml:207 in 441a8b450b outdated
    205         uses: actions/cache/save@v4
    206         if: github.event_name != 'pull_request' && steps.vcpkg-binary-cache.outputs.cache-hit != 'true'
    207         with:
    208           path: ~/AppData/Local/vcpkg/archives
    209-          key: ${{ github.job }}-vcpkg-binary-${{ hashFiles('cmake_version', 'msbuild_version', 'toolset_version', 'vcpkg.json') }}
    210+          key: ${{ matrix.job-type }}-vcpkg-binary-${{ hashFiles('cmake_version', 'msbuild_version', 'toolset_version', 'vcpkg.json') }}
    


    hebasto commented at 2:24 pm on November 6, 2024:
    Instead of saving another cache, we can simply skip the cache saving step for the “fuzz” job type.

    dergoegge commented at 2:43 pm on November 6, 2024:
    Got rid of all the duplicate caches
  14. dergoegge force-pushed on Nov 6, 2024
  15. dergoegge force-pushed on Nov 6, 2024
  16. dergoegge force-pushed on Nov 6, 2024
  17. in .github/workflows/ci.yml:166 in 59993e058d outdated
    161+          - job-type: standard
    162+            generate-options: '-DBUILD_GUI=ON -DWITH_BDB=ON -DWITH_ZMQ=ON -DBUILD_BENCH=ON -DWERROR=ON'
    163+            job-name: 'Win64 native, VS 2022'
    164+          - job-type: fuzz
    165+            generate-options: '-DBUILD_FOR_FUZZING=ON -DWERROR=ON'
    166+            job-name: 'Win64 native fuzz, VS 2022'
    


    maflcko commented at 10:58 am on November 7, 2024:

    It seems a bit annoying that the Windows build is now duplicated and seems to take 2h each times 2 = 4 CI hours total.

    I wonder if this could be avoided (or reduced) by using a cmake multi config build. Putting the fuzz result into a separate folder and then using that in the fuzz_runner should be enough?


    dergoegge commented at 11:00 am on November 7, 2024:

    It seems a bit annoying that the Windows build is now duplicated and seems to take 2h each times 2 = 4 CI hours total.

    I assumed this is a caching issue? This shouldn’t be doing anything different from the current windows CI…


    fanquake commented at 11:05 am on November 7, 2024:
    The majority of the time here is vcpkg compiling Qt modules from source, in the “Generate build system step”: https://github.com/bitcoin/bitcoin/actions/runs/11705992609/job/32602049089?pr=31221. That should be getting cached in some way. Maybe push again and see if things are rebuilt?

    dergoegge commented at 11:07 am on November 7, 2024:
    Re-pushed

    fanquake commented at 11:10 am on November 7, 2024:
    I would also think we can just turn off everything GUI related for fuzzing?

    maflcko commented at 11:13 am on November 7, 2024:

    I would also think we can just turn off everything GUI related off for fuzzing?

    I don’t think this is possible while sharing the cache


    dergoegge commented at 11:13 am on November 7, 2024:
    Looks like it’s rebuilding everything again, could it be that it can’t store new caches on PRs?

    dergoegge commented at 11:20 am on November 7, 2024:

    Looks like it’s rebuilding everything again, could it be that it can’t store new caches on PRs?

    The pkg cache is not stored on PRs (see here).

    But I think the main problem was that I changed the job name from win64-native to windows-native which changed the lookup key for the cache (I assume). Re-pushed again.


    dergoegge commented at 11:26 am on November 7, 2024:
    “Cache restored successfully” 🎉. “Generate build system” only took 4min now. Resolving.
  18. dergoegge force-pushed on Nov 7, 2024
  19. dergoegge force-pushed on Nov 7, 2024
  20. [ci] Split out native fuzz jobs for macOS and windows c3354ea6dc
  21. in .github/workflows/ci.yml:137 in 2a0997b3a5 outdated
    129@@ -120,6 +130,8 @@ jobs:
    130 
    131       - name: CI script
    132         run: ./ci/test_run_all.sh
    133+        env:
    134+          FILE_ENV: ${{ matrix.file-env }}
    135 
    136       - name: Save Ccache cache
    137         uses: actions/cache/save@v4
    


    maflcko commented at 11:45 am on November 7, 2024:
    This will create a stale ccache with fuzz blobs, which will then be picked up the normal build, no?

    dergoegge commented at 11:49 am on November 7, 2024:
    Hm, should we have a separate ccache for each job type? I assume, there is no way to merge the ccache from the two

    dergoegge commented at 12:18 pm on November 7, 2024:

    have a separate ccache for each job type

    Done

  22. dergoegge force-pushed on Nov 7, 2024
  23. marcofleon commented at 4:44 pm on November 7, 2024: contributor
    Concept ACK
  24. maflcko commented at 12:44 pm on November 8, 2024: member

    skimmed over the diff and a bit of the CI output and it appears plausible.

    lgtm ACK c3354ea6dc34a3cadc60346c363478af22a5c0c2

  25. DrahtBot requested review from marcofleon on Nov 8, 2024
  26. dergoegge commented at 1:16 pm on November 8, 2024: member
    @hebasto could you give this another look?
  27. fanquake commented at 2:18 pm on November 11, 2024: member
    Concept ACK.
  28. maflcko added this to the milestone 29.0 on Nov 17, 2024

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: 2024-11-21 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me