-
Notifications
You must be signed in to change notification settings - Fork 37.8k
ci: Split out native fuzz jobs for macOS and windows #31073
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.
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
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Not sure about rushing this. Maybe wait for a fix for #31057 first, after which this may or may not be needed? |
Can this be closed? |
I still think this would make sense but there seems to be no interest. Also #30950 has been resolved. |
…ZZING=ON to fuzz fafbf8a Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to execute a fuzz target (MarcoFalke) fae3cf0 ci: Temporarily disable macOS/Windows fuzz step (MarcoFalke) Pull request description: `g_fuzzing` is used inside `Assume` at runtime, causing significant overhead in hot paths. See #31178 One could simply remove the `g_fuzzing` check from the `Assume`, but this would make fuzzing a bit less useful. Also, it would be unclear if `g_fuzzing` adds a runtime overhead in other code paths today or in the future. Fix all issues by making `G_FUZZING` equal to the build option `BUILD_FOR_FUZZING`, and for consistency in fuzzing, require it to be set when executing any fuzz target. Fixes #31178 Temporarily this drops fuzzing from two CI tasks, but they can be re-added in a follow-up with something like #31073 ACKs for top commit: marcofleon: Tested ACK fafbf8a davidgumberg: I still ACK fafbf8a for fixing the regression measured in #31178. ryanofsky: Code review ACK fafbf8a but approach -0, because this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing. So I would like to at least consider other solutions to this problem even if we go with this one. dergoegge: utACK fafbf8a Tree-SHA512: 124fc2e8b35e0c4df414436556a7a0a36cd1bec4b3000b40dcf2ab8c85f32e0610bf7f70d2fd79223d62f3c3665b6c09da21241654c7b9859461b8ca340d5421
This may be re-considered after a rebase. However, I wonder if the bloat can be reduced, assuming GHA has a matrix feature, so that the non-fuzz and fuzz-build can share most of the GHA config and steps? |
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 enableAssume
assertions as well asFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
.