Skip to content
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

refactor(fuzz): add #[cfg(fuzzing)] #1948

Closed
wants to merge 1 commit into from

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Jul 31, 2024

quinn-proto exposes some hooks for fuzzing only. These are behind a #[cfg(fuzzing)].

The fuzz crate imports these hooks, but without the #[cfg(fuzzing)].

Running cargo check --workspace thus fails importing each of these hooks. For example:

error[E0432]: unresolved import `proto::fuzzing`
  --> fuzz/fuzz_targets/packet.rs:7:5
   |
7  |     fuzzing::{PacketParams, PartialDecode},
   |     ^^^^^^^ could not find `fuzzing` in `proto`

This commit adds the various #[cfg(fuzzing)] to fuzz/, preventing the above errors.


Needed for #1934.

@@ -1,13 +1,19 @@
#![no_main]

#[cfg(fuzzing)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cfg statements here are noisy. Would you prefer a single mod behind a cfg?

`quinn-proto` exposes some hooks for fuzzing only. These are behind a `#[cfg(fuzzing)]`.

The `fuzz` crate imports these hooks, but without the `#[cfg(fuzzing)]`.

Running `cargo check --workspace` thus fails importing each of these hooks. For example:

```
error[E0432]: unresolved import `proto::fuzzing`
  --> fuzz/fuzz_targets/packet.rs:7:5
   |
7  |     fuzzing::{PacketParams, PartialDecode},
   |     ^^^^^^^ could not find `fuzzing` in `proto`
```

This commit adds the various `#[cfg(fuzzing)]` to `fuzz/`, preventing the above errors.
@Ralith
Copy link
Collaborator

Ralith commented Jul 31, 2024

I'm not sure this is the right approach. #[cfg(...)] disables code, so you're really just skipping the crate entirely by verbose means. What do other projects using this fuzzer do? Maybe we should exclude the crate from the workspace by default?

@djc
Copy link
Member

djc commented Jul 31, 2024

I'm not sure this is the right approach. #[cfg(...)] disables code, so you're really just skipping the crate entirely by verbose means. What do other projects using this fuzzer do? Maybe we should exclude the crate from the workspace by default?

Yeah, I think using the workspace default-members config value might be a better approach.

@mxinden
Copy link
Contributor Author

mxinden commented Aug 5, 2024

I wasn't aware of default-members. Thank you. Agreed, the status quo is much cleaner.

quinn/Cargo.toml

Lines 1 to 3 in 2d06eef

[workspace]
members = ["quinn", "quinn-proto", "quinn-udp", "bench", "perf", "fuzz"]
default-members = ["quinn", "quinn-proto", "quinn-udp", "bench", "perf"]

@mxinden mxinden closed this Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants