[PATCH v2] rust: Support latest version of `rust-analyzer`

[PATCH v2] rust: Support latest version of `rust-analyzer`

[Sarthak Singh]

From: Sarthak Singh <ss269@uw.edu>

Sets the `sysroot` field in rust-project.json which is now needed in
newer versions of rust-analyzer instead of the `sysroot_src` field.

`sysroot` over `sysroot_src` has been working since rust 1.74.0 (which
was released on Nov 16, 2023). Since we are going to target rust 1.78.0
going forward we should set `sysroot` to support all currently used and
future versions of rust-analyzer.

Code editors like VS Code try to use the latest version of rust-analyzer
(which is updated every week) instead of the version of rust-analyzer
that comes with the rustup toolchain (which is updated every six weeks
along with the rust version).

Without this change rust-analyzer is breaking for anyone using VS Code.

Signed-off-by: Sarthak Singh <ss269@uw.edu>
---
Hello,

I fixed some typos that I made in the v1 of the patch as pointed out by
Dirk. Let me know if anything else is wrong. This is my first time
contributing to linux.

 rust/Makefile                     | 2 +-
 scripts/generate_rust_analyzer.py | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/rust/Makefile b/rust/Makefile
index bf05e65365da..e3d0ba099b18 100644
--- a/rust/Makefile
+++ b/rust/Makefile
@@ -350,7 +350,7 @@ rust-analyzer:
 	$(Q)$(srctree)/scripts/generate_rust_analyzer.py \
 		--cfgs='core=$(core-cfgs)' --cfgs='alloc=$(alloc-cfgs)' \
 		$(realpath $(srctree)) $(realpath $(objtree)) \
-		$(RUST_LIB_SRC) $(KBUILD_EXTMOD) > \
+		$(rustc_sysroot) $(RUST_LIB_SRC) $(KBUILD_EXTMOD) > \
 		$(if $(KBUILD_EXTMOD),$(extmod_prefix),$(objtree))/rust-project.json
 
 redirect-intrinsics = \
diff --git a/scripts/generate_rust_analyzer.py b/scripts/generate_rust_analyzer.py
index f270c7b0cf34..8529c3940136 100755
--- a/scripts/generate_rust_analyzer.py
+++ b/scripts/generate_rust_analyzer.py
@@ -145,6 +145,7 @@ def main():
     parser.add_argument('--cfgs', action='append', default=[])
     parser.add_argument("srctree", type=pathlib.Path)
     parser.add_argument("objtree", type=pathlib.Path)
+    parser.add_argument("sysroot", type=pathlib.Path)
     parser.add_argument("sysroot_src", type=pathlib.Path)
     parser.add_argument("exttree", type=pathlib.Path, nargs="?")
     args = parser.parse_args()
@@ -154,9 +155,12 @@ def main():
         level=logging.INFO if args.verbose else logging.WARNING
     )
 
+    # Making sure that the sysroot and sysroot_src belong to the same toolchain
+    assert args.sysroot in args.sysroot_src.parents
+
     rust_project = {
         "crates": generate_crates(args.srctree, args.objtree, args.sysroot_src, args.exttree, args.cfgs),
-        "sysroot_src": str(args.sysroot_src),
+        "sysroot": str(args.sysroot),
     }
 
     json.dump(rust_project, sys.stdout, sort_keys=True, indent=4)
-- 
2.45.2

Re: [PATCH v2] rust: Support latest version of `rust-analyzer`

[Miguel Ojeda]

Hi Sarthak,

Thanks for the patch! A few notes below.

On Tue, Jul 23, 2024 at 12:41 PM Sarthak Singh
<sarthak.singh99@gmail.com> wrote:
>
> From: Sarthak Singh <ss269@uw.edu>

In case you are wondering, this line is here because you are
submitting the patch from a different email address than this one. It
would be ideal if you can submit from the address that you are
signing-off-by (if you cannot, please send me a dummy private message
from that address).

> Sets the `sysroot` field in rust-project.json which is now needed in
> newer versions of rust-analyzer instead of the `sysroot_src` field.
>
> `sysroot` over `sysroot_src` has been working since rust 1.74.0 (which
> was released on Nov 16, 2023). Since we are going to target rust 1.78.0
> going forward we should set `sysroot` to support all currently used and
> future versions of rust-analyzer.

Hmm... it would be nice to know the root cause, i.e. what changed in
rust-analyzer?

From a quick look, the `sysroot` key has existed in rust-analyzer
since 2022 (189521a4db6e), but undocumented. Then it got documented in
2023 (5da14237eb29). So I am not sure what "`sysroot` over
`sysroot_src`" means (and the current docs are not very clear on what
each does and their intended interactions, e.g. what happens if one
specifies `sysroot_src` but not `sysroot`, or the other way around?
Should `sysroot_src` be relative to `sysroot` like the default?)

Anyway, we are passing the paths for `core` etc. in `crates`, and we
don't have a custom `alloc` anymore, so I wonder if we should either
delete both keys (or however one disables auto-search) or delete the
crates in `crates` and let the sysroot logic find them.

(Cc'ing Lukas since he may able to tell us about the background.)

> Code editors like VS Code try to use the latest version of rust-analyzer
> (which is updated every week) instead of the version of rust-analyzer
> that comes with the rustup toolchain (which is updated every six weeks
> along with the rust version).
>
> Without this change rust-analyzer is breaking for anyone using VS Code.

Just to confirm, does this make it work for distribution and
`rustup`-using installs?

> Signed-off-by: Sarthak Singh <ss269@uw.edu>

Dirk gave a Tested-by tag to v1 -- normally you would pick up those
tags if you submit a new version and you consider them still valid
(before the Signed-off-by). I think you didn't change the contents of
the patch, right?

Cheers,
Miguel

Re: [PATCH v2] rust: Support latest version of `rust-analyzer`

[Sarthak Singh]

Hello Miguel,

Thank you for reviewing. How do you send replies? To send this one I
am writing a text file manually to look like the generated patch
file that I emailed earlier. Is that the best way?

I don't have access to the ss269@uw.edu email anymore.

Should I resend the patch with the fixed email address? Afaik there is
no way to update a patch, let me know if that is possible. I can also
add the missing Tested-by tag in the new version of the patch.

In the PR [1] the ability to guess the sysroot based on the sysroot_src
was removed. That is why every version of rust-analyzer after this PR
fails with the current config generation system. rust-analyzer needs to
know the sysroot so that it can find its proc-macro server.

> From a quick look, the `sysroot` key has existed in rust-analyzer
> since 2022 (189521a4db6e), but undocumented. Then it got documented in
> 2023 (5da14237eb29). So I am not sure what "`sysroot` over
> `sysroot_src`" means (and the current docs are not very clear on what
> each does and their intended interactions, e.g. what happens if one
> specifies `sysroot_src` but not `sysroot`, or the other way around?
> Should `sysroot_src` be relative to `sysroot` like the default?)

sysroot need to point to the root location of the toolchain. This
contains the rust core and std library the compiler will link against.
It also contains the rustc and cargo for that toolchain.

sysroot_src need to point to the root location of the rust library. It
contains the source code of the core / std / alloc crates. I think for
our purposes it need to be a subset of the sysroot since we install rust
using rustup. If someone installs it in a manner where the source code
does not live in a sub directory of the sysroot path then they will have
to set it to that.
I think the main invariant that we need to uphold between them is that
the built libraries in sysroot and the source code in sysroot_src belong
to the same built of rust.

> Anyway, we are passing the paths for `core` etc. in `crates`, and we
> don't have a custom `alloc` anymore, so I wonder if we should either
> delete both keys (or however one disables auto-search) or delete the
> crates in `crates` and let the sysroot logic find them.

I think it will always need a `sysroot` key as it needs to know that
to find the proc-macro server. I don't think there is a way to specify
the version of rust in a `rust-project.json` other than specifying the
`sysroot` [2].

