-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Prevent serde from bringing in std
unless the std
feature is enabled for this crate
#499
Conversation
Cargo.toml
Outdated
@@ -11,7 +11,7 @@ rust-version = "1.60.0" | |||
|
|||
[dependencies] | |||
arrayvec = { version = "0.7", default-features = false } | |||
serde = { version = "1.0.183", optional = true } | |||
serde = { version = "1.0.183", default-features = false, features = ["alloc", "derive"], optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we dont actually need the alloc
and derive
features at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, potentially no. This got me looking into how serde
is used and it looks like it's actually gated by the std
feature?
https://github.com/paritytech/parity-scale-codec/blob/master/src/compact.rs#L195-L196
Seems like that should change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a change that always includes serde
, even if std
isn't enabled. I think that should work...
Co-authored-by: Bastian Köcher <git@kchr.de>
@eranrund did you checked that it still compiles? |
I did, not seeing any issues. |
I ran into an issue using this crate in a
no_std
build where I wanted to haveserde
enabled, but withoutstd
. The change here addresses that.Thank you!