Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rust-analyzer proxy. #3022

Merged
merged 2 commits into from
Aug 27, 2022
Merged

Add rust-analyzer proxy. #3022

merged 2 commits into from
Aug 27, 2022

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jul 6, 2022

This adds rust-analyzer as a proxy.

rust-analyzer is replacing RLS (see https://blog.rust-lang.org/2022/07/01/RLS-deprecation.html). As part of that process, I'd like to make rust-analyzer easier to run for non-VSCode users.

The rust-analyzer component has left preview mode as of rust-lang/rust#98640, which should be part of the 1.64 stable release (September 22, 2022). I think it would be nice to have rustup support that around the same time.

The rust-analyzer proxy was previously added in #2408 and then backed out in #2560 due to being nightly-only.

There is some risk this could cause disruption if a user is relying on having rust-analyzer in their PATH and this gets in the way. I suspect (hope?) that isn't too common. We may want to consider holding off on releasing this until after the 1.64 release.

@kinnison
Copy link
Contributor

We have removed snap support so you will need to rebase and remove that bit of your change, also since this adds a proxy, we must be sure to put it front-and-centre on any release blog post. (the latter there is more for my reference when I prepare changelogs)

@ehuss
Copy link
Contributor Author

ehuss commented Jul 16, 2022

Thanks, I have resolved the conflict.

@fasterthanlime
Copy link

This change is very timely - it's one piece of the puzzle for:

There's discussion on Zulip about switching ra from a submodule to a subtree, which would make this even more useful.

The sooner this lands, the better (imo).

src/lib.rs Outdated
@@ -27,6 +27,7 @@ pub static TOOLS: &[&str] = &[
"rust-gdb",
"rust-gdbgui",
"rls",
"rust-analyzer",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be in DUP_TOOLS? That way it won't override a rust-analyzer someone directly installed in .cargo/bin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I went ahead and moved it. I did not realize that the rust-analyzer documentation recommended using cargo install.

Copy link
Member

@lnicola lnicola Jul 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not cargo install, but we have an x.py-like helper for source builds, cargo xtask install, which installs it under ~/.cargo/bin.

The rust-analyzer documentation includes the step `cargo install`
into `~/.cargo/bin` when building from source. Since this has a chance
of accidentally overriding the user's copy, move this to DUP_TOOLS.
@kinnison
Copy link
Contributor

I am loathe to merge this until/unless we are certain it won't confuse others. Is the right answer to get it merged/released just before the stable release which contains r-a ?

@fasterthanlime
Copy link

I am loathe to merge this until/unless we are certain it won't confuse others.

Most VSCode users won't be affected by this — the rust-analyzer VSCode extension will use its own bundled version of rust-analyzer, published on the marketplace within the extension itself.

VSCode users who have rust-analyzer.server.path set to rust-analyzer already have a rust-analyzer binary in $PATH and so they won't be affected either, thanks to the DUP_TOOLS thing.

For other editors, some can download the rust-analyzer binary themselves (e.g. coc-rust-analyzer for Vim/NeoVim), those shouldn't be affected. If their editor plugin relies on rust-analyzer being in $PATH, that means less set up for them if they don't already have it, and if they already have it, no change.

@fasterthanlime
Copy link

Is the right answer to get it merged/released just before the stable release which contains r-a ?

@kinnison There's incentive to get it merged before the next Rust stable: it's part of a plan to make proc macros never break again when using rust-analyzer with rust nightly, cf. this zulip message.

If this doesn't get merged, there's alternatives (teaching rust-analyzer to use rustup which / rustup component add and using the absolute path of the toolchain-specific rust-analyzer binary), but this would be a nice start.

@Veykril
Copy link
Member

Veykril commented Jul 23, 2022

I am loathe to merge this until/unless we are certain it won't confuse others.

As was outlined given DUP_TOOLS this should not have any effect on current users, even if they cargo installed the rust-analyzer binary. So I don't think there are any concrete problems that would arise with adding the proxy.

It would be preferable to get this merged as soon as possible as this is technically a blocker for making rust-analyzer's proc-macro-server handling more robust.

@ehuss
Copy link
Contributor Author

ehuss commented Jul 23, 2022

It would be preferable to get this merged as soon as possible as this is technically a blocker for making rust-analyzer's proc-macro-server handling more robust.

Can you say more why that would be? I would not expect this to have any relationship with proc-macro-server. The proxy just means there is a convenient rust-analyzer exe in PATH, which is helpful for configuring r-a with non vs-code editors.

@Veykril
Copy link
Member

Veykril commented Jul 23, 2022

Reason being we would use the component to also spawn the proc-macro-srv from within r-a, judging from zulip right now we might just go with a standalone proc-macro-srv component though so I retract that statement (we assumed that would take a lot longer to coordinate but that might not be the case)

@ehuss
Copy link
Contributor Author

ehuss commented Jul 23, 2022

Based on the zulip conversation, there seems to be some confusion about how rustup works. There is already a component for rust-analyzer. Today, you can run rustup which rust-analyzer to get its path and run it. That's no different from having this PR merged and running rust-analyzer directly instead.

I agree with pietro that if you can include a small binary in the sysroot, that would be better, though.

@fasterthanlime
Copy link

I agree with pietro that if you can include a small binary in the sysroot, that would be better, though.

Based on the zulip conversation(s), we're pursuing that path in rust-lang/rust-analyzer#12858

As pointed out by @lnicola, this PR is still valuable for other editors: rust-lang/rust-analyzer#12858 (comment)

bors added a commit to rust-lang/rust-analyzer that referenced this pull request Jul 26, 2022
Add `rust-analyzer-proc-macro-srv` binary, use it if found in sysroot

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.
@ehuss
Copy link
Contributor Author

ehuss commented Aug 21, 2022

@kinnison Just checking in to see if we can get this merged, and get a new release in the next few weeks.

@kinnison
Copy link
Contributor

@ehuss I thought r-a had switched to some kind of thing in the compiler component for proc-macros -- do we still want this?

@fasterthanlime
Copy link

As pointed out by @lnicola, this PR is still valuable for other editors: rust-lang/rust-analyzer#12858 (comment)

@kinnison yes.

@kinnison kinnison merged commit 8f6b536 into rust-lang:master Aug 27, 2022
@cuviper
Copy link
Member

cuviper commented Sep 6, 2022

The rust-analyzer component has left preview mode as of rust-lang/rust#98640, which should be part of the 1.64 stable release (September 22, 2022). I think it would be nice to have rustup support that around the same time.

Nice to see the proxy merged -- is there a rustup release planned before 1.64 comes out?

@David-Else
Copy link

David-Else commented Mar 23, 2023

Please could someone explain the current status of rust-analyzer and rustup? I can find no mention of rust-analyzer in the rustup release notes, do I still need:

rustup component add rust-analyzer
sudo ln -s "$(rustup which rust-analyzer)" $bin_install_folder/rust-analyzer

Thanks.

@ehuss
Copy link
Contributor Author

ehuss commented Mar 23, 2023

@David-Else The rust-analyzer proxy should be available in the next release (1.26) which is currently going through the motions of being released, and should be available soon.

@prez
Copy link

prez commented Jul 17, 2023

I am on 1.26.0, why do I still need to symlink it manually into ~/.cargo/bin after rustup component add rust-analyzer?

@fasterthanlime
Copy link

I am on 1.26.0, why do I still need to symlink it manually into ~/.cargo/bin after rustup component add rust-analyzer?

I don't know the exact behavior here, but afaik rustup tries not to clobber anything in ~/.cargo/bin with proxy binaries.

On a recently reinstalled macOS system, ~/.cargo/bin has the proxy:

❯ rustup --version
rustup 1.26.0 (2023-04-05)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.70.0 (90c541806 2023-05-31)

❯ which rust-analyzer
/Users/amos/.cargo/bin/rust-analyzer

❯ rust-analyzer --version
error: 'rust-analyzer' is not installed for the toolchain 'stable-aarch64-apple-darwin'

@ehuss
Copy link
Contributor Author

ehuss commented Jul 17, 2023

Correct, when rustup updates it won't overwrite any files that were previously added.

Just to clarify, if you want to fix your installation, delete your old ~/.cargo/bin/rust-analyzer and run rustup self update. That should set up the proxy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants