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@xxxxxxxxx> 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