Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221130220825.1545758-1-carlos.bilbao@amd.com>
In-Reply-To: <20221130220825.1545758-1-carlos.bilbao@amd.com>
From:   Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date:   Thu, 1 Dec 2022 13:42:08 +0100
Message-ID: <CANiq72nMY5f85tJJFg7AFsh4YRrKObhurhT8TVawYqoZU+J-Fg@mail.gmail.com>
Subject: Re: [PATCH] docs: Integrate rustdoc into Rust documentation
To:     Carlos Bilbao <carlos.bilbao@amd.com>
Cc:     corbet@lwn.net, ojeda@kernel.org, bilbao@vt.edu,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        konstantin@linuxfoundation.org, Akira Yokosawa <akiyks@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Nov 30, 2022 at 11:08 PM Carlos Bilbao <carlos.bilbao@amd.com> wrote:
>
> Include HTML output generated with rustdoc into the Linux kernel
> documentation on Rust. Change target `make htmldocs` to combine RST Sphinx
> and the generation of Rust documentation, when support is available.

Looks better, thanks for this v2! A few comments below...

> +ifdef CONFIG_RUST
> +       @make LLVM=1 rustdoc
> +endif

The Rust docs should probably be built with the build
system/config/... as given, whether it is GCC, LLVM, etc. This should
probably use `$(MAKE)` too; and if you intended to remove the command
line definitions, `MAKEOVERRIDES` too.

Ideally `htmldocs` would depend on `rustdoc`, though that would
require shuffling quite a few things since to build the Rust docs we
need a subset of the Rust dependencies built. Given we may be changing
things soon anyway on the Rust `Makefile`, we can leave that for the
future.

By the way, while checking this, I noticed we use some `CONFIG_`s in
this `Makefile`, but we do not perform a config sync for the `*docs`
targets, so one needs to do so manually, i.e. it can be a pitfall for
e.g. `CONFIG_WARN_MISSING_DOCUMENTS` and ` as well as a potential
`CONFIG_RUST`. Should this be fixed orthogonally, or is it intended?
(some targets do not need the sync, and the ones that need are
probably less used, so I guess that could be the reason?).

> +Rustdoc output
> +==============
> +
> +If this documentation includes rustdoc-generated HTML, the entry point
> +can be found `here. <rustdoc/kernel/index.html>`_

Perhaps this sentence could be moved to the top of the index file, so
that users do not need two clicks when visiting "Rust"? That way we
avoid one more file too.

> +RUSTDOC_OUTPUT=$(objtree)/Documentation/output/rust/rustdoc

Please add a space around the equal sign to be consistent with (most)
of the rest of the file.

Cheers,
Miguel