[RFC PATCH 0/2] Generate API documentation for 'bindings' crate

[RFC PATCH 0/2] Generate API documentation for 'bindings' crate

[Trevor Gross]

The 'bindings' crate currently doesn't have API documentation available.
With this change, it will be generated as part of the output of 'make
rustdoc' (similar to the 'kernel' crate's docs,
https://rust-for-linux.github.io/docs/kernel/).

This patch is a RFC because there are a few questions:

1. Do we want to make this the default, or a separate target/
   configuration? I don't think there is much downside to always
   generating.
2. The entire '.config' file could be included in the doc output, to
   make it easy to tell what settings the documentation was generated
   with. Would this be desired? Maybe with a '--cfg
   include-dotcfg=".config"' flag so published docs would have the
   option (unsure whether it may ever have sensitive information).

Bindgen is currently invoked with '--no-doc-comments', I think this may
be because some blocks were mistakenly interpreted as doctests. Once we
upgrade our bindgen version we might be able to remove this.

Side note, 'rust/Makefile' seems to have a mix of tabs and spaces - is
this intentional?

Trevor Gross (2):
  rust: bindings: generate API documentation for the 'bindings' crate
  rust: bindings: add warning about configuration dependence

 rust/Makefile        | 14 +++++++++++---
 rust/bindings/lib.rs |  3 +++
 2 files changed, 14 insertions(+), 3 deletions(-)

-- 
2.34.1

[RFC PATCH 1/2] rust: bindings: generate API documentation for the 'bindings' crate

[Trevor Gross]

The 'bindings' crate is currently not included in rustdoc output. Add
this crate to documentation output to provide an easy reference when
developing abstractions or inspecting Rust-C bindings.

Signed-off-by: Trevor Gross <tmgross@umich.edu>
---
 rust/Makefile | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/rust/Makefile b/rust/Makefile
index f7c9a6e54c85..b5ce57a50eb5 100644
--- a/rust/Makefile
+++ b/rust/Makefile
@@ -81,7 +81,7 @@ quiet_cmd_rustdoc = RUSTDOC $(if $(rustdoc_host),H, ) $<
 # command-like flags to solve the issue. Meanwhile, we use the non-custom case
 # and then retouch the generated files.
 rustdoc: rustdoc-core rustdoc-macros rustdoc-compiler_builtins \
-    rustdoc-alloc rustdoc-kernel
+    rustdoc-alloc rustdoc-bindings rustdoc-kernel
 	$(Q)cp $(srctree)/Documentation/images/logo.svg $(objtree)/$(obj)/doc
 	$(Q)cp $(srctree)/Documentation/images/COPYING-logo $(objtree)/$(obj)/doc
 	$(Q)find $(objtree)/$(obj)/doc -name '*.html' -type f -print0 | xargs -0 sed -Ei \
@@ -114,12 +114,20 @@ rustdoc-alloc: private rustc_target_flags = $(alloc-cfgs) \
 rustdoc-alloc: $(src)/alloc/lib.rs rustdoc-core rustdoc-compiler_builtins FORCE
 	$(call if_changed,rustdoc)
 
+rustdoc-bindings: private rustc_target_flags = --extern alloc \
+    --extern build_error --extern macros=$(objtree)/$(obj)/libmacros.so \
+    --extern uapi
+rustdoc-bindings: $(src)/bindings/lib.rs rustdoc-core rustdoc-macros \
+    rustdoc-compiler_builtins rustdoc-alloc $(obj)/libmacros.so \
+    $(obj)/bindings.o FORCE
+	$(call if_changed,rustdoc)
+
 rustdoc-kernel: private rustc_target_flags = --extern alloc \
     --extern build_error --extern macros=$(objtree)/$(obj)/libmacros.so \
     --extern bindings --extern uapi
 rustdoc-kernel: $(src)/kernel/lib.rs rustdoc-core rustdoc-macros \
-    rustdoc-compiler_builtins rustdoc-alloc $(obj)/libmacros.so \
-    $(obj)/bindings.o FORCE
+    rustdoc-compiler_builtins rustdoc-alloc rustdoc-bindings \
+    $(obj)/libmacros.so FORCE
 	$(call if_changed,rustdoc)
 
 quiet_cmd_rustc_test_library = RUSTC TL $<
-- 
2.34.1

[RFC PATCH 2/2] rust: bindings: add warning about configuration dependence

[Trevor Gross]

Add a note stating that binding documentation may differ from a user's
configuration, to make users aware that published documentation may not
always match a specific setup.

Signed-off-by: Trevor Gross <tmgross@umich.edu>
---
 rust/bindings/lib.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs
