*: Use `auto_doc_cfg` instead of `doc(cfg)` attributes #2983

umgefahren · 2022-10-05T08:55:47Z

Description

In developing #2975 we discovered that doc_cfg_auto is ready for use by our crate. Therefore we are using it to document items behind feature flags.

Links to any relevant issues

This closes #2950.

Open Questions

Should we add the doc_cfg_auto to every crate?

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

umgefahren · 2022-10-05T09:16:11Z

Could you add the hacktoberfest-accepted label? @thomaseizinger

umgefahren · 2022-10-05T09:18:22Z

I'm quite unsure about the changelogs here. We probably have to add an entry to every crate.

thomaseizinger

I don't think we need changelog entries for this.

Can you please:

Investigate, whether we can now remove the existing doc(cfg(...) uses
Post some screenshots of what the docs now look like compared to before

In reading about doc_auto_cfg, I've seen multiple uses of doc_cfg_hide feature as well which I think is used to fine-tune the output. Can you please spot-check some of our more complex cfg uses (e.g. in the root libp2p crate) and verify that it doesn't look horrendous or try to fix it if it does?

Thanks for working on this 😊

umgefahren · 2022-10-06T09:24:58Z

"Looking horrendous" is a quite difficult standard. If we remove all custom doc(cfg( we get more complicated flags, but they are also more representative of the truth. So I think it's hard to judge here. I would be in favor of just abolishing doc(cfg( at the cost of maybe more complicated flags.

You requested some screenshots, so here we go:

Just `doc_auto_cfg`

Modules

Development transports

As it was before (just with `doc_cfg`)

Modules

Development transports

With `doc_auto_cfg` and `doc_cfg_hide`

This didn't really work or maybe I didn't understand something. If we want that effect I would go with the existing manual written doc_cfg on the main crate and everywhere we have them.

My Recommendation

If we really want simple flags (just for the features) we should just use the doc_cfg where they exist. Everywhere else doc_auto_cfg is probably fine. I can't see any reason to use doc_auto_cfg apart from hiding some cfgs from the library user (which i think is misleading).

On a little side note: For some reason there is no incremental compilation on this, so every reiteration takes some time.

Another note: We seem to be using #[cfg(not(any(target_os = "emscripten", target_os = "wasi", target_os = "unknown")))] all the time which results in a massive flag. I might be mistaken, but it probably would just be easier to use: #[cfg(not(any(target_os = "unknown", target_family = "wasm")))] if that's really what we want. This results in this:

Modules

Development transports

thomaseizinger · 2022-10-06T23:11:56Z

Okay, thanks a lot for this work!

Given the screenshots, I am in favor of removing all doc(cfg) attributes for just letting auto_cfg_doc do its job. We can fine-tune later if people complain that something doesn't make sense :)

…p2p into use-rustdoc-auto-cfg

umgefahren · 2022-10-10T15:05:02Z

Anything to do for me?

thomaseizinger

Sorry for the delay, just two more comments 😊

misc/keygen/Cargo.toml

misc/quickcheck-ext/Cargo.toml

umgefahren · 2022-10-10T21:27:17Z

Done 🙂

thomaseizinger

This looks good to me!

@mxinden Any objections in merging this?

misc/keygen/Cargo.toml

misc/quickcheck-ext/Cargo.toml

transports/plaintext/Cargo.toml

mxinden

Looks good to me, minus the one comment from @jxs.

@melekes and @elenaf9 can you do the corresponding changes in the new crates you are introducing?

thomaseizinger · 2022-10-15T01:39:15Z

Looks good to me, minus the one comment from @jxs.

@melekes and @elenaf9 can you do the corresponding changes in the new crates you are introducing?

WebRTC doesn't have any feature-flags at this point. We could gate it all behind the tokio one though as no other implementation exists.

melekes · 2022-10-16T08:37:22Z

Looks good to me, minus the one comment from @jxs.
@melekes and @elenaf9 can you do the corresponding changes in the new crates you are introducing?

WebRTC doesn't have any feature-flags at this point. We could gate it all behind the tokio one though as no other implementation exists.

Soo do I need to make any changes or not? 😐

Co-authored-by: João Oliveira <hello@jxs.pt>

elenaf9 · 2022-10-16T10:10:38Z

src/lib.rs

@@ -182,20 +169,16 @@ pub mod tcp {
    pub use libp2p_tcp::*;


Sorry if that was already discussed, but could you explain again why the #[cfg_attr(docsrs, doc(cfg(feature = "..")))] flag is not removed for the crates that have extra features (like tcp/ dns / ..)?

It hasn't been discussed, well spotted!

I think those should be removed too, yes! Line 166 here for example.

In other words, every manually added #[cfg_attr(docsrs, doc(cfg(feature = "..")))] should be removed? I'm assuming yes, sorry that I missed that.

I did most of this task with Ripgrep, fd and neovim search and replace, so that might be the reason why i missed that. Thanks for the manual review :)

thomaseizinger · 2022-10-16T22:59:14Z

Looks good to me, minus the one comment from @jxs.
@melekes and @elenaf9 can you do the corresponding changes in the new crates you are introducing?

WebRTC doesn't have any feature-flags at this point. We could gate it all behind the tokio one though as no other implementation exists.

Soo do I need to make any changes or not? neutral_face

I would be in favor. Especially with tokio, it is important that the socket is polled on a thread with an active reactor. Can you add put the transport into a libp2p_webrtc::tokio module and feature gate that on tokio?

thomaseizinger

A few more comments but otherwise ready I believe!

Thank you!

protocols/dcutr/Cargo.toml

protocols/relay/Cargo.toml

thomaseizinger · 2022-10-16T23:02:32Z

src/lib.rs

@@ -182,20 +169,16 @@ pub mod tcp {
    pub use libp2p_tcp::*;


It hasn't been discussed, well spotted!

I think those should be removed too, yes! Line 166 here for example.

melekes · 2022-10-17T11:38:10Z

Looks good to me, minus the one comment from @jxs.
@melekes and @elenaf9 can you do the corresponding changes in the new crates you are introducing?

WebRTC doesn't have any feature-flags at this point. We could gate it all behind the tokio one though as no other implementation exists.

Soo do I need to make any changes or not? neutral_face

I would be in favor. Especially with tokio, it is important that the socket is polled on a thread with an active reactor. Can you add put the transport into a libp2p_webrtc::tokio module and feature gate that on tokio?

done in f7c8ab5

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

thomaseizinger

Two minor comments, otherwise LGTM!

muxers/yamux/Cargo.toml

transports/uds/src/lib.rs

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

mxinden

Looks good to me. Thanks @umgefahren and @thomaseizinger for the work.

I suggest we tackle libp2p/test-plans#59 before merging here. Otherwise good to go from my end.

Includes necessary package metadata to improve documentation on docs.rs (shows which items are behind a cfg). Read more on: #2983 Pull-Request: #4599.

Includes necessary package metadata to improve documentation on docs.rs (shows which items are behind a cfg). Read more on: libp2p#2983 Pull-Request: libp2p#4599.

Co-authored-by: João Oliveira <hello@jxs.pt> Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Adding doc_cfg_auto support to crate and all subcrates.

2277c39

thomaseizinger added the hacktoberfest-accepted label Oct 6, 2022

thomaseizinger reviewed Oct 6, 2022

View reviewed changes

umgefahren added 2 commits October 6, 2022 10:24

Merge branch 'master' into use-rustdoc-auto-cfg

4048ad7

Just doc_auto_cfg

43220c9

umgefahren added 2 commits October 7, 2022 19:13

Merge branch 'use-rustdoc-auto-cfg' of github.com:umgefahren/rust-lib…

00d647f

…p2p into use-rustdoc-auto-cfg

Merge branch 'master' into use-rustdoc-auto-cfg

f5443fa

thomaseizinger reviewed Oct 10, 2022

View reviewed changes

misc/keygen/Cargo.toml Outdated Show resolved Hide resolved

misc/quickcheck-ext/Cargo.toml Outdated Show resolved Hide resolved

Removed flags from crates we don't publish

c15ee43

thomaseizinger changed the title ~~Adding doc_cfg_auto support to crate and all subcrates.~~ *: Use auto_doc_cfg instead of doc(cfg) attributes Oct 10, 2022

thomaseizinger approved these changes Oct 11, 2022

View reviewed changes

jxs reviewed Oct 12, 2022

View reviewed changes

misc/keygen/Cargo.toml Outdated Show resolved Hide resolved

misc/quickcheck-ext/Cargo.toml Outdated Show resolved Hide resolved

transports/plaintext/Cargo.toml Show resolved Hide resolved

mxinden reviewed Oct 14, 2022

View reviewed changes

thomaseizinger mentioned this pull request Oct 15, 2022

.github/workflows: Enforce semver compliance with cargo semver-checks #2647

Merged

4 tasks

umgefahren and others added 4 commits October 16, 2022 11:03

Update misc/keygen/Cargo.toml

0549a55

Co-authored-by: João Oliveira <hello@jxs.pt>

Update misc/quickcheck-ext/Cargo.toml

592026c

Co-authored-by: João Oliveira <hello@jxs.pt>

Added missing cfg attr

7f4b185

Merge branch 'master' into use-rustdoc-auto-cfg

ea6595e

elenaf9 reviewed Oct 16, 2022

View reviewed changes

elenaf9 added a commit to kpp/rust-libp2p that referenced this pull request Oct 16, 2022

transports/quic: follow libp2p#2983

3982a6c

thomaseizinger reviewed Oct 16, 2022

View reviewed changes

umgefahren and others added 4 commits October 18, 2022 08:28

Update protocols/relay/Cargo.toml

22399d2

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Update protocols/dcutr/Cargo.toml

1532346

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Merge branch 'master' into use-rustdoc-auto-cfg

6327cca

Removed unnecessary usages of the doc feature

caefc13

thomaseizinger approved these changes Oct 19, 2022

View reviewed changes

muxers/yamux/Cargo.toml Outdated Show resolved Hide resolved

transports/uds/src/lib.rs Outdated Show resolved Hide resolved

umgefahren and others added 2 commits October 19, 2022 11:52

Update transports/uds/src/lib.rs

fb5d706

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Update muxers/yamux/Cargo.toml

2befe25

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

thomaseizinger requested review from mxinden and elenaf9 October 19, 2022 13:11

mxinden approved these changes Oct 20, 2022

View reviewed changes

umgefahren and others added 3 commits October 20, 2022 23:43

Merge branch 'master' into use-rustdoc-auto-cfg

345b464

Merge branch 'master' into use-rustdoc-auto-cfg

7c7adae

Merge branch 'master' into use-rustdoc-auto-cfg

2a15129

thomaseizinger merged commit fcadc83 into libp2p:master Oct 24, 2022

umgefahren deleted the use-rustdoc-auto-cfg branch October 24, 2022 07:56

umgefahren restored the use-rustdoc-auto-cfg branch October 24, 2022 07:56

umgefahren deleted the use-rustdoc-auto-cfg branch October 24, 2022 07:56

umgefahren restored the use-rustdoc-auto-cfg branch October 24, 2022 07:56

umgefahren mentioned this pull request Oct 8, 2023

chore(upnp, webrtc): add missing docs.rs metadata #4599

Merged

1 task

mergify bot pushed a commit that referenced this pull request Oct 9, 2023

chore(upnp, webrtc): add missing docs.rs metadata

3458950

Includes necessary package metadata to improve documentation on docs.rs (shows which items are behind a cfg). Read more on: #2983 Pull-Request: #4599.

umgefahren added a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024

*: Use auto_doc_cfg instead of doc(cfg) attributes (libp2p#2983)

6bae946

Co-authored-by: João Oliveira <hello@jxs.pt> Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Use `auto_doc_cfg` instead of `doc(cfg)` attributes #2983

*: Use `auto_doc_cfg` instead of `doc(cfg)` attributes #2983

umgefahren commented Oct 5, 2022

umgefahren commented Oct 5, 2022

umgefahren commented Oct 5, 2022

thomaseizinger left a comment

umgefahren commented Oct 6, 2022

thomaseizinger commented Oct 6, 2022

umgefahren commented Oct 10, 2022

thomaseizinger left a comment

umgefahren commented Oct 10, 2022

thomaseizinger left a comment

mxinden left a comment

thomaseizinger commented Oct 15, 2022

melekes commented Oct 16, 2022

elenaf9 Oct 16, 2022 •

edited

Loading

thomaseizinger Oct 16, 2022

umgefahren Oct 18, 2022 •

edited

Loading

thomaseizinger commented Oct 16, 2022

thomaseizinger left a comment

thomaseizinger Oct 16, 2022

melekes commented Oct 17, 2022

thomaseizinger left a comment

mxinden left a comment

*: Use auto_doc_cfg instead of doc(cfg) attributes #2983

*: Use auto_doc_cfg instead of doc(cfg) attributes #2983

Conversation

umgefahren commented Oct 5, 2022

Description

Links to any relevant issues

Open Questions

Change checklist

umgefahren commented Oct 5, 2022

umgefahren commented Oct 5, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

umgefahren commented Oct 6, 2022

Just doc_auto_cfg

Modules

Development transports

As it was before (just with doc_cfg)

Modules

Development transports

With doc_auto_cfg and doc_cfg_hide

My Recommendation

Modules

Development transports

thomaseizinger commented Oct 6, 2022

umgefahren commented Oct 10, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

umgefahren commented Oct 10, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

thomaseizinger commented Oct 15, 2022

melekes commented Oct 16, 2022

elenaf9 Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

thomaseizinger Oct 16, 2022

Choose a reason for hiding this comment

umgefahren Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

thomaseizinger commented Oct 16, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Oct 16, 2022

Choose a reason for hiding this comment

melekes commented Oct 17, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

*: Use `auto_doc_cfg` instead of `doc(cfg)` attributes #2983

*: Use `auto_doc_cfg` instead of `doc(cfg)` attributes #2983

Just `doc_auto_cfg`

As it was before (just with `doc_cfg`)

With `doc_auto_cfg` and `doc_cfg_hide`

elenaf9 Oct 16, 2022 •

edited

Loading

umgefahren Oct 18, 2022 •

edited

Loading