Remove no-std and actual-serde features #769
Conversation
Cargo.toml
Outdated
| bitcoin = { version = "0.32.0", default-features = false } | ||
|
|
||
| # Do NOT use this as a feature! Use the `serde` feature instead. | ||
| actual-serde = { package = "serde", version = "1.0.103", optional = true } |
There was a problem hiding this comment.
By looking at rust-bitcoin it looks like this can be renamed to just plain old serde (the actual-serde was only used because we didn't have "dep:serde" available).
Will also need
#[cfg(feature = "serde")]
-pub use actual_serde as serde;
+pub use serde;There was a problem hiding this comment.
Right, good point!
README.md
Outdated
| The library can be built and tested using [`cargo`](https://github.com/rust-lang/cargo/): | ||
|
|
||
| ``` | ||
| git clone git@github.com:rust-bitcoin/rust-miniscript.git | ||
| cd rust-miniscript | ||
| cargo build | ||
| ``` | ||
|
|
||
| You can run tests with: | ||
|
|
||
| ``` | ||
| cargo test | ||
| ``` | ||
|
|
||
| Please refer to the [`cargo` documentation](https://doc.rust-lang.org/stable/cargo/) for more | ||
| detailed instructions. |
There was a problem hiding this comment.
These changes are unrelated to this PR, right? (FTR I never liked these docs in rust-bitcoin, if one doesn't know how to run Rust unit tests they probably shouldn't be writing financial software - and rust-miniscript is even more hardcore than rust-bitcoin.
There was a problem hiding this comment.
Yeah. The Building section had more relevant info about how to select the no-std/std features, but now it is just one line (I added that to make it more complete).
Technically the no_std support is already explained in High-Level Features and doesn't require more in-depth explanation, so based on your comment I think it makes sense to remove Building.
There was a problem hiding this comment.
They're not totally unrelated because they replace docs that talked about nostd.
Though agreed that we probably don't need to tell people to run cargo test if there's not even any env variables or anything :). If this were end-user software then sure, but this is a Rust library that you can't use without being familiar with the language and its ecosystem.
| you may need to pin some dependencies. See `./contrib/test.sh` for current pinning. | ||
| you may need to pin some dependencies. See `./contrib/pin.sh` for current pinning. | ||
|
|
There was a problem hiding this comment.
This change is good, its an unrelated change for this PR though. (Sorry for being picky, we just uncovered a bug in rust-bitcoin that slipped through review so I'm trying to review more anally diligently.
There was a problem hiding this comment.
Thanks, no worries about being thorough. It is unrelated, but I figured it was okay since it’s just an outdated path in the README.
| # Test all these features with "no-std" enabled. | ||
| # rust-miniscript only: https://github.com/rust-bitcoin/rust-miniscript/issues/681 | ||
| FEATURES_WITH_NO_STD="compiler trace serde rand base64" | ||
|
|
||
| # Test all these features without "std" enabled. |
There was a problem hiding this comment.
Mad! If this merges we can patch rust-bitcoin-maintainer-tools to remove the logic that supports this.
JoseSK999
force-pushed
the
remove-no-std-feature
branch
from
8722d58
to
8372e8c
Compare
|
Updated with @tcharding comments |
|
CI failure is unrelated (will be fixed by #766). |
|
Thanks bro. |
|
ACK. Can you rebase to get the CI fix in? Or maybe can @apoelstra directly merge after running tests on merge commit? |
The `no-std` feature is no longer needed with `bitcoin` version 0.32.0 - Update README.md, including updating to the `./contrib/pin.sh` path - Disable `actual-serde` implicit feature
JoseSK999
force-pushed
the
remove-no-std-feature
branch
from
8372e8c
to
ccc2815
Compare
|
Rebased! |
Closes #681
The
no-stdfeature is no longer needed withbitcoinversion 0.32.0rust-bitcoin)actual-serdeimplicit featureI think
FEATURES_WITH_NO_STDnow could be removed from the maintainer tools. Let me know if there's something missing or to improve!