Skip to content
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

Update documentation #285

Merged

Conversation

jacksonbrim
Copy link
Contributor

##Summary of the PR
Annotated modules in lib.rs to indicate their feature dependencies such that it is reflected in the docs, enhancing documentation clarity for users on docs.rs.

##Building Documentation Locally:

To build the documentation locally with the specified feature flags, use the following command:

RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --all-features --open

This command sets the necessary RUSTDOCFLAGS to include conditional compilation flags and features specific to our project, ensuring the generated documentation reflects the same level of detail and conditions as seen on docs.rs.

@jacksonbrim jacksonbrim force-pushed the update-documentation branch 4 times, most recently from dd31220 to b830a7a Compare March 19, 2024 19:07
@jacksonbrim jacksonbrim marked this pull request as ready for review March 19, 2024 19:39
Copy link
Collaborator

@roypat roypat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jacksonbrim,
Thank you for your contribution! We've done a similar thing on some other rust-vmm crates (e.g. rust-vmm/linux-loader@26a0d86), so it'd be nice to keep the implementation consistent (e.g. use doc_auto_cfg) across the crates. Could you have a look at the comments below to see if they make sense? Thanks!

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
@@ -29,6 +30,7 @@ pub mod address;
pub use address::{Address, AddressValue};

#[cfg(feature = "backend-atomic")]
#[cfg_attr(docsrs, doc(cfg(feature = "backend-atomic")))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[cfg_attr(docsrs, doc(cfg(feature = "backend-atomic")))]

src/lib.rs Outdated
@@ -63,7 +65,9 @@ mod mmap_xen;
mod mmap_windows;

#[cfg(feature = "backend-mmap")]
#[cfg_attr(docsrs, doc(cfg(feature = "backend-mmap")))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[cfg_attr(docsrs, doc(cfg(feature = "backend-mmap")))]

@jacksonbrim
Copy link
Contributor Author

Hi @jacksonbrim, Thank you for your contribution! We've done a similar thing on some other rust-vmm crates (e.g. rust-vmm/linux-loader@26a0d86), so it'd be nice to keep the implementation consistent (e.g. use doc_auto_cfg) across the crates. Could you have a look at the comments below to see if they make sense? Thanks!

Yes, that looks much better.

Copy link
Collaborator

@roypat roypat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Cargo.toml Outdated
@@ -44,3 +44,4 @@ codegen-units = 1

[package.metadata.docs.rs]
all-features = true
rustdoc-args = ["--cfg", "docsrs", "--cfg", "feature=\"backend-mmap\"", "--cfg", "feature=\"backend-atomic\""]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that rustdoc-args enables features in the rustdoc binary, so this only needs to be rustdoc-args = ["--cfg", "docsrs"]. The features backend-mmap etc. will be enabled by all-features = true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen the later commit removes these, I think in this case it would make sense to squash the commits in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, we don't get paid by the commit?

Squashed it. Thanks for the patience.

* Add feature flag doc generation in lib.rs with auto_doc_cfg
* Update Cargo.toml with rustdoc-args in [package.metadata.docs.rs] for docs.rs build.

Local Doc Build Command:
```
RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --all-features --open
```

Signed-off-by: Jackson Brim <jackson.cr.brim@gmail.com>
@JonathanWoollett-Light JonathanWoollett-Light merged commit 236afa4 into rust-vmm:main Mar 22, 2024
2 checks passed
@roypat roypat mentioned this pull request Sep 3, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants