-
Notifications
You must be signed in to change notification settings - Fork 279
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
Revert: Set default-features = false for rust-bitcoin and rust-miniscript #897
Revert: Set default-features = false for rust-bitcoin and rust-miniscript #897
Conversation
This reverts commit 87c558c.
e8f92c2
to
3466393
Compare
74a39c0
to
6ffed05
Compare
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.
ACK.. One comment..
miniscript = { version = "9.0", default-features = false, features = ["serde"] } | ||
bitcoin = { version = "0.29.2", default-features = false, features = ["serde", "base64", "rand"] } | ||
miniscript = { version = "9.0", features = ["serde"] } | ||
bitcoin = { version = "0.29.1", features = ["serde", "base64", "rand"] } |
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.
Is it intentional to downgrade bitcoin
to 0.29.1
? I ran some tests with latest version and all seemed to work well with it. Am I missing something..
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.
This was part of the original commit and since I used the git revert
command undid the bitcoin 0.29.2 change too. To keep it simple I'd rather not make any other changes in this PR but to revert the original commit (and corresponding changelog entry). Also even if we leave this at 0.29.1 anyone who does a cargo update
will still get the latest 0.29.2 version.
@@ -14,8 +14,8 @@ license = "MIT OR Apache-2.0" | |||
[dependencies] | |||
bdk-macros = "^0.6" | |||
log = "^0.4" | |||
miniscript = { version = "9.0", default-features = false, features = ["serde"] } |
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.
Mh, rather than outright reverting it, could we maybe leave default-features = false
and set miniscript/std
in BDK's default
feature? This way downstream projects could opt-out of std
by setting default-features = false
for BDK.
@benthecarman Would this work for you?
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.
Yeah that would work
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.
This should work but will still force any downstream projects that use bdk
and have default-features = false
to now have to also enable a new std
feature. That's the kind of breaking change I want to avoid on a patch release. Ideally I want to defer all breaking changes to the 1.0 release.
Besides Mutiny are there any other downstream projects that need to disable std
for bitcoin
and miniscript
?
6ffed05
to
751ba3c
Compare
751ba3c
to
9bc8cf9
Compare
In another PR I mentioned that we should always just enable the |
I'm closing this PR in favor of bumping the maintenance release version to 0.28.0 and add a default |
Description
Since the
bitcoin
andminiscript
dependencies require enabling eitherstd
orno-std
the only way we can make this change is to add a correspondingstd
andno-std
features and requiring our downstream users to enable one. But BDK doesn't really supportno-std
yet and we don't want to break downstream projects on a patch release. So unfortunately I need to revert #882.See also: rust-bitcoin/rust-miniscript#533
Notes to the reviewers
I've also yanked release
0.27.2
on crates.io and will be sure to test future releases with downstream bdk-cli and bdk-ffi projects to catch any issues like this.Changelog notice
None
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committing