Consolidate all fuzz test info (corpus, dictionaries, etc.) in one rule. #94
Conversation
stefanbucur
requested review from
asraa,
inferno-chromium,
markus-kusano and
oliverchang
as code owners
| # Building and runing empty_fuzz_test_with_invalid_dict should be ok, | ||
| # but building and running empty_fuzz_test_with_invalid_dict_run are expected to get an error message | ||
| # due to the invalid dictionary. | ||
| cc_fuzz_test( |
There was a problem hiding this comment.
Is this still being used or does it still make sense to have it around (e.g., for testing)? Mostly asking since the PR description sounded like this was mostly going to be a refactoring
There was a problem hiding this comment.
That's a great point - this was intended to be a refactoring, but it does indeed slightly change the semantics of the cc_fuzz_test rule: while the rule output stays the same, there are now more targets in the macro that are built by default. This test was assuming one of these targets (the dictionary) would not be built by default, so someone could manually check that this fuzz test would fail.
But since these manual "compilation failure" tests are rather brittle and less useful, I opted to remove this one for the time being.
I opened #95 to track the addition of a proper regression test.
| }, | ||
| ) | ||
|
|
||
| def _fuzzing_binary_transition_impl(settings, attr): |
There was a problem hiding this comment.
It is not for this particular transition, but this is part of the standard signature required by the implementation attribute of the transition() definition below. (It's a bit like argc and argv in a main function.)
The new rule supersedes the instrumented_fuzzing_binary rule and gets to incorporate all the other metadata about the fuzz test, such as corpus and dictionary paths, and the info about the fuzzing engine used. This simplifies the creation of additional rules that depend on fuzz test binaries, which can retrieve this metadata from the new CcFuzzingBinaryInfo provider.
stefanbucur
force-pushed
the
fuzzing-binary-rule-consolidation
branch
from
c34d0de to
a86b11d
Compare
This is an internal refactoring that largely has no effect on the interface of the
cc_fuzz_testmacro.The new
fuzzing_binaryrule supersedes theinstrumented_fuzzing_binaryrule. In addition to providing binary instrumentation, it collects all the other metadata about the fuzz test, such as corpus and dictionary paths, and the info about the fuzzing engine used, and makes it available to consumers via the newCcFuzzingBinaryInfoprovider. This simplifies the creation of additional rules that depend on fuzz test binaries, such as the upcoming OSS-Fuzz packaging rule.There is one major change in behavior for
cc_fuzz_test: it now always builds the corpus and dictionary dependencies as part of building the main fuzzing binary. This is advantageous, but at the same time it invalidates an existing "compilation failure" test which was run manually. The test is disabled for the time being - I opened #95 to track this issue.