chore: fix serde std flags for no-std build #987
Conversation
bins/revme/Cargo.toml
Outdated
| serde = { version = "1.0", features = ["derive", "rc"] } | ||
| serde_json = { version = "1.0", features = ["preserve_order"] } | ||
| serde = { version = "1.0", default-features = false, features = ["derive", "rc"] } | ||
| serde_json = { version = "1.0", default-features = false, features = ["alloc"]} |
There was a problem hiding this comment.
preserve_order was a nice feature, does it work with no_std?
There was a problem hiding this comment.
it works, sry i added back at one place for miss the other :)
There was a problem hiding this comment.
I think preserve_order doesn't really work with no-std because of https://github.com/serde-rs/json/blob/master/Cargo.toml#L58 . That's why I had to remove it in cbbfe9c in order to make revme no-std. But that should be fine in this repo because as pointed out it's only required by revme which is a separate tool, also I don't think it would be possible to fully make revme no-std because of c-kzg.
There was a problem hiding this comment.
They have PR for it: serde-rs/json#1101 still not merged
Will answer about its usefulness here #987 (comment)
bins/revme/Cargo.toml
Outdated
| @@ -16,17 +16,20 @@ indicatif = "0.17" | |||
| plain_hasher = "0.2" | |||
| revm = { path = "../../crates/revm", version = "3.5.0", default-features = false, features = [ | |||
| "ethersdb", | |||
| "std", | |||
| "serde", | |||
| "c-kzg", | |||
There was a problem hiding this comment.
c-kzg is hard to build in no_std environment, it is c lib. But c-kzg is needed for the test so it needs to be included.
I am fine with making revme std only as it is binary and more like a tool/utility.
There was a problem hiding this comment.
Maybe it can be a test only / bench only dependencies?
There was a problem hiding this comment.
I looked again, and it does make sense to keep this on std cuz it's a command-line app. I'm gonna revert changes for revme
| serde = { version = "1.0", features = ["derive", "rc"], optional = true } | ||
| serde_json = { version = "1.0", features = ["preserve_order"], optional = true } | ||
| serde = { version = "1.0", default-features = false, features = ["derive", "rc"], optional = true } | ||
| serde_json = { version = "1.0", default-features = false, features = ["alloc", "preserve_order"], optional = true } |
There was a problem hiding this comment.
@rakita how critical is having preserve_order here?
There was a problem hiding this comment.
It seems not at all critical, I thought it is there to format output, but after checking, this is not the case. I am okay with removing it.
There was a problem hiding this comment.
Just curious, why would you need binary to be no_std compatible?
There was a problem hiding this comment.
My need (and in this case @CeciliaZ030 's as well) is not the binary per se, but rather revm+serde. We want no-std revm to make ZK proofs for it with powdr , which works fine. However it would be great to also be able to enable the serde feature in revm, in order to have better UX in message passing between the host and the guest when making ZK proofs.
There was a problem hiding this comment.
Makes sense, I don't focus a lot of my attention on revme (binary) as it is used only for testing, was curious to see if I need to revise that view.
Okay, I just noticed that preserve_order is in both revm and revme. preserved_order is nice in revm as it formats trace json output that is easier to read, still okay with removing it.
Can you make a PR?
There was a problem hiding this comment.
Sure! We can also wait to see if serde-rs/json#1101 gets merged, then we don't need to remove it.
There was a problem hiding this comment.
Your call, if this blocks you or you can use this right now it would be best to make PR
No description provided.