Properly set rustdoc attr. Closing #2950 #2975
Conversation
|
In order to build the doc like docs.rs would do it (hopefully): RUSTDOCFLAGS="--cfg docsrs" RUSTFLAGS="--cfg docsrs" cargo doc --open --workspace --all-featuresThe |
|
We might consider adding a something to the CI to run this. |
There was a problem hiding this comment.
Thanks for working on this!
It is a bit messy but there isn't actually a whole lot of cfg code in the repository. libp2p-core is probably the biggest user of it.
Are all the items you are annotating actually public API? Because we don't need to put it on the ones that aren't.
core/src/identity.rs
Outdated
| @@ -32,12 +32,15 @@ | |||
| //! (e.g. [ed25519 binary format](https://datatracker.ietf.org/doc/html/rfc8032#section-5.1.5)). | |||
| //! All key types have functions to enable conversion to/from their binary representations. | |||
|
|
|||
| #[cfg(feature = "ecdsa")] | |||
| #[cfg(any(feature = "ecdsa", docsrs))] | |||
There was a problem hiding this comment.
Why are we combining these with any?
Shouldn't the regular feature be enough?
There was a problem hiding this comment.
Yes, you are right. Only if it's compiled with all features though, but yes.
| @@ -35,6 +35,8 @@ | |||
| //! define how to upgrade each individual substream to use a protocol. | |||
| //! See the `upgrade` module. | |||
|
|
|||
| #![cfg_attr(docsrs, feature(doc_cfg))] | |||
There was a problem hiding this comment.
These seems like it is a left over from another attempt at solving this.
There was a problem hiding this comment.
No I don't think so. This turns the required feature gate on. I.e. #[doc(cfg(...))].
There was a problem hiding this comment.
Ah yes, my bad! Thanks for clarifying!
| [package.metadata.docs.rs] | ||
| all-features = true | ||
| rustdoc-args = ["--cfg", "docsrs"] | ||
| rustc-args = ["--cfg", "docsrs"] |
There was a problem hiding this comment.
Can we add a newline so this icon goes away? 😇
| #[cfg(feature = "ecdsa")] | ||
| #[cfg_attr(docsrs, doc(cfg(feature = "ecdsa")))] |
There was a problem hiding this comment.
I wonder whether it is worth it using a macro to remove this duplication. I can see a couple of downsides, not sure how critical they actually are:
- Less flexibility in specifying the rendered feature combination.
- Increased compile-time because the macro needs to run before all our other crates.
- Newcomers will be unfamiliar with the solution.
There was a problem hiding this comment.
I just found doc_auto_cfg which seems to be useful to automate all of this. The magic line seems to be:
#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]See https://github.com/tokio-rs/valuable/pull/80/files for example.
There was a problem hiding this comment.
I will try that, although if we decide to go with this, it would be much easier for me to do that in a new branch (and pull request).
| [package.metadata.docs.rs] | ||
| all-features = true | ||
| rustdoc-args = ["--cfg", "docsrs"] | ||
| rustc-args = ["--cfg", "docsrs"] |
There was a problem hiding this comment.
I think it may be worthwhile to just copy-paste this into all manifests? Otherwise, if we add features later we may forget about this.
We can include a link to this documentation to make this a bit easier to understand.
|
|
Description
I added the necessary rustdoc flags in docs.rs in all crates. It was just easier to do it on all Cargo.toml files, we can remove the addition if there are any subcrates which don't have any cfg's in them.
Links to any relevant issues
This is the solution for #2950, which was detected during development of #2899. Since we remove all default features in #2918 this might me more relevant.
Open Questions
Are you happy with the changes I made to libp2p-core? I will continue, once this is approved. We also might consider, that there is already a stabilization PR for the feature (rust-lang/rust#100883). However this doesn't makes this work here obsolete. We could just do a regex on all code and remove the
cfg_attr(docsrspart and the#![cfg_attr(docsrs, feature(doc_cfg))]. Or we just waitChange checklist
Quite unsure what to do about the changelog here...