-
Notifications
You must be signed in to change notification settings - Fork 23
feat: enable no_std compilation and check it in CI
#93
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
base: main
Are you sure you want to change the base?
Conversation
| bitcoin_hashes = { version = "0.19", default-features = false } | ||
| hex-conservative = { version = "1.0.0", default-features = false } | ||
| serde = { version = "1.0", features = ["derive"], optional = true } | ||
| hashbrown = "0.16.1" |
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.
Just to make sure: you can't disable a dependency if a feature is on, right? You can only enable them. Because now we have at least one non rust-bitcoin dependency that's not feature-gated. I guess it's ok, since hashbrown is super small and is the basis for std's hashmap implementation
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.
Yes, sort of ugly. Otherwise we would have to create a no-std or hashbrown feature required for no-std.
But as you say, std::collections::HashMap is wrapping the hashbrown one: https://doc.rust-lang.org/src/std/collections/hash/map.rs.html
This commit adds two new dependencies that are needed for `no_std` support: `hashbrown` (`HashMap` alternative) and `bitcoin_io` (`io` trait and type replacements). The `AccumulatorHash` trait bounds depend on whether we use `std` or `no_std` now.
606db6e to
21ae740
Compare
|
Pushed 21ae740
And very important:
|
|
@JoseSK999 you could also add a |
I had extended the test:
cargo +nightly test --no-default-features
cargo +nightly test --all-features
cargo +stable test --no-default-features
cargo +stable test --all-featuresand use it in CI? (not sure) |
|
Oh, I thought I had changed CI to use the justfile's recipes. I think that is a better approach (I do it on my own repos). Thoughts @Davidson-Souza |
|
CI here uses |
|
I guess that |
This ensures skipping the `std` feature works as expected even though some traits and types are from `hashbrown` and `bitcoin_io`. Also runs doctests and clippy. Created `test-stable` and `test-nightly` recipes for use in CI.
21ae740 to
8101330
Compare
|
So I have made
|
Description
This PR adds two new dependencies that are needed for
no_stdsupport:hashbrown(HashMapalternative) andbitcoin_io(iotrait and type replacements).The
AccumulatorHashtrait bounds depend on whether we usestdorno_stdnow.Note
This seems to work but perhaps we should add a feature powerset CI check to ensure all 4 combinations work fine for clippy and testing.
The
preludelist is fromfloresta-common, I think I can trim a couple of these imports.