Document cargo features by Kixunil · Pull Request #633 · rust-bitcoin/rust-bitcoin

Kixunil · 2021-07-28T19:53:14Z

This documents cargo features in two ways: explictly in text and in code
using #[doc(cfg(...))] attribute where possible. Notably, this is
impossible for serde derives. The attribute is contitional and only
activated for docs.rs or explicit local builds.

This change also adds package.metadata.docs.rs field to Cargo.toml
which instructs docs.rs to build with relevant features and with
docsrs config activated enabling `#[doc(cfg(...))] attributes.

I also took the opportunity to fix a few missing spaces in nearby code.

Notes for reviewers:

To build and look at result locally run: RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --features std,secp-recovery,base64,rand,use-serde,bitcoinconsensus --open (don't confuse RUSTDOCFLAGS with RUSTFLAGS - took me a while to figure that out 😞)
You should see needed features being called out - e.g. in Script::verify
More information and some screenshots: cargo doc: include all available features in generated docs. rust-lang/cargo#8103 (comment)
For an example why this matters see: Use rust-bitcoin feature instead of base64 dep Kixunil/loptos#1 😉
Serde issue: Enable documenting conditional derives serde-rs/serde#2063

src/lib.rs

src/util/amount.rs

Cargo.toml

dr-orlovsky

Thank you for a lot of careful work on this thing!

utACK, even if there are valid Nits in my comments, they can be addressed later in a separate PR, since any improvements to the docs exposing the items behind feature gates are better than nothing.

Two main comments:

Seems like we need to re-base onto master to take into account all changes in feature use from no_std 0.27 version (or do a follow-up PR to this one)
I will test this locally if all of the existing feature gates are exposed to docs once all of the commits in PR will pass CI and compile: @apoelstra usually puts this as a requirement for merging any PR into the rust-bitcoin. You can see more in PR #587, where I also had included the test script

dr-orlovsky · 2021-08-09T19:55:26Z

src/lib.rs

Seems we are missing alloc feature here, introduced by recent PR.

You mean no-std which is documented below? I don't see any alloc PR.

Right, I mixed up with bitcoin_hashes which does have

dr-orlovsky · 2021-08-09T19:56:53Z

src/util/amount.rs

Finally somebody started introducing practice of [`Type`] instead of [Type]

dr-orlovsky · 2021-08-09T19:58:28Z

src/util/misc.rs

Not sure why we have a feature gate here, when we anyway re-export them into public space above via a feature-gated export

TBH, I somewhat blindly added the attribute to each instance of #[cfg] I ripgreped. I could remove it but I'm also thinking keeping it may be good way to avoid accidentally messing up in refactors.

dr-orlovsky · 2021-08-09T20:00:36Z

src/util/misc.rs

dr-orlovsky · 2021-08-09T20:02:09Z

src/util/psbt/mod.rs

Should't we just feature-gate the mod itself and not specific impls?

I'm not sure rustdoc is that smart and it will do what we want. I could check later.

dr-orlovsky · 2021-08-09T20:02:15Z

src/util/psbt/mod.rs

dr-orlovsky · 2021-08-09T20:02:23Z

src/util/psbt/mod.rs

Kixunil · 2021-08-10T06:48:37Z

Squashed commits.

apoelstra · 2021-09-08T22:02:08Z

src/blockdata/script.rs

Lol whoops, we definitely should not have feature-gated enum variants. Surprised nobody has complained about this.

Looks like Tamás and I did this in March 2018.

Nothing to do with this PR but we should fix it in another one.

Yeah, for normal enums that'd be breaking because some crate could do exhaustive match on it while other crate turns on the feature. I think it'd be fine for #[non_exhaustive] enums.

Opened #645 with a suggested solution.

src/lib.rs

apoelstra

ACK 1e766d22b4c1bd6f080c7a35274ebd05953e44b6

Thank you for all this work! I would appreciate a fix for the typo but won't hold up merging for it.

dr-orlovsky

utACK 0670bdaef490ea100ab4dfb2b7ade829f66a67be

apoelstra · 2021-09-13T21:38:31Z

Needs rebase

This documents cargo features in two ways: explictly in text and in code using `#[doc(cfg(...))]` attribute where possible. Notably, this is impossible for `serde` derives. The attribute is contitional and only activated for docs.rs or explicit local builds. This change also adds `package.metadata.docs.rs` field to `Cargo.toml` which instructs docs.rs to build with relevant features and with `docsrs` config activated enabling `#[doc(cfg(...))] attributes. I also took the opportunity to fix a few missing spaces in nearby code.

Kixunil · 2021-09-14T10:25:38Z

Done.

dr-orlovsky

ACK 95fb4e0

Kixunil · 2021-09-14T15:43:58Z

Hmm, GH notified me that there was a fail but I don't see it here. The failure is a fluke (failed to download Rust).

apoelstra

ACK 95fb4e0

