Skip to content

fuzz: Add missing fuzz targets to cmake build #30712

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 28, 2024
Merged

fuzz: Add missing fuzz targets to cmake build #30712

merged 1 commit into from
Aug 28, 2024
+2 −0

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Aug 25, 2024

Fixes #30454 (comment)

Can be tested via:

PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-autot/src/test/fuzz/fuzz > /tmp/f_autot
PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-cmake/src/test/fuzz/fuzz > /tmp/f_cmake
diff --unified /tmp/{f_autot,f_cmake}

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 25, 2024 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
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:

  • #30717 (rpc: Add test-only RPCs under -test=<option> flag by Prabhat1308)
  • #30685 (build: Mark x86_64-linux-gnu release binaries as CET-enabled by hebasto)
  • #30634 (ci: Use clang-19 from apt.llvm.org by maflcko)
  • #30619 (doc: Change nproc in docs to getconf _NPROCESSORS_ONLN by l0rinc)
  • #30527 (Bump python minimum supported version to 3.10 by maflcko)
  • #30465 (depends: Set CMAKE_SYSTEM_VERSION for CMake builds by hebasto)
  • #30043 (net: Replace libnatpmp with built-in PCP+NATPMP implementation by laanwj)
  • #29881 (guix: use GCC 13 to builds releases by fanquake)
  • #29868 (Reintroduce external signer support for Windows by hebasto)
  • #25573 ([POC] guix: produce a fully -static-pie bitcoind by fanquake)
  • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
  • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
  • #10102 (Multiprocess bitcoin by ryanofsky)

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.

@DrahtBot DrahtBot changed the title fuzz: Add missing fuzz targets to cmake build fuzz: Add missing fuzz targets to cmake build Aug 25, 2024
@DrahtBot DrahtBot added the Tests label Aug 25, 2024
@hebasto
Copy link
Member

hebasto commented Aug 25, 2024

Looks correct.

This was referenced Aug 25, 2024
Closed
Merged
Merged
Closed
Merged
Closed
Merged
Merged
Draft
Draft
Draft
Draft
Closed
@hebasto hebasto mentioned this pull request Aug 27, 2024
Merged
@hebasto
Copy link
Member

hebasto commented Aug 27, 2024

Can be tested via:

PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-autot/src/test/fuzz/fuzz > /tmp/f_autot
PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-cmake/src/test/fuzz/fuzz > /tmp/f_cmake
diff --unified /tmp/{f_autot,f_cmake}

Such a check can be automated: https://github.com/hebasto/bitcoin/commits/pr30712_amended.

hebasto
hebasto reviewed Aug 27, 2024
View reviewed changes
cmake/script/Coverage.cmake Outdated
@@ -5,6 +5,7 @@
include(${CMAKE_CURRENT_LIST_DIR}/CoverageInclude.cmake)

set(functional_test_runner test/functional/test_runner.py)
list(APPEND functional_test_runner --timeout-factor=8) # Required, because coverage builds are slower
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed the same -- hebasto#341 (comment):

However, on my machine, I had to add --timeout-factor=4.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped from this pull for now

@maflcko maflcko force-pushed the 2408-fuzz-cmake branch 2 times, most recently from 0dbe55d to 41a5479 Compare August 27, 2024 15:28
@DrahtBot DrahtBot mentioned this pull request Aug 28, 2024
Merged
4 tasks
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

@maflcko maflcko marked this pull request as ready for review August 28, 2024 10:00
@maflcko
Copy link
Member Author

maflcko commented Aug 28, 2024

Rebased.

Should probably be merged before switching any fuzzing over to cmake.

@maflcko
Copy link
Member Author

maflcko commented Aug 28, 2024

@hebasto I see your comments, but I am not sure if you approve this pull request, or if you want to open an alternative.

hebasto
hebasto approved these changes Aug 28, 2024
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa0e1e4

@fanquake fanquake merged commit e45913e into bitcoin:master Aug 28, 2024
5 of 16 checks passed
@maflcko maflcko deleted the 2408-fuzz-cmake branch August 28, 2024 10:16
@hebasto hebasto mentioned this pull request Aug 29, 2024
Merged
@bitcoin bitcoin locked and limited conversation to collaborators Aug 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maflcko @DrahtBot @hebasto @fanquake