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

Enable Serde for no-std targets #896

Closed
wants to merge 1 commit into from
Closed

Conversation

ass3rt
Copy link

@ass3rt ass3rt commented Mar 21, 2022

Before this patch, Serde still depended on std, and the use-serde
feature of this crate would not compile for pure no-std targets.

Before this patch, Serde still depended on std, and the `use-serde`
feature of this crate would not compile for pure no-std targets.
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

The problem you are hitting (if I understand correctly) is that there is no way to conditionally enable features and sub-features currently in the manifest file. I've hit this myself a bunch of times lately :)

@@ -27,7 +27,7 @@ secp-recovery = ["secp256k1/recovery"]
# Instead no-std enables additional features required for this crate to be usable without std.
# As a result, both can be enabled without conflict.
std = ["secp256k1/std", "bitcoin_hashes/std", "bech32/std"]
no-std = ["hashbrown", "core2/alloc", "bitcoin_hashes/alloc", "secp256k1/alloc"]
no-std = ["hashbrown", "core2/alloc", "bitcoin_hashes/alloc", "secp256k1/alloc", "serde/alloc"]
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work because serde is optional but now no-std enables it.

Copy link
Member

Choose a reason for hiding this comment

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

Oof, good catch. This is extremely misleading syntax on the part of the cargo folks. I stupidly read this as a conditional feature enabling, even though we've had endless discussions about that being impossible..

@@ -41,7 +41,7 @@ core2 = { version = "0.3.0", optional = true, default-features = false }

base64-compat = { version = "1.0.0", optional = true }
bitcoinconsensus = { version = "0.19.0-3", optional = true }
serde = { version = "1", features = [ "derive" ], optional = true }
serde = { version = "1", default-features = false, features = [ "derive" ], optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

With this change, users of std and use-serde no longer get serde/std. I don't know a solution to this problem, conditionally enabling features.

@ass3rt
Copy link
Author

ass3rt commented Mar 21, 2022

Ah, good points. This highlights a limitation of feature selection in Rust. Thanks for the quick feedback. I'll go ahead and close this PR, as its clearly not the solution I thought it was 🤦

I am working on an embedded project (cortex m7), and I would like to use the "official" rust-bitcoin, without forking it if possible. I'll think a bit more to see if there is another way around this hurdle.

@ass3rt ass3rt closed this Mar 21, 2022
@tcharding
Copy link
Member

One work around is to depend on rust-bitcoin with no default features and then turn on features individually. Then use explicit dependencies for all the rust-bitcoin dependencies (rust-secp256k1, bitcoin_hashes ect.) and manage features in the same way. If this does not work please raise an issue because it should :) AFAICT no-std is still work in progress so any bug reports or issues you raise are most appreciated.

Also, no-std support does not uphold the same MSRV as the rest of the crate.

@ass3rt
Copy link
Author

ass3rt commented Mar 22, 2022

Ah, great suggestion. Seems obvious in retrospect! Thanks 🍻

@ass3rt
Copy link
Author

ass3rt commented Mar 24, 2022

@tcharding, unfortunately your suggestion was not possible, because cargo requires sub-crate features to be re-exported, before a dependent crate can access them.

I've opened an issue to discuss further: #904

@tcharding
Copy link
Member

tcharding commented Mar 24, 2022

I explained further on the issue :)

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.

None yet

3 participants