index 9bcbea04dac3..869632c2341c 100644
--- a/rust/bindings/lib.rs
+++ b/rust/bindings/lib.rs
@@ -7,6 +7,9 @@
 //! This crate may not be directly used. If you need a kernel C API that is
 //! not ported or wrapped in the `kernel` crate, then do so first instead of
 //! using this crate.
+//!
+//! Note that this module is heavily configuration-dependent, published versions
+//! of documentation may not accurately reflect local configuration.
 
 #![no_std]
 // See <https://github.com/rust-lang/rust-bindgen/issues/1651>.
-- 
2.34.1

Re: [RFC PATCH 0/2] Generate API documentation for 'bindings' crate

[Miguel Ojeda]

On Thu, Aug 3, 2023 at 11:36 AM Trevor Gross <tmgross@umich.edu> wrote:
>
> The 'bindings' crate currently doesn't have API documentation available.
> With this change, it will be generated as part of the output of 'make
> rustdoc' (similar to the 'kernel' crate's docs,
> https://rust-for-linux.github.io/docs/kernel/).
>
> This patch is a RFC because there are a few questions:

I think the first question to answer would be whether we want to
expose `bindings`, i.e. what are the advantages/disadvantages?

If `kernel` were a "normal library", then I would say we shouldn't,
because users should not need to care; and, in addition, the goal is
that leaf modules do not need to access them directly.

But, as sometimes happen, it may still be quite useful for some
developers nevertheless (the same way documenting the internal/private
details could be).

So, it would be nice to have an overview from your point of view on
why it should be done (or not).

> 1. Do we want to make this the default, or a separate target/
>    configuration? I don't think there is much downside to always
>    generating.

One downside of doing it by default would be going against the "avoid
`bindings`" guideline (ideally rule).

Another one is render time (the C side is trying to reduce it), I
guess, especially if we keep adding headers over time.

> 2. The entire '.config' file could be included in the doc output, to
>    make it easy to tell what settings the documentation was generated
>    with. Would this be desired? Maybe with a '--cfg
>    include-dotcfg=".config"' flag so published docs would have the
>    option (unsure whether it may ever have sensitive information).

This may be useful orthogonally to rendering `bindings` or not.

> Bindgen is currently invoked with '--no-doc-comments', I think this may
> be because some blocks were mistakenly interpreted as doctests. Once we
> upgrade our bindgen version we might be able to remove this.

Yes, that is https://github.com/Rust-for-Linux/linux/issues/323 and
https://github.com/rust-lang/rust-bindgen/issues/2057, which led to
the addition of `process_comments` to `bindgen` in v0.63.0.

> Side note, 'rust/Makefile' seems to have a mix of tabs and spaces - is
> this intentional?

Yes, it is intentional. For instance, the command definitions use
spaces like the vast majority of the kernel `Makefile`s.

Cheers,
Miguel

Re: [RFC PATCH 0/2] Generate API documentation for 'bindings' crate

[Trevor Gross]

On Thu, Aug 3, 2023 at 10:08 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> I think the first question to answer would be whether we want to
> expose `bindings`, i.e. what are the advantages/disadvantages?
>
> If `kernel` were a "normal library", then I would say we shouldn't,
> because users should not need to care; and, in addition, the goal is
> that leaf modules do not need to access them directly.
>
> But, as sometimes happen, it may still be quite useful for some
> developers nevertheless (the same way documenting the internal/private
> details could be).
>
> So, it would be nice to have an overview from your point of view on
> why it should be done (or not).

I do understand that dilemma, but am not sure what the happy medium is
between rendering them and hiding them. Where I see value is:

1. For anyone reading/writing abstractions, it's useful to quickly see
how exactly bindgen did the C -> Rust mapping
2. Abstractions may want to link to the C side somehow, linking the
bindings is an easier first step than linking to sphinx (in the future
we may be able to do a bindings -> sphinx link)

Maybe a stronger "prefer abstractions over bindings" message would
suffice to discourage use outside of reference?

In any case, I will put this behind a flag so it is not enabled by
default. While I'm at it - is there value in adding a config option to
pass `--document-private-items` to the kernel crate, or supporting
`RUSTDOCFLAGS` like Cargo does?

> > 1. Do we want to make this the default, or a separate target/
> >    configuration? I don't think there is much downside to always
> >    generating.
>
> One downside of doing it by default would be going against the "avoid
> `bindings`" guideline (ideally rule).
>
> Another one is render time (the C side is trying to reduce it), I
> guess, especially if we keep adding headers over time.

That makes sense, I will add the flag option.

> > 2. The entire '.config' file could be included in the doc output, to
> >    make it easy to tell what settings the documentation was generated
> >    with. Would this be desired? Maybe with a '--cfg
> >    include-dotcfg=".config"' flag so published docs would have the
> >    option (unsure whether it may ever have sensitive information).
>
> This may be useful orthogonally to rendering `bindings` or not.
>

I will add this in a separate patch.

> > Bindgen is currently invoked with '--no-doc-comments', I think this may
> > be because some blocks were mistakenly interpreted as doctests. Once we
> > upgrade our bindgen version we might be able to remove this.
>
> Yes, that is https://github.com/Rust-for-Linux/linux/issues/323 and
> https://github.com/rust-lang/rust-bindgen/issues/2057, which led to
> the addition of `process_comments` to `bindgen` in v0.63.0.

How would switching to the library work? Since that seems like a more
involved discussion, would postprocesing `generated_bindings.rs` be
acceptable instead? I have been playing around with a python script
that extracts the `#[doc ...]` blocks and (1) fixes the escaping and
(2) formats parameters and fixes their spacing, I could extract this
to a separate patch if it may be a while before we can use the
library.

> > Side note, 'rust/Makefile' seems to have a mix of tabs and spaces - is
> > this intentional?
>
> Yes, it is intentional. For instance, the command definitions use
> spaces like the vast majority of the kernel `Makefile`s.

Ah thanks, it just looks a bit weird in the diff.

> Cheers,
> Miguel

Thanks!
Trevor

Re: [RFC PATCH 0/2] Generate API documentation for 'bindings' crate

[Miguel Ojeda]

On Thu, Aug 3, 2023 at 5:35 PM Trevor Gross <tmgross@umich.edu> wrote:
>
> 1. For anyone reading/writing abstractions, it's useful to quickly see
> how exactly bindgen did the C -> Rust mapping

Do you mean using the integrated search in the generated docs and that
that is faster than e.g. grepping the generated file?

> 2. Abstractions may want to link to the C side somehow, linking the
> bindings is an easier first step than linking to sphinx (in the future
> we may be able to do a bindings -> sphinx link)

We definitely/already want to link to the C side in some places (e.g.
all header links, and some docs that refer to the C side, etc.), so
this is definitely valuable. But the ideal solution would be linking
to the actual C docs, indeed. I think we should avoid duplicating the
C docs infrastructure, if that is the use case here.

For that, I proposed to the `rustdoc` maintainers taking a map of
references (strings) to external resources (URLs), e.g. via a JSON
file or similar. This would be useful for a bunch of projects / use
cases, and I think the maintainers saw value in the feature. In the
kernel, for instance, we would generate a list of links in the C side
(e.g. mapping the `foo()` string to a URL, so that then a reference
like [`foo()`] in the Rust docs would be mapped to that URL).

Writing the `rustdoc` RFC has been in my backlog for a long time, but
if you would like to get involved, please let me know. It is a nice
time to come back to that, because the Rust docs are going to be in
kernel.org soon.

> Maybe a stronger "prefer abstractions over bindings" message would
> suffice to discourage use outside of reference?

Not sure if you mean in the module documentation or elsewhere (if the
latter, we have that in the kernel docs already as a "should").

> In any case, I will put this behind a flag so it is not enabled by
> default. While I'm at it - is there value in adding a config option to
> pass `--document-private-items` to the kernel crate, or supporting
> `RUSTDOCFLAGS` like Cargo does?

Personally, what I would love to see is that the documentation allows
you to see the private items but keeps them hidden by default (and
this could be applied to the `bindings` discussion too).

That way, users of subsystems do not need to see docs for private
items, or implementation details, or `bindings`. But, when needed, one
can simply flip a switch and see those immediately.

This would provide most of the benefits, while keeping the amount of
knobs/variables to maintain (and to learn, for users that want those
docs) as low as possible. A potential downside is, of course, build
time.

However, as far as I understand, that is not possible right now with
`rustdoc`, but would be ideal for us (and I imagine other projects).

An alternative that does not require `rustdoc` changes is providing a
second render of the docs in a subpage, e.g. `private` or
`implementation` or similar. If we go this way, a "top bar" (similar
to the one in docs.rs or Elixir) in order to select the kernel version
could be interesting, and could also be useful for other things such
as the "private" switch or arch/config selection if we end up with
that.

> How would switching to the library work? Since that seems like a more
> involved discussion, would postprocesing `generated_bindings.rs` be
> acceptable instead? I have been playing around with a python script
> that extracts the `#[doc ...]` blocks and (1) fixes the escaping and
> (2) formats parameters and fixes their spacing, I could extract this
> to a separate patch if it may be a while before we can use the
> library.

I think it would be best to go for the switch directly, i.e. to try to
use the "official" feature first (it was added on our request... :)

> Ah thanks, it just looks a bit weird in the diff.

My pleasure!

Cheers,
Miguel