rust-vmm / event-manager

Apache License 2.0
46 stars 32 forks source link

Build docs.rs documentation with `remote_endpoint` feature #119

Closed 00xc closed 1 year ago

00xc commented 1 year ago

Summary of the PR

The current documentation build in docs.rs does not include the remote_endpoint feature, which might lead so users of the crate not being aware of its existence.

For more information on the table introduced in Cargo.toml see: https://docs.rs/about/metadata

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

00xc commented 1 year ago

CI failure should be unrelated.

00xc commented 1 year ago

It would be better to add


[package.metadata.docs.rs]
all-features = true

Do we want to document test_utilities?

rustdoc-args = ["--cfg", "docsrs"]

I'll add this.

JonathanWoollett-Light commented 1 year ago

Do we want to document test_utilities?

I think not. Ideally test_utilities should be removed in the future.

00xc commented 1 year ago

Do we want to document test_utilities?

I think not. Ideally test_utilities should be removed in the future.

all-features = true would include that as far as I can tell. PR should be good now.

JonathanWoollett-Light commented 1 year ago

Do we want to document test_utilities?

I think not. Ideally test_utilities should be removed in the future.

all-features = true would include that as far as I can tell. PR should be good now.

Yes, this is fine it will enable the feature. But we do not need to add #[cfg_attr(docsrs, doc(cfg(feature = "test_utilities")))].

00xc commented 1 year ago

Should be good now. Tested it with RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --all-features --open

JonathanWoollett-Light commented 1 year ago

Like you put https://github.com/rust-vmm/vmm-sys-util/pull/209/files#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R8 I think it also makes sense to use this approach here.

00xc commented 1 year ago

Like you put https://github.com/rust-vmm/vmm-sys-util/pull/209/files#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R8 I think it also makes sense to use this approach here.

That's what I did, except we need the doc_cfg feature as well to be able to do #[cfg_attr(docsrs, doc(cfg(feature = "remote_endpoint")))], in order to omit the test-utilities feature. Otherwise we get:

$ RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --open --all-features
error[E0658]: `#[doc(cfg)]` is experimental
  --> src/lib.rs:31:20
   |
31 | #[cfg_attr(docsrs, doc(cfg(feature = "remote_endpoint")))]
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #43781 <https://github.com/rust-lang/rust/issues/43781> for more information
   = help: add `#![feature(doc_cfg)]` to the crate attributes to enable
JonathanWoollett-Light commented 1 year ago

Like you put https://github.com/rust-vmm/vmm-sys-util/pull/209/files#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R8 I think it also makes sense to use this approach here.

That's what I did, except we need the doc_cfg feature as well to be able to do #[cfg_attr(docsrs, doc(cfg(feature = "remote_endpoint")))], in order to omit the test-utilities feature. Otherwise we get:

$ RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --open --all-features
error[E0658]: `#[doc(cfg)]` is experimental
  --> src/lib.rs:31:20
   |
31 | #[cfg_attr(docsrs, doc(cfg(feature = "remote_endpoint")))]
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #43781 <https://github.com/rust-lang/rust/issues/43781> for more information
   = help: add `#![feature(doc_cfg)]` to the crate attributes to enable

I think if its easier we can include the test-utilities feature.

00xc commented 1 year ago

I think if its easier we can include the test-utilities feature.

It's not that much simpler in my opinion, we are saving two lines of code and using one less feature which isn't even used in regular builds.

JonathanWoollett-Light commented 1 year ago

I think if its easier we can include the test-utilities feature.

It's not that much simpler in my opinion, we are saving two lines of code and using one less feature which isn't even used in regular builds.

Yeah but it is an inconsistent pattern that introduces a special case. Someone will look back and ask why aren't all feature covered. If the test feature is removed in the future it should warrant a PR to change to use the generic approach but this may be forgotten which may leave the code in a state where there is a special case without a reason.

I think it is best to follow the typical pattern here.

00xc commented 1 year ago

Ok, it turns out there is a #![doc(hidden)] for the utilities module, so it won't show up anyway. Updated the PR.