Message-ID: <CABVgOS=UN2AEnkTTdbMKmsROk19-r2SkokxcC2ps7i1t8ocXQA@mail.gmail.com>
Date: Sat, 18 Oct 2025 15:18:10 +0800
From: David Gow <davidgow@...gle.com>
To: Miguel Ojeda <ojeda@...nel.org>
Cc: Guillaume Gomez <guillaume1.gomez@...il.com>, Brendan Higgins <brendan.higgins@...ux.dev>, 
	Rae Moar <rmoar@...gle.com>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, kunit-dev@...glegroups.com, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 1/1] Use new `--output-format=doctest` rustdoc command
 line flag to improve doctest handling

Thanks Guillaume, Miguel.

This is looking good to me.

On Thu, 9 Oct 2025 at 17:14, Miguel Ojeda <ojeda@...nel.org> wrote:
>
> On Tue, 17 Jun 2025 16:00:33 +0200 Guillaume Gomez <guillaume1.gomez@...il.com> wrote:
> >
> > The goal of this patch is to remove the use of 2 unstable
> > rustdoc features (`--no-run` and `--test-builder`) and replace it with a
> > stable feature: `--output-format=doctest`, which was added in
> > https://github.com/rust-lang/rust/pull/134531.
> >
> > Before this patch, the code was using very hacky methods in order to retrieve
> > doctests, modify them as needed and then concatenate all of them in one file.
> >
> > Now, with this new flag, it instead asks rustdoc to provide the doctests
> > code with their associated information such as file path and line number.
> >
> > Signed-off-by: Guillaume Gomez <guillaume1.gomez@...il.com>
> > ---
>
> (Procedural bit: normally we provide a changelog between versions after
> this `---` line so that reviewers now what changed so far.)
>
> I finally took a look at this again, so I rebased it and got:
>
>     thread 'main' panicked at scripts/rustdoc_test_gen.rs:92:15:
>     No path candidates found for `rust_kernel_alloc_allocator.rs`.This is likely a bug in the build system, or some files went away while compiling.
>
> which brings me to the bigger point: the main reason to have the new
> output format is to avoid all these hacks, including the "find the real
> path back to the original file" hack here. More generally, to avoid the
> 2 scripts approach.
>
> So now we can finally get rid of all that and simplify. That is, we can
> just merge it all in a single script that reads the JSON and builds the
> result directly, since now we have everything we need (originally I
> needed the 2 scripts approach since `rustdoc` executed the test builder
> once per test so I had to somehow collect the results).
>
> i.e. no more hundreds of generated files/processes, just a simple pipe.

Yeah, this definitely seems the right way -- even though outputting
the generated rustdoc.json to a file is sometimes useful for debugging
it. But unless this actually breaks often (which hopefully it will
less frequently with this setup), just passing things around directly
in a pipe is neater.

>
> Anyway, just to check we had everything we needed, I did a quick try --
> please see the draft patch below.
>
> I gave it a go -- please see the draft patch below. The diff w.r.t. your
> patch would be something like +217 -341, i.e. we get rid of quite a lot
> of lines. I added as well some more context in the commit message, and
> put the right docs in the unified script. This also improves the sorting
> of the tests (it now follows the line number better).
>
> We still have to preserve the support for the old compilers, so what I
> think I will do is just have the new script separately, keeping the old
> ones as-is until we can remove them when we upgrade the minimum for e.g.
> the next Debian Stable.

Agreed: we definitely need to keep the old way running for a little
bit longer. Whether or not it makes sense to have both versions
running in parallel, or just delay this until we no-longer need the
old implementation is a matter of taste, I suspect. Having just one
"active" implementation at a time has a bunch of advantages
(particularly in the case there's some inconsistency in, e.g., test
names between compiler versions) -- it's probably what I'd go for
(mostly because it's easier), but I've no objection to having both
running in parallel, assuming it doesn't cause any more issues than
the code getting more complicated. And I personally don't feel that
having both is likely to be a big enough burden to be worth bumping
the compiler version alone.

>
> Cc'ing David and KUnit, since this is closer to getting ready -- please
> let me know if this raises alarms for anyone.
>

Apart from the fact that it breaks the older compilers, this looks
like a definite improvement. (I was a little worried about adding a
whole json parser in, but it's nicely small and self-contained, so I
actually quite like it.)

So I'm happy with this from the KUnit side, so long as we either don't
remove support for the old system/old compilers, or delay merging this
until we can drop support for those altogether.

Cheers,
-- David

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5281 bytes)

Powered by blists - more mailing lists