-
Notifications
You must be signed in to change notification settings - Fork 673
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
Clean up the manifest files of top level crates #1612
Clean up the manifest files of top level crates #1612
Conversation
77f47db
to
7cfa8a0
Compare
In b48dfd1 I think we should enable std by default across our crates. I'm open to debate about this. I find it surprising when crates don't have std features on by default, and when I'm doing no-std stuff I'm accustomed to automatically adding In 7cfa8a0 I think you should leave the "Please don't forget" note in. Yes, clearly we've all forgotten nonetheless, but without that note, would you have remembered to do it while making this PR? :) Otherwise looks good to me. Lots of stuff here. |
Cool, I don't have a view on the policy of "std" as default. Before I rush out and change it did you have some specific thoughts on the matter @Kixunil when you left it out of
I'll put a comment back in, no problem. FTR I fixed this because I finally fully groked what the |
7cfa8a0
to
c4b3927
Compare
Force pushed a docs change in the modified last patch. Will give Kix time to weigh in before pushing the default std change. |
Yes, |
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.
Apart from capitalization this looks OK.
repository = "https://github.com/rust-bitcoin/rust-bitcoin/" | ||
documentation = "https://docs.rs/bitcoin/" | ||
description = "General purpose library for using and interoperating with Bitcoin and other cryptocurrencies." | ||
description = "General purpose library for using and interoperating with Bitcoin." |
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.
❤️
c4b3927
to
e45a5e4
Compare
Force push changes the final patch to use |
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 e45a5e4
We can check which files are included in the packaged release with `cargo package --list `. Add an `exclude` section to each manifest that excludes `tests/` and `contrib/`. Not all crates have a `tests/` directory yet but they should so add the exclude anyway to future proof the crates.
Improve all manifest package sections by doing: - Order the list of options uniformly - Remove unnecessary homepage option (currently same as repo) - Add categories section
`core2` is for Read/Write, nothing to do with allocation and we do not use the "alloc" feature of `core2` in `hashes`. Fix core2 dependency/features by doing: - Explicitly enable "bitcoin_hashes/core2" in `bitcoin`. - Do not enable "core2/alloc" in `hashes`
In `bitcoin` when we use the `core2` dependency we always need the "alloc" feature. Enabling "alloc" when enabling "core2" in the "no-std" feature is confusing because it makes it seem that we don't always need it. Set usage of the "alloc" feature of `core2` in the `features` list.
e45a5e4
to
c339d0c
Compare
Force push fixes the categories capitalization, no other changes. |
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 c339d0c
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.
Apart from being unsure about std
not being default in hashes
, this looks good.
hashes/Cargo.toml
Outdated
# If you disable std, you can still use a Write trait via the core2 feature. | ||
# You can also use ToHex via the alloc feature, as it requires Vec/String. | ||
# And you can still just disable std by disabling default features, without enabling these two. | ||
default = [] |
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.
Why is this still here? IIUC bitcoin_hashes
is not considered internal, and hex
definitely isn't.
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.
To be clear -- you are suggesting we have default = ["std"]
for hashes?
I agree, but I don't think it's clear from your earlier comments here.
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, that's what I meant. "nothing else should depend on it" doesn't apply to anything that doesn't have -internals
, -private
or a similar suffix. :)
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.
Done, excuse me for being slow but I don't understand the policy for when to use "std" as default, or if its not policy what's the technical reasoning. If anyone has time please tell me [like I'm 5]
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.
We're trying to optimize "use the library in most projects without too much hassle". For downstream libraries having default features is a hassle (assuming those libraries don't necessarily need them). For most downstream binaries not having std
by default is hassle since they probably use std
anyway. For embedded projects std
is hassle.
We assume that most downstream projects are binaries.
bitcoin
, bitcoin_hashes
, secp256k1
, hex
, bech32
and others have maintained public API, intended to be used by any downstream project, so making std
the default makes sense. bitcoin-internals
have zero downstream binaries (apart from potential fools using it but we aren't obligated to support them) and is only used by our libraries so adding defaults is a hassle for us and doesn't help anyone (except the fools).
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.
Awesome, got it. Thanks man
Improve std/alloc features by doing: - Fix incorrect feature enabling: "std" should enable "internals/std" not "internals/alloc". - Put the "alloc" feature under the "std" feature, same as in other crates. - Remove the comment, devs should understand what dependencies are for. We don't have comments everywhere else so we don't really need this one.
Clean up the optional dependencies by doing: - Put the optional dependencies below the non-optional dependencies as is customary, separated by a line of whitespace. - Put `optional = true` as the last item so as to be uniform - Put `core2` at the top, aids reading because of how the comments are distributed (`core2` does not have a comment on it).
rustdoc build emits a few instances of: warning: this URL is not a hyperlink
Many moon ago we were unable to build with `--all-features`, this is no longer the case. Instruct docs.rs to build the docs with `--all-features`.
c339d0c
to
8596e40
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 8596e40
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 8596e40
Do a complete overhaul of the manifest of the top level crates (i.e., not
embedded
,fuzzing
ect.).Many of the problems being fixed here were introduced over the last year by my poor understanding of exactly what was going on with every line of code in the manifests, after this PR I hope that is no longer a problem.
I'm closing #1571 because it is now done more fully at the end of this PR.
During review please be liberal with any questions so we can ensure everything is spot on now - as we add more crates there are going to be a proliferation of manifest files, best get it right now.