18f74d5 Clarify what does "less security" mean (Martin Habovstiak) 94c55b4 Fixed typos/grammar mistakes (Martin Habovštiak) 1bf0552 Documented features (Martin Habovstiak) Pull request description: This documents the Cargo features making sure docs.rs shows warning for feature-gated items. They are also explicitly spelled out in the crate documentation. The PR is similar in spirit to rust-bitcoin/rust-bitcoin#633 ACKs for top commit: apoelstra: ACK 18f74d5 Tree-SHA512: 8aac3fc5fd8ee887d6b13606d66b3d11ce44662afb92228c4f8da6169e3f70ac6a005b328f427a91d307f8d36d091dcf24bfe4d17dfc034d02b578258719a90a

…error 50db03c cargo fmt (Andrew Poelstra) 220f101 ci: screw up imports for rust-lang/rust#121684 (Andrew Poelstra) df069af ci: remove unnecessary imports of `bitcoin` (Andrew Poelstra) 3725549 ci: tighten import of `Vec` (Andrew Poelstra) d441ccd ci: remove imports that are redundant with super::* (Andrew Poelstra) b87b4fb remove sketchy `LikelyFalse` error (Andrew Poelstra) Pull request description: This error would trigger on `l:0` on the basis that this is equivalent to `u:0` but always less efficient. There are no other "linting" errors like this in this library, and the C++ implementation doesn't have it, so we should drop it. Fixes rust-bitcoin#633 ACKs for top commit: tcharding: ACK 50db03c sanket1729: ACK 50db03c Tree-SHA512: 30681e6abe7957b9fbe059e808d8057fd174ea156d1853960370d2fd63b032a500f85965a5c7424764df76ed804c62d9a781ae38cdc2b123e9b4b53308dd89f5

18f74d52422fc18f0e4c866203d7aa12c8faa3f9 Clarify what does "less security" mean (Martin Habovstiak) 94c55b4d09727433c606301d27173748c8ff36df Fixed typos/grammar mistakes (Martin Habovštiak) 1bf05523f072f833f388a1fe5e5d538fd00119c8 Documented features (Martin Habovstiak) Pull request description: This documents the Cargo features making sure docs.rs shows warning for feature-gated items. They are also explicitly spelled out in the crate documentation. The PR is similar in spirit to rust-bitcoin/rust-bitcoin#633 ACKs for top commit: apoelstra: ACK 18f74d52422fc18f0e4c866203d7aa12c8faa3f9 Tree-SHA512: 8aac3fc5fd8ee887d6b13606d66b3d11ce44662afb92228c4f8da6169e3f70ac6a005b328f427a91d307f8d36d091dcf24bfe4d17dfc034d02b578258719a90a

TheBlueMatt reviewed Jul 28, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Kixunil mentioned this pull request Jul 29, 2021

Removed fuzztarget feature #634

Merged

Kixunil force-pushed the doc-features branch from a97b71c to d345611 Compare August 2, 2021 08:41

RCasatta reviewed Aug 2, 2021

View reviewed changes

src/util/amount.rs Outdated Show resolved Hide resolved

RCasatta reviewed Aug 2, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

dr-orlovsky previously approved these changes Aug 9, 2021

View reviewed changes

dr-orlovsky added this to the 0.28.0 milestone Aug 9, 2021

Kixunil dismissed dr-orlovsky’s stale review via 1e766d2 August 10, 2021 06:48

Kixunil force-pushed the doc-features branch from e03af7a to 1e766d2 Compare August 10, 2021 06:48

apoelstra reviewed Sep 8, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

apoelstra previously approved these changes Sep 8, 2021

View reviewed changes

Kixunil dismissed apoelstra’s stale review via 0670bda September 9, 2021 08:52

Kixunil force-pushed the doc-features branch from 1e766d2 to 0670bda Compare September 9, 2021 08:52

dr-orlovsky previously approved these changes Sep 10, 2021

View reviewed changes

Kixunil dismissed dr-orlovsky’s stale review via 95fb4e0 September 14, 2021 10:25

Kixunil force-pushed the doc-features branch from 0670bda to 95fb4e0 Compare September 14, 2021 10:25

dr-orlovsky approved these changes Sep 14, 2021

View reviewed changes

apoelstra approved these changes Sep 14, 2021

View reviewed changes

apoelstra merged commit 65d8bda into rust-bitcoin:master Sep 14, 2021

Kixunil deleted the doc-features branch September 14, 2021 17:31

Kixunil mentioned this pull request Jan 4, 2022

Documented features rust-bitcoin/rust-secp256k1#353

Merged

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Conversation

Kixunil commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dr-orlovsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Aug 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Sep 13, 2021

Uh oh!

Kixunil commented Sep 14, 2021

Uh oh!

dr-orlovsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Sep 14, 2021

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Kixunil commented Jul 28, 2021 •

edited

Loading

dr-orlovsky left a comment •

edited

Loading

dr-orlovsky left a comment •

edited

Loading