search

apoelstra / rust-jsonrpc

Rust JSONRPC library
Creative Commons Zero v1.0 Universal
158 stars 62 forks source link

Add workspace #98

Closed tcharding closed 1 year ago

tcharding commented 1 year ago

As we are doing in other repos; add a workspace and move current code into jsonrpc crate. Add fuzz to the workspace too.

Draft because this isn't quite right. Was being able to run cargo test in the fuzz crate the main motivation for creating a workspace? Here that doesn't work because there is no code in the fuzz test to run if we are not fuzzing (because of FUZZ_TCP_SOCKET).

Throwing this up just for input please.

apoelstra commented 1 year ago

@tcharding yes, one motivation is to be able to run cargo test -- though as you say, in this case our fuzz tests require a cfg flag to avoid pinging the network. Another motivation is so that crate2nix can find the fuzzing crate without my doing awful path hacks.

But you don't need to move any existing files to do this. You can just add

[workspace]
members = ["fuzz"]

to the bottom of the existing Cargo.toml file.

tcharding commented 1 year ago

But you don't need to move any existing files to do this. You can just add

[workspace]
members = ["fuzz"]

to the bottom of the existing Cargo.toml file.

Ha! Cool, TIL. I'll do that instead.

apoelstra commented 1 year ago

nit: I'd prefer the workspace section to be at the bottom of Cargo.toml to match the other crates where we've done this.

But more importantly: this PR currently won't let you run cargo test --all at the base of this crate. I think we should demote the fuzzing error to a warning and simply skip the tests if the flag is not set.

tcharding commented 1 year ago

nit: I'd prefer the workspace section to be at the bottom of Cargo.toml to match the other crates where we've done this.

ha ha, I moved this on purpose to match one other place its like this, noticing that not all crates had it in the same spot :) I'll put it at the bottom and when I remember which crate I copied I'll move it to the bottom in that crate as well.

But more importantly: this PR currently won't let you run cargo test --all at the base of this crate. I think we should demote the fuzzing error to a warning and simply skip the tests if the flag is not set.

Oh so there is a difference between having a workspace that has all members in their own directories (how I initially had this PR) compared to just adding a workspace section to the manifest of the "main" crate (how PR is now). With sub directories cargo test from repo root runs tests for all crates but with one main crate the --all (or --workspace) flag is needed to run the tests for all members. I just know that that is going to bite me again.

tcharding commented 1 year ago

Its not pretty and I'm not sure it is going to be obvious to others exactly why the code in this PR exists but cargo test --all works, as does RUSTFLAGS='--cfg=fuzzing' cargo test --all.