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

fix: use cargo-metadata to support init command in workspace #349

Merged
merged 5 commits into from
Dec 12, 2023

Conversation

0xalpharush
Copy link
Contributor

@0xalpharush 0xalpharush commented Dec 8, 2023

closes #333

Instead of manually parsing the toml file and failing in workspace members, the package name and edition are obtained via cargo-metadata (this also seems to obviate the need to pass that path which is a breaking change to a public API). The way the tests are setup was making cargo-metadata think that each test is a member of cargo-fuzz so I added an empty workspace entry to the tests' Cargo.toml.

@0xalpharush 0xalpharush force-pushed the fix/init-in-workspace branch from fab3eb3 to e6c984d Compare December 8, 2023 05:22
@0xalpharush 0xalpharush force-pushed the fix/init-in-workspace branch from e6c984d to 076c771 Compare December 8, 2023 05:22
@fitzgen
Copy link
Member

fitzgen commented Dec 11, 2023

I've had issues with cargo-metadata blowing up compile times in the past. Can you provide the timings for cargo clean; cargo build both on main and on this feature branch?

@0xalpharush
Copy link
Contributor Author

Using hyperfine on my M1, it adds ~1s

Feature branch:

Benchmark 1: cargo clean; cargo build
  Time (mean ± σ):      6.016 s ±  0.043 s    [User: 21.272 s, System: 2.145 s]
  Range (min … max):    5.968 s …  6.093 s    10 runs

Main branch:

Benchmark 1: cargo clean; cargo build
 Time (mean ± σ):      5.009 s ±  0.113 s    [User: 14.871 s, System: 1.736 s]
 Range (min … max):    4.859 s …  5.199 s    10 runs

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

One second doesn't seem too bad, given that this fixes bugs and cleans up some fiddly bits.

Some questions below.

src/project.rs Outdated Show resolved Hide resolved
src/project.rs Outdated Show resolved Hide resolved
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks!

@fitzgen
Copy link
Member

fitzgen commented Dec 11, 2023

Looks like CI is failing? Don't have time to dig in myself at the moment.

@0xalpharush
Copy link
Contributor Author

The tests were relying on the panic error format pre rustc 1.73.0 https://blog.rust-lang.org/2023/10/05/Rust-1.73.0.html

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks!

@fitzgen fitzgen merged commit c4a4d33 into rust-fuzz:main Dec 12, 2023
1 check passed
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.

cargo fuzz init fails with edition.workspace = true
3 participants