Actual no_std support #1028
Actual no_std support #1028
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1028 +/- ##
==========================================
+ Coverage 90.80% 90.82% +0.02%
==========================================
Files 60 61 +1
Lines 31460 31458 -2
==========================================
+ Hits 28568 28573 +5
+ Misses 2892 2885 -7
Continue to review full report at Codecov.
|
devrandom
force-pushed
the
2021-08-no-std
branch
from
305f723 to
ce1cef6
Compare
|
Completes the work started in #842 |
lightning/Cargo.toml
Outdated
|
|
||
| default = ["std"] | ||
|
|
||
| [dependencies] | ||
| bitcoin = "0.27" | ||
| bitcoin = { version = "0.27", default-features = false, features = ["secp-recovery"] } | ||
| # TODO maybe bitcoin no-std should pull in this feature? |
There was a problem hiding this comment.
Yes, it absolutely should.
There was a problem hiding this comment.
I'll submit that to rust-bitcoin, and clean up here in a followup PR after it's released?
There was a problem hiding this comment.
Opened rust-bitcoin/rust-bitcoin#637 to address this
|
|
||
| [dev-dependencies.bitcoin] | ||
| version = "0.27" | ||
| features = ["bitcoinconsensus"] | ||
| default-features = false |
There was a problem hiding this comment.
Given we rely on libbitcoinconsensus, do we need this?
There was a problem hiding this comment.
Are you saying that libbitcoinconsensus is dependent on std anyway, so there's no point in turning it off? shouldn't that library be made no_std?
There was a problem hiding this comment.
Hmm, yea, I guess it could be made no_std eventually, though I doubt anyone is gonna sit down and fight to get it there.
There was a problem hiding this comment.
So if I remove default-features = false then cargo test --no-default-features --features no_std fails. I think that's because the bitcoin crate has the std feature on, but this crate doesn't, so there's a mismatch in the bitcoin API signatures.
devrandom
force-pushed
the
2021-08-no-std
branch
3 times, most recently
from
4f3eef9 to
34fe1e5
Compare
lightning/src/lib.rs
Outdated
| pub use alloc::borrow::ToOwned; | ||
| pub use alloc::string::ToString; | ||
|
|
||
| pub use io_extras::read_to_end; |
There was a problem hiding this comment.
Just personal preference, but I'm really not a fan of transparently including functions in prelude - it makes it somewhat unclear where they're coming from in code using it. Can we instead export copy from io_extras and then explicitly use copy and read_to_end from io_extras in files that use them?
There was a problem hiding this comment.
Agreed. Moving those and also sink.
lightning/src/lib.rs
Outdated
| #[cfg(any(test, feature = "_test_utils"))] extern crate hex; | ||
| #[cfg(any(test, feature = "fuzztarget", feature = "_test_utils"))] extern crate regex; | ||
|
|
||
| #[cfg(feature = "no_std")] extern crate core2; |
There was a problem hiding this comment.
This line uses core2 under feature = no_std but core2 is also used below as not(feature = std). Should they be unified?
There was a problem hiding this comment.
I wonder if we should almost always use not(feature = std), as, if both are set, we really just want to use std::io because it probably means someone should/will set the core2/std flag.
devrandom
force-pushed
the
2021-08-no-std
branch
from
db5b483 to
c483d85
Compare
|
I addressed the comments. I also pushed a commit that renames the |
matches rust-bitcoin
devrandom
force-pushed
the
2021-08-no-std
branch
from
c483d85 to
32d13a2
Compare
No description provided.