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

Consider using a workspace instead of a fully separate crate #345

Closed
jyn514 opened this issue Jun 29, 2023 · 5 comments
Closed

Consider using a workspace instead of a fully separate crate #345

jyn514 opened this issue Jun 29, 2023 · 5 comments

Comments

@jyn514
Copy link

jyn514 commented Jun 29, 2023

Right now, cargo fuzz init generates the following Cargo.toml:

[package]
name = "example-fuzz"
version = "0.0.0"
publish = false
edition = "2021"

[package.metadata]
cargo-fuzz = true

[dependencies]
libfuzzer-sys = "0.4"

[dependencies.example]
path = ".."

# Prevent this from interfering with workspaces
[workspace]
members = ["."]

[profile.release]
debug = 1

[[bin]]
name = "fuzz_target_1"
path = "fuzz_targets/fuzz_target_1.rs"
test = false
doc = false

In particular, this puts all the fuzz targets in a separate workspace. I am not sure why that decision was made - it prevents reusing the build cache between the fuzzer and the rest of the code. It would be nice if they shared a workspace and therefore a cache.

I do see that this configures profile.release, so to avoid interfering with existing configs it could use a new profile.fuzzing or something like that.

@fitzgen fitzgen transferred this issue from rust-fuzz/arbitrary Jun 29, 2023
@fitzgen
Copy link
Member

fitzgen commented Jun 29, 2023

# Prevent this from interfering with workspaces

This has been there since before I started hacking on cargo fuzz. Not sure what the "interfering" alluded to actually is.

@Manishearth any idea?

@jyn514
Copy link
Author

jyn514 commented Jun 29, 2023

"interfering" is probably referring to this error you get if you remove the [workspace] declaration:

; c fuzz check   
error: current package believes it's in a workspace when it's not:
current:   /Users/jyn/src/example/fuzz/Cargo.toml
workspace: /Users/jyn/src/example/Cargo.toml

this may be fixable by ensuring that this crate is depended on by the workspace root: /Users/jyn/src/example/Cargo.toml
Alternatively, to keep it out of the workspace, add the package to the `workspace.exclude` array, or add an empty `[workspace]` table to the package's manifest.
Error: failed to build fuzz script: "cargo" "check" "--manifest-path" "/Users/jyn/src/example/fuzz/Cargo.toml" "--target" "aarch64-apple-darwin" "--release" "--bins"

so to make this part of the workspace, you'd have to also edit example/Cargo.toml, not just create the new example/fuzz/Cargo.toml.

@Manishearth
Copy link
Member

Yep. We can remove that if we also fix the workspace editing. We ought to.

@arvidn
Copy link

arvidn commented Jul 8, 2023

I agree that fuzzers should be part of the main workspace. Sharing the Cargo.lock with the main workspace ensures that you fuzz the exact dependencies that you build against. It also includes fuzz targets in cargo fmt --all and cargo clippy --workspace invocations, which is nice.

In addition to adding the fuzz directory to the root workspace members, you should also define the fuzz targets with:

bench = false

otherwise, cargo bench --workspace will get stuck, running fuzz targets as if they were benchmarks.

kdarkhan added a commit to kdarkhan/mdbook-i18n-helpers that referenced this issue Oct 4, 2023
Currently caching does not work for `fuzz` job which can
be verified by looking at the Github workflow output.

The main cause of the issue is the config `workspaces: fuzz -> target`.
The fuzz target does not use a proper workspace but a separate
crate. There is [a pending issue](rust-fuzz/cargo-fuzz#345)
discussing that.

The `rust-cache` action is executed in the root of the project and is
not able to locate `fuzz` directory by workspace name.

I decided to list the directories to cache instead of relying
on target dir detection which seems to work.

I also fixed the path `fuzz/corpus` to `i18n-helpers/fuzz/corpus`.

I adjusted the caching condition `safe-if` to cache only on `main`
builds. I think this makes sense because we don't need to cache PRs
which might not get merged or get updated multiple times during review.
kdarkhan added a commit to kdarkhan/mdbook-i18n-helpers that referenced this issue Oct 4, 2023
Currently caching does not work for `fuzz` job which can
be verified by looking at Github workflow exec outputs.

The main cause of the issue is the config `workspaces: fuzz -> target`.
The fuzz target does not use a proper workspace but a separate
crate. There is [a pending issue](rust-fuzz/cargo-fuzz#345)
discussing that.

The `rust-cache` action is executed in the root of the project and is
not able to locate `fuzz` directory by workspace name.

I decided to list the directories to cache instead of relying
on target dir detection which seems to work.

I also fixed the path `fuzz/corpus` to `i18n-helpers/fuzz/corpus`.

I adjusted the caching condition `save-if` to cache only on `main`
builds. I think this makes sense because we don't need to cache PRs
which might not get merged or get updated multiple times during review.
antoniolinhart pushed a commit to antoniolinhart/mdbook-i18n-helpers that referenced this issue Jan 29, 2024
Currently caching does not work for `fuzz` job which can
be verified by looking at Github workflow exec outputs.

The main cause of the issue is the config `workspaces: fuzz -> target`.
The fuzz target does not use a proper workspace but a separate
crate. There is [a pending issue](rust-fuzz/cargo-fuzz#345)
discussing that.

The `rust-cache` action is executed in the root of the project and is
not able to locate `fuzz` directory by workspace name.

I decided to list the directories to cache instead of relying
on target dir detection which seems to work.

I also fixed the path `fuzz/corpus` to `i18n-helpers/fuzz/corpus`.

I adjusted the caching condition `save-if` to cache only on `main`
builds. I think this makes sense because we don't need to cache PRs
which might not get merged or get updated multiple times during review.
@kdarkhan
Copy link
Contributor

I believe this issue can be resolved now given that 0.11.4 version of cargo-fuzz contains changes from #357

@fitzgen fitzgen closed this as completed Jan 30, 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

No branches or pull requests

5 participants