linux-kernel - Re: [PATCH 1/1] Use new `--output-format=doctest` rustdoc command line flag to improve doctest handling.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72mENfqG6iNjgXpq4LVEceZ4174yGhg-RB0MsMxLVed-1A@mail.gmail.com>
Date: Fri, 28 Feb 2025 16:43:29 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Guillaume Gomez <guillaume1.gomez@...il.com>
Cc: ojeda@...nel.org, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] Use new `--output-format=doctest` rustdoc command
 line flag to improve doctest handling.

On Fri, Feb 28, 2025 at 2:29 PM Guillaume Gomez
<guillaume1.gomez@...il.com> wrote:
>
> 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.

Thanks for doing this (and the `rustdoc` side of it!), and welcome!

Normally in commit messages we need to give more details:

  - A Signed-off-by is required (please see for what that implies).

    https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

  - The commit message should explain the motivation, i.e. removing
the 2 unstable `rustdoc` features, and moving to the new,
intended-to-be-stable one (which one? etc.), and so on.

  - Also, a bit of context (or at least linking to it) would be good,
e.g. explaining that we were working on removing unstable features,
and that `rustdoc` provided a new flag for the kernel, ideally with
links to the relevant Rust tracking issues/PRs (typically with a
"Link: " tag), plus which version the unstable feature is available,
etc. (well, when we know for sure, i.e. I understand that we may need
to tweak it still).

Anyway, those are procedural details we can sort out later :) What is
most important I think are the following two notes, please see below.

> -       $(RUSTDOC) --test $(rust_flags) \
> +       $(RUSTDOC) --output-format=doctest $(rust_flags) \
>                 -L$(objtree)/$(obj) --extern ffi --extern kernel \
>                 --extern build_error --extern macros \
>                 --extern bindings --extern uapi \
> -               --no-run --crate-name kernel -Zunstable-options \
> +               --crate-name kernel -Zunstable-options \
>                 --sysroot=/dev/null \
> -               --test-builder $(objtree)/scripts/rustdoc_test_builder \
> -               $< $(rustdoc_test_kernel_quiet); \
> +               $< $(rustdoc_test_kernel_quiet) > rustdoc.json; \
> +       cat rustdoc.json | $(objtree)/scripts/rustdoc_test_builder; \
>         $(objtree)/scripts/rustdoc_test_gen

We currently support versions 1.78+, so we will need to do these
things conditionally. I can help with that, so don't worry too much
about it for the moment (e.g. we have `rustc-option` and other helpers
to pick the right flags given a version etc.).

Similarly, we will also need to tell the script whether to use the old
or the new way, too.

> +    // We replace the `Result` if needed.
> +    let doctest_code = doctest_code.replace(
> +        "fn main() { fn _inner() -> Result<",
> +        "fn main() { fn _inner() -> core::result::Result<",
> +    );

Hmm... I was hoping we could do something on the `rustdoc` side to
remove these hacks altogether since we are going to have a "proper
flag" for it, i.e. to avoid relying on the particular "format" of the
tests somehow.

I wrote about a bit of that here:

    https://github.com/rust-lang/rust/pull/134531#discussion_r1894610592

> +    let doctest_code = doctest_code.replace(
> +        "} _inner().unwrap() }",
> +        "} let test_return_value = _inner(); assert!(test_return_value.is_ok()); }",
>      );
> +    std::fs::write(path, doctest_code.as_bytes()).unwrap();
> +}

Same for this.

> +    } else {
> +        panic!("missing `format_version` field");
> +    }

`expect` maybe?

> @@ -87,8 +87,8 @@ fn find_candidates(
>
>      assert!(
>          valid_paths.len() > 0,
> -        "No path candidates found. This is likely a bug in the build system, or some files went \
> -        away while compiling."
> +        "No path candidates found for `{file}`. This is likely a bug in the build system, or some \
> +        files went away while compiling.",
>      );
>
>      if valid_paths.len() > 1 {
> @@ -97,8 +97,8 @@ fn find_candidates(
>              eprintln!("    {path:?}");
>          }
>          panic!(
> -            "Several path candidates found, please resolve the ambiguity by renaming a file or \
> -            folder."
> +            "Several path candidates found for `{file}`, please resolve the ambiguity by renaming \
> +            a file or folder."
>          );
>      }

These two bits could go in a first patch, I think, though it isn't a big deal.

Thanks again for getting us out of unstable in `rustdoc`!

Cheers,
Miguel