-
Notifications
You must be signed in to change notification settings - Fork 698
Update cargo-fuzz setup; split fuzz_value_sanitize
so it runs
#6372
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
base: develop
Are you sure you want to change the base?
Update cargo-fuzz setup; split fuzz_value_sanitize
so it runs
#6372
Conversation
- Fix compiler errors in `clarity/fuzz/` and refresh dependencies. - Move `fuzz_value_sanitize` into its own fuzz target so `cargo-fuzz` runs it. Notes: - Now builds and runs on `+nightly`. - I have not fully reviewed semantic validity; kept the diff minimal. - Prelude for stacks-network#6355 (Expand input coverage with fuzzing). How to: - List: `cargo +nightly fuzz list` - Build: `cargo +nightly fuzz build [target]` - Run: `cargo +nightly fuzz run fuzz_value_sanitize`
/cc @BowTiedRadone |
ad92993
to
f8db633
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve added a bunch of comments about the workflows.
I’d also like to raise a broader structural idea: instead of keeping the fuzz
folder nested inside the clarity
crate, would it make sense to move it under the contrib/
folder?
We could, for example, introduce a dedicated fuzzing/
folder inside contrib/
, which could later house multiple fuzzers as we expand coverage. The current clarity/fuzz
would simply become one of them:
contrib/
fuzzing/
clarity-fuzzer/ (current clarity/fuzz)
other1-fuzzer/
other2-fuzzer/
...
This would avoid nested crates inside clarity
, make the structure more consistent, and give us room to organize future fuzzers without cluttering core crates.
If we agree this structure is beneficial, we could address it in this PR. The effort should be minimal, and since we’re already restoring the old fuzzer, could be the right time to make this change.
inputs: | ||
rust-nightly-version: | ||
description: Nightly toolchain to use. | ||
required: false | ||
type: string | ||
default: nightly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not entirely clear why this workflow requires this input. Could you elaborate?
We might consider simplifying by hardcoding the version in the fuzz-targets
job or by using an environment variable instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, cargo fuzz
needs nightly. However we could follow the rule of three and inline it for now directly in the job.
fuzz-targets: | ||
name: Fuzz targets (nightly) | ||
runs-on: ubuntu-latest | ||
needs: setup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fuzz-targets
job doesn’t actually need the setup dependency, as it completely overrides the toolchain
parameter with its own value.
We might consider removing this dependency to make clear the intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this doesn’t use any outputs from setup
. 👍
toolchain: ${{ inputs.rust-nightly-version }} | ||
cache: true | ||
cache-key: > | ||
fuzz-${{ hashFiles('clarity/fuzz/Cargo.lock') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe cache key is too narrow. If nightly version changes, we could get stale caches that fail to build because dependencies or the compiler version mismatch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps
fuzz-${{ inputs.rust-nightly-version }}-${{ hashFiles('clarity/fuzz/Cargo.lock') }}
or something like
fuzz-${{ inputs.rust-nightly-version }}-${{ runner.os }}-${{ hashFiles('clarity/fuzz/Cargo.lock') }}
We'll have to try.
|
||
- name: Build fuzz targets (compile-only) | ||
run: | | ||
cargo +${{ inputs.rust-nightly-version }} build \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use check here instead of build?
Since the objective is only to verify whether it compiles, this would be faster and more aligned with the purpose of the workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I always do cargo +nightly fuzz build
but here check would be more efficient.
Right now it makes more sense to keep the That way it matches how we handle example-based and property-based tests, and keeps fuzzing a first-class part of the folder/crate rather than something tucked away in Also, with this structure, Moving it under I think it’s a good idea to tackle reorganizing later, once we add more fuzzers, so we can keep #6355 moving. |
This makes
clarity/fuzz/
build and actually run on nightly. It also movesfuzz_value_sanitize
into its own target socargo-fuzz
detects and runs it. Diff kept minimal.Notes:
How to:
cargo +nightly fuzz list
cargo +nightly fuzz build
cargo +nightly fuzz build fuzz_sanitize
cargo +nightly fuzz build fuzz_value_sanitize
cargo +nightly fuzz run fuzz_sanitize
cargo +nightly fuzz run fuzz_value_sanitize