> Just to confirm, does this make it work for distribution and
> `rustup`-using installs?

Yes this should work with every setup. The `sysroot_src` that
rust-analyzer is internally generating is the same. The `sysroot`
that rust-analyzer was generating earlier is also defined to be
the same in this version.

Thank you,
Sarthak

[1] https://github.com/rust-lang/rust-analyzer/pull/17287/files#diff-53e4b6b2c1ff2465d8abd50db160753199426fd788ddcad925752e0838e28de2L367-L374
[2] https://rust-analyzer.github.io/manual.html#non-cargo-based-projects

Re: [PATCH v2] rust: Support latest version of `rust-analyzer`

[Miguel Ojeda]

On Tue, Jul 23, 2024 at 6:01 PM Sarthak Singh <sarthak.singh99@gmail.com> wrote:
>
> Thank you for reviewing. How do you send replies? To send this one I
> am writing a text file manually to look like the generated patch
> file that I emailed earlier. Is that the best way?

Unless I am sending patches, I typically just reply using the normal
Gmail web UI. :)

> I don't have access to the ss269@uw.edu email anymore.
>
> Should I resend the patch with the fixed email address? Afaik there is
> no way to update a patch, let me know if that is possible. I can also
> add the missing Tested-by tag in the new version of the patch.

You can just send a new version, no worries!

> In the PR [1] the ability to guess the sysroot based on the sysroot_src
> was removed. That is why every version of rust-analyzer after this PR
> fails with the current config generation system. rust-analyzer needs to
> know the sysroot so that it can find its proc-macro server.

That makes sense -- thanks for taking a look. It is indeed a hack what
they had there :)

Please add that information to the commit message (and possibly
include your link in a "Link: https://...... [1]" tag).

It would be nice if the rust-analyzer docs said what is the
intention/expectation here, even if the current implementation does X
or Y.

> sysroot need to point to the root location of the toolchain. This
> contains the rust core and std library the compiler will link against.
> It also contains the rustc and cargo for that toolchain.

For instance, for this one, from rust-analyzer's docs alone, it sounds
like it is required, but it is optional, and it does not say what
happens in that case.

> sysroot_src need to point to the root location of the rust library. It

I am not sure -- does it? i.e. the docs seem to claim it is not always needed:

    /// By default, this is `lib/rustlib/src/rust/library`
    /// relative to the sysroot.

and:

    /// if you omit this path, you can specify sysroot
    /// dependencies yourself and, for example, have
    /// several different "sysroots" in one graph of
    /// crates.

So it sounds like one can just give the `sysroot`, like you do in your
patch. I guess you may have meant "if given, it needs to point to that
particular path"?

Anyway, in your patch, you are still passing both (which makes sense,
given `RUST_LIB_SRC`), but you are not setting `sysroot_src`.

So I assume things could break (or be confusing) if someone provides
(intentionally) a `RUST_LIB_SRC` that is different than the one in the
actual sysroot, no?

> our purposes it need to be a subset of the sysroot since we install rust
> using rustup. If someone installs it in a manner where the source code

Just to clarify: not everybody installs things via `rustup`, and we
also support Linux distributions' toolchains
(https://lore.kernel.org/rust-for-linux/20240709160615.998336-14-ojeda@kernel.org/),
kernel.org's LLVM+Rust combined toolchain tarballs
(https://mirrors.edge.kernel.org/pub/tools/llvm/rust/), as well as
Rust's standalone installers
(https://forge.rust-lang.org/infra/other-installation-methods.html#standalone-installers).

> Yes this should work with every setup. The `sysroot_src` that
> rust-analyzer is internally generating is the same. The `sysroot`
> that rust-analyzer was generating earlier is also defined to be
> the same in this version.

By internally generating, do you mean in e.g. the debug output of
rust-analyzer or similar?

We should double-check that it works for "common" setups.

Thanks for getting to the root of all this!

Cheers,
Miguel