Add `rust-analyzer-proc-macro-srv` binary, use it if found in sysroot #12858

fasterthanlime · 2022-07-23T20:54:30Z

This adds a bin crate which simply runs proc_macro_srv::cli::run() (it does no CLI argument parsing, nothing).

The intent is to build that crate in Rust CI as part of the dist::Rustc component, then ship it in the sysroot: it would probably land in something like ~/.rustup/toolchains/nightly-2022-07-23-x86_64-unknown-linux-gnu/libexec/proc-macro-srv-cli.

This makes rust-lang/rustup#3022 less pressing. (Instead of teaching RA about rustup components, we simply teach it to look in the sysroot via rustc --print sysroot. If it can't find proc-macro-srv-cli, it falls back to its own proc-macro subcommand).

This is closely related to #12803 (but doesn't close it yet).

Things to address now:

What should the binary be named? What should the crate be named? We can pick different names with [bin] in the Cargo.toml

Things to address later:

Disable the "multi ABI compatibility scheme" when building that binary in Rust CI (that'll probably happen in rust-lang/rust)
Teaching RA to look in the sysroot

Things to address much, much later:

Is JSON a good fit here
Do we want to add versioning to future-proof it?
Other bikesheds

When built with --features sysroot on nightly-2022-07-23-x86_64-unknown-linux-gnu, the binary is 7.4MB. After stripping debuginfo, it's 2.6MB. When compressed to .tar.xz, it's 619KB.

In a Zulip discussion, @jyn514 and @Mark-Simulacrum seemed to think that those sizes weren't a stopper for including the binary in the rustc component, even before we shrink it down further.

lnicola · 2022-07-23T20:56:37Z

This closes rust-lang/rustup#3022

Wouldn't a server binary accessible from PATH be useful for other editors too? Most of them simply run rust-analyzer IIUC.

fasterthanlime · 2022-07-23T20:58:09Z

Wouldn't a server binary accessible from PATH be useful for other editors too? Most of them simply run rust-analyzer IIUC.

Right! I edited the original message to read "makes this issue less pressing" instead of "closes this issue".

Veykril

proc-macro-srv-cli seems fine to me, though I don't really have any hard feelings on the matter.

crates/proc-macro-srv-cli/src/main.rs

fasterthanlime · 2022-07-24T12:53:08Z

One hesitation I have is.. should we bring an args parsing library in there? I'm thinking of future-proofing it: if we add a different format, maybe we'll want it to support --format-version 1 / --format-version 2 etc., it might be useful to have older proc-macro-srv binaries say what version they support at most, etc.

Veykril · 2022-07-24T13:19:03Z

Hmm, I don't think there is a need for this now until we think a bit about versioning in general? Not sure tbh

fasterthanlime · 2022-07-24T13:27:53Z

Hmm, I don't think there is a need for this now until we think a bit about versioning in general? Not sure tbh

Yeah I suppose the versioning can happen in the protocol itself. I'm just slightly uncomfortable that the default behavior of the CLI is to sit there waiting for stdin, with zero output whatsoever and no argument handling at all.

But I suppose we can get started without that.

pietroalbini · 2022-07-24T21:47:49Z

Not a rust-analyzer maintainers, but I have some ideas on this PR (feel free to ignore them):

We should have rust-analyzer somewhere in the binary name.
I'd add a RUSTC_BOOTSTRAP=1-esque gate to run the binary, to avoid people accidentally relying on it without knowing it's an unstable implementation detail. Maybe something like this?
```
RUST_ANALYZER_INTERNALS_DO_NOT_USE=this is unstable
```

fasterthanlime · 2022-07-24T22:47:45Z

@pietroalbini I agree on all these.

I turned the PR back to draft because I want to address these + start implementing "look into sysroot" functionality — it probably doesn't need to be spread over multiple PRs / syncs

jyn514 · 2022-07-24T22:58:34Z

@fasterthanlime it will be hard to test before we've landed the binary because it won't be in the sysroot yet. You can test everything else by adding some way to override the path of the macro client, though.

fasterthanlime · 2022-07-24T23:14:50Z

it will be hard to test before we've landed the binary because it won't be in the sysroot yet.

@jyn514 I was thinking of using a specialized tool to test that locally (/bin/cp)

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

…, start looking in there for a rust-analyzer-proc-macro-srv binary

fasterthanlime · 2022-07-25T14:23:48Z

This works for me locally:

[INFO rust_analyzer::reload] Found a cargo workspace...
[INFO rust_analyzer::reload] Found a cargo workspace with a sysroot...
[INFO rust_analyzer::reload] And the server exists at /home/amos/.rustup/toolchains/nightly-2022-07-23-x86_64-unknown-linux-gnu/libexec/rust-analyzer-proc-macro-srv
[INFO rust_analyzer::reload] Using proc-macro server at /home/amos/.rustup/toolchains/nightly-2022-07-23-x86_64-unknown-linux-gnu/libexec/rust-analyzer-proc-macro-srv with args []

After doing:

cargo +nightly-2022-07-23 build --release -p proc-macro-srv-cli --features proc-macro-srv/sysroot-abi
cp ./target/release/rust-analyzer-proc-macro-srv ~/.rustup/toolchains/nightly-2022-07-23-x86_64-unknown-linux-gnu/libexec/

This is good for review!

bjorn3 · 2022-07-25T14:43:49Z

crates/project-model/src/sysroot.rs

+    /// Returns sysroot directory, where `bin/`, `etc/`, `lib/`, `libexec/`
+    /// subfolder live, like:
+    /// `$HOME/.rustup/toolchains/nightly-2022-07-23-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library`
+    pub fn src_root(&self) -> &AbsPath {


I think the names of this and root should be swapped. Also it did be nice if path/to/sysroot/lib/rustlib/src/rust could be derived from path/to/sysroot if the former is not given.

oh I got my comments mixed up, hang on

(I already did the name swap - thankfully ProjectJson already had the proper naming)

Okay, comments fixed in 6967751

Also it did be nice if path/to/sysroot/lib/rustlib/src/rust could be derived from path/to/sysroot if the former is not given.

We never have a case where root is given but src_root is missing: previously, Sysroot could only be built when passing src_root. And simply joining lib/rustlib/src/rust is not a great idea. We already have a method for that (discover_sysroot_src_dir) and it respects RUST_SRC_PATH.

I considered making it so that building a Sysroot always takes root and an optional src_root (and if src_root is missing, we use the "discover" method), but decided against it, since:

There's a test case with a fake sysroot that only specifies src_root (it doesn't reproduce the directory structure of a real sysroot). We could fix that test case, no big deal, but...

We support rust-project.json, which up until now only allowed a sysroot_src field, not a sysroot field.

And going from sysroot_src to sysroot seems like a bad idea - it would involve checking that the last few path components are ["lib", "rustlib", "src", "rust], and removing them. I don't feel great about that.

I think we should allow only giving the sysroot field and skipping sysroot_src.

And simply joining lib/rustlib/src/rust is not a great idea.

Why not?

Why not?

Because non-cargo projects (ProjectWorkspace::Json) might have "libstd sources" somewhere but not a "sysroot" like we expect for cargo projects, maybe? I'm not sure.

I think we should allow only giving the sysroot field and skipping sysroot_src.

The question is where?

For cargo projects, we already call discover_* functions for both root and src_root. Those respect RUST_SRC_PATH for example.

For ProjectWorkspace::Json projects, I'm not sure who sets the standard (maybe RA does?), but the rust-project.json format currently only has a sysroot_src field. Is that the case where you want to allow passing sysroot and "guess" sysroot_src ?

Because non-cargo projects (ProjectWorkspace::Json) might have "libstd sources" somewhere but not a "sysroot" like we expect for cargo projects, maybe? I'm not sure.

Using sysroot + lib/rustlib/src/rust as fallback for sysroot_src would work fine in that case, right?

Because non-cargo projects (ProjectWorkspace::Json) might have "libstd sources" somewhere but not a "sysroot" like we expect for cargo projects, maybe? I'm not sure.

Using sysroot + lib/rustlib/src/rust as fallback for sysroot_src would work fine in that case, right?

No, they specify the lib/rustlib/src/rust - we can't get what we need by joining additional path segments, we need to strip some, and that feels really iffy (but we can do it if we want)

There's zero codepaths where we have sysroot but not sysroot_src.

sysroot is a directory with src/, lib/, libexec/, etc/, etc. sysroot_src is a deeper path with libstd sources (sorry about the initial confusion, my comments were swapped).

You don't have sysroot_src if the user didn't specify one. I think the behavior should be:

if user specifies sysroot_src => use it, else use sysroot + lib/rustlib/src/rust

Or to put it another way:

sysroot_src is not specified by the user, set it to sysroot + lib/rustlib/src/rust

You don't have sysroot_src if the user didn't specify one.

Ah, right. But right now no one is specifying sysroot either. That would require bazel (or other build systems that generate rust-project.json files) to set that field.

We should really discuss the Cargo / Json cases separately 😂

I implemented what I think you and @jyn514 asked for in 2c2520f

fasterthanlime · 2022-07-25T18:02:42Z

@bjorn3 so, to summarize:

For cargo projects, Sysroot::root and Sysroot::src_root are auto-discovered. This should always work.

For rust-project.json projects, we now allow passing either:

sysroot_src as before (in which case we pop 5 path components to find sysroot and hope for the best)
sysroot, which is new (in which case we join lib/rustlib/src/rust/library and hope for the best)
both, in which case there's no finger-crossing involved
none, in which case the Sysroot isn't even built

As a reminder, we need:

sysroot_src / src_root to index the Rust standard library's code
sysroot / root to find the rust-analyzer-proc-macro-srv binary

Veykril · 2022-07-26T06:35:10Z

@bors r+

bors · 2022-07-26T06:35:12Z

📌 Commit 2c2520f has been approved by Veykril

It is now in the queue for this repository.

bors · 2022-07-26T06:35:18Z

⌛ Testing commit 2c2520f with merge 7ba94a8...

bors · 2022-07-26T06:47:02Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 7ba94a8 to master...

Find standalone proc-macro-srv on windows too I forgot that executables end with `.exe` on Windows in: * #12858

woody77 · 2022-08-10T21:17:04Z

for sysroot, we'd want the path to the host machine rust sysroot, correct?

historically, we've had the sysroot as the first crate (due to how we generate rust-project.json with GN:

"crates": [
    {
      "crate_id": 0,
      "root_module": "<homedir>/development/fuchsia/prebuilt/third_party/rust/linux-x64/lib/rustlib/src/rust/library/core/src/lib.rs",
      "label": "core",
      "source": {
          "include_dirs": [
               "/usr/local/google/home/aaronwood/development/fuchsia/prebuilt/third_party/rust/linux-x64/lib/rustlib/src/rust/library/core/src/"
          ],
      },
    },

and so we would just need to add:

{
  sysroot: "<homedir>/development/fuchsia/prebuilt/third_party/rust/linux-x64",
  crates: [ ....
}

and the appending of lib/rustlib/src/rust/library will pick it up correctly?

fasterthanlime · 2022-08-10T21:51:23Z

@woody77 that's an excellent point, I hadn't really thought about cross compilation when working on this.

For sure, proc macros must compile and run on the host platform. However for regular (non-build and non-proc-macro) dependencies, RA should probably be looking at the target platform's sysroot.

We probably need to introduce more fields to properly support cross compilation, unless I'm missing something.

jyn514 · 2022-08-10T22:07:39Z

@fasterthanlime I don't understand how cross-compilation is related. RA is only looking for the proc-macro-srv binary, right? So it only needs to look in the host sysroot, because that's the platform the server will be run on, regardless of the target platform.

fasterthanlime · 2022-08-10T22:11:59Z

@jyn514 that's the "sysroot use" I added, yes. But before that, RA was already detecting sysroot for cargo projects (and accepting a sysroot path for rust-project.json workspaces) so it could index libstd sources — those need to be for the target, not for the host (afaict).

woody77 · 2022-08-10T22:17:24Z

We have had to explicitly add the sysroot crates as crates in rust-project.json, which definitely finds the correct sysroot for us. We use the same sysroot for host/target, but with different compilation options. But that's just how we do things. I could envision things done differently elsewhere.

I'm testing a change with GN right now that adds the sysroot field if there is only one sysroot used in the whole rust-project.json file (just to simplify things in case someone does have more than one sysroot with their GN project).

bjorn3 · 2022-08-10T22:17:45Z

The only time the host and target sysroot can be different is when you build a sysroot for the target yourself. Regular --target uses the same sysroot.

Edit: that was in reply to @fasterthanlime

bjorn3 · 2022-08-10T22:19:44Z

@woody77 I'm curious. What is GN short for?

woody77 · 2022-08-10T22:21:05Z

Generate Ninja - https://gn.googlesource.com/gn/

woody77 · 2022-08-10T22:47:12Z

crates/rust-analyzer/src/reload.rs

+                        let mut args = args.clone();
+                        let mut path = path.clone();
+
+                        if let ProjectWorkspace::Cargo { sysroot, .. } = ws {


So at this point in time, we don't have any support for ProjectWorkspace::Json, even if sysroot is set?

Did proc-macros ever work for project.json workspaces actually? From the looks of it we never even build build scripts for them since we can't really do that

Yes, proc macros should work with project.json workspaces. We have a field that allows specifying the path of the compiled proc macro.

Can you point me to where the proc-macro-srv is spawned for ProjectWorkspace::Json workspaces?

rust-analyzer/crates/project-model/src/workspace.rs

Line 484 in b1e9bcd

Some(it) => load_proc_macro(

I think.

@woody77 that match is definitely missing an arm I forgot to add when I added the additional field to rust-project.json's schema. I also didn't have any projects to test it with. I encourage you to open the small PR to fix it!

fasterthanlime mentioned this pull request Jul 23, 2022

Option to use the rustup-provided proc-macro-srv #12855

Closed

fasterthanlime marked this pull request as ready for review July 23, 2022 21:00

This was referenced Jul 23, 2022

Add rust-analyzer proxy. rust-lang/rustup#3022

Merged

Convert rust-analyzer to an in-tree tool rust-lang/rust#99603

Merged

Veykril reviewed Jul 24, 2022

View reviewed changes

crates/proc-macro-srv-cli/src/main.rs Outdated Show resolved Hide resolved

fasterthanlime marked this pull request as draft July 24, 2022 22:46

fasterthanlime and others added 4 commits July 25, 2022 15:19

Add a proc-macro-srv-cli crate

5f54ec0

Add doc comment to pass tidy check

fba6adf

Return result directly

fd1b64e

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

Name the binary 'proc-macro-srv'

4364531

fasterthanlime force-pushed the proc-macro-srv-bin branch from eacccf7 to 4364531 Compare July 25, 2022 13:20

fasterthanlime added 2 commits July 25, 2022 16:07

Remember the difference between 'sysroot root' and 'sysroot src root'…

dadb832

…, start looking in there for a rust-analyzer-proc-macro-srv binary

Gate and rename binary, use it if it's in the sysroot

74a2fad

fasterthanlime marked this pull request as ready for review July 25, 2022 14:22

fasterthanlime changed the title ~~Add a proc-macro-srv-cli crate~~ Add rust-analyzer-proc-macro-srv binary, use it if found in sysroot Jul 25, 2022

bjorn3 reviewed Jul 25, 2022

View reviewed changes

fasterthanlime added 2 commits July 25, 2022 16:59

Fix mix up in comments

6967751

Allow specifying sysroot OR sysroot_src

2c2520f

Veykril approved these changes Jul 26, 2022

View reviewed changes

bors merged commit 7ba94a8 into rust-lang:master Jul 26, 2022

fasterthanlime mentioned this pull request Jul 26, 2022

Find standalone proc-macro-srv on windows too #12878

Merged

bors added a commit that referenced this pull request Jul 26, 2022

Auto merge of #12878 - fasterthanlime:standalone-pms-exe, r=lnicola

1c75284

Find standalone proc-macro-srv on windows too I forgot that executables end with `.exe` on Windows in: * #12858

fasterthanlime mentioned this pull request Jul 28, 2022

Resolving proc-macro ABI breakage for rustc nightly users: one way forward #12803

Closed

woody77 reviewed Aug 10, 2022

View reviewed changes

woody77 mentioned this pull request Sep 23, 2022

Methods provided via Deref<Target> are not being found #13238

Closed

Add rust-analyzer-proc-macro-srv binary, use it if found in sysroot #12858

Add rust-analyzer-proc-macro-srv binary, use it if found in sysroot #12858

Conversation

fasterthanlime commented Jul 23, 2022 • edited Loading

lnicola commented Jul 23, 2022

fasterthanlime commented Jul 23, 2022

Veykril left a comment • edited Loading

Choose a reason for hiding this comment

fasterthanlime commented Jul 24, 2022

Veykril commented Jul 24, 2022

fasterthanlime commented Jul 24, 2022

pietroalbini commented Jul 24, 2022

fasterthanlime commented Jul 24, 2022

jyn514 commented Jul 24, 2022

fasterthanlime commented Jul 24, 2022

fasterthanlime commented Jul 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasterthanlime commented Jul 25, 2022 • edited Loading

Veykril commented Jul 26, 2022

bors commented Jul 26, 2022

bors commented Jul 26, 2022

bors commented Jul 26, 2022

woody77 commented Aug 10, 2022

fasterthanlime commented Aug 10, 2022

jyn514 commented Aug 10, 2022

fasterthanlime commented Aug 10, 2022

woody77 commented Aug 10, 2022

bjorn3 commented Aug 10, 2022 • edited Loading

bjorn3 commented Aug 10, 2022

woody77 commented Aug 10, 2022

Choose a reason for hiding this comment

Veykril Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `rust-analyzer-proc-macro-srv` binary, use it if found in sysroot #12858

Add `rust-analyzer-proc-macro-srv` binary, use it if found in sysroot #12858

fasterthanlime commented Jul 23, 2022 •

edited

Loading

Veykril left a comment •

edited

Loading

fasterthanlime commented Jul 25, 2022 •

edited

Loading

bjorn3 commented Aug 10, 2022 •

edited

Loading

Veykril Aug 11, 2022 •

edited

Loading