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

Binary named rust-analyzer added to cargo bin path even when component is not added #3846

Open
2 tasks done
praveenperera opened this issue May 29, 2024 · 10 comments
Open
2 tasks done
Labels
Milestone

Comments

@praveenperera
Copy link

praveenperera commented May 29, 2024

Verification

Problem

Rustup keeps adding a binary named rust-analyzer to the .cargo/bin folder. But I didn't add it because I want homebrew to manage rust-analyzer version, because I want the latest version. But since there is a rust-analyer in my path at ~/.cargo/bin/rust-analyzer my editor tries to use it.

Opening it i get a message

error: Unknown binary 'rust-analyzer' in official toolchain 'stable-aarch64-apple-darwin'.
To install, run `rustup component add rust-analyzer`

This is very annoying because I keep having to delete this dummy binary everytime i run rustup so my editor can find the actual binary. Is there any reason to create these dummy binaries? Can I disable this?

Steps

  1. Install rustup with with minimal

Possible Solution(s)

No response

Notes

No response

Rustup version

rustup 1.27.1 (54dd3d00f 2024-04-24)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.78.0 (9b00956e5 2024-04-29)`

Installed toolchains

Default host: aarch64-apple-darwin
rustup home:  /Users/praveen/.rustup

stable-aarch64-apple-darwin (default)
rustc 1.78.0 (9b00956e5 2024-04-29)

OS version

MacOS Version 14.5 (23F79)
@praveenperera
Copy link
Author

praveenperera commented May 29, 2024

I can make this less annoying by putting homebrew path before the cargo path, but I don't understand why its installing these dummy binaries into the PATH in the first place

@rami3l
Copy link
Member

rami3l commented May 29, 2024

@praveenperera Thanks for filing this issue!

Is there any reason to create these dummy binaries?

Yes, since rustup would want to add a shim for rust-analyzer.

Can i disable this?

AFAIK you can't, unfortunately. However this point has been pointed out in the original PR by @ehuss:

This adds rust-analyzer as a proxy.
...
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.
#3022 (comment)

If you are a VSCode user, it would be okay if you just use the plugin-bundled version of rust-analyzer, as it's guaranteed to be up-to-date. If you are not, well, to be frank I think this is a point to be improved, since I also use a non-Rustup distribution of rust-analyzer (from Neovim's mason to be precise).

@rbtcollins Do you think it'll be fine if we only add shims for installed components (or DUP_TOOLS, to be more specific)? Will that disrupt our current stability guarantees/business logic too much?

@rami3l rami3l modified the milestones: 1.29.0, 1.28.0 May 29, 2024
@praveenperera
Copy link
Author

praveenperera commented May 29, 2024

Thanks, ya I meant is there a reason that the dummy binary needs to added when the component is not added?

I guess the current method gives a message that shows people how to install it using rustup, but I don't know if thats a worthwhile tradeoff. Unless theres other benefits I'm missing?

@rami3l
Copy link
Member

rami3l commented May 29, 2024

@praveenperera Looking at

for tool in DUP_TOOLS {
let tool_path = bin_path.join(&format!("{tool}{EXE_SUFFIX}"));
if let Ok(handle) = Handle::from_path(&tool_path) {
// Like above, don't clobber anything that's already hardlinked to
// avoid extraneous errors from being returned.
if rustup == handle {
continue;
}
// If this file exists and is *not* equivalent to all other
// preexisting tools we found, then we're going to assume that it
// was preinstalled and actually pointing to a totally different
// binary. This is intended for cases where historically users
// ran `cargo install rustfmt` and so they had custom `rustfmt`
// and `cargo-fmt` executables lying around, but we as rustup have
// since started managing these tools.
//
// If the file is managed by rustup it should be equivalent to some
// previous file, and if it's not equivalent to anything then it's
// pretty likely that it needs to be dealt with manually.
if tool_handles.iter().all(|h| *h != handle) {
warn!("tool `{}` is already installed, remove it from `{}`, then run `rustup update` \
to have rustup manage this tool.",
tool, bin_path.display());
continue;
}
}
utils::hard_or_symlink_file(&rustup_path, &tool_path)?;
}

... a possible workaround would be manually adding a symlink under ~/.cargo/bin/ pointing to wherever your actual rust-analyzer installation would be.

@djc
Copy link
Contributor

djc commented May 29, 2024

@rbtcollins Do you think it'll be fine if we only add shims for installed components (or DUP_TOOLS, to be more specific)? Will that disrupt our current stability guarantees/business logic too much?

Don't we decide which components to install on a per-toolchain basis? If that is the case, it would be tricky to decide which shims to install.

@rami3l
Copy link
Member

rami3l commented May 29, 2024

@rbtcollins Do you think it'll be fine if we only add shims for installed components (or DUP_TOOLS, to be more specific)? Will that disrupt our current stability guarantees/business logic too much?

Don't we decide which components to install on a per-toolchain basis? If that is the case, it would be tricky to decide which shims to install.

@djc Indeed, although this is a qualitative description.

I'm not sure about this idea security-wise, but I was thinking about implementing a rust-analyzer shim that, right before throwing the aforementioned "binary not found" error, turns into a "transparent" shim and tries to call rust-analyzer once more but without searching ~/.cargo/bin/. This behavior is per-toolchain because with a toolchain that has rust-analyzer installed, the error would't have been thrown in the first place.

If you think that sounds too disruptive (or even unrealistic), another solution could be confirming and solidifying the hack in #3846 (comment). Maybe by adding a help message or something.

@djc
Copy link
Contributor

djc commented May 29, 2024

I'm not sure about this idea security-wise, but I was thinking about implementing a rust-analyzer shim that, right before throwing the aforementioned "binary not found" error, turns into a "transparent" shim and tries to call rust-analyzer once more but without searching ~/.cargo/bin/. This behavior is per-toolchain because with a toolchain that has rust-analyzer installed, the error would't have been thrown in the first place.

Sounds reasonable to me... Security-wise it seems okay to me because we're merely relying on the PATH that the user has set anyway (and so implicitly trusts).

@rbtcollins
Copy link
Contributor

Fundamentally this is out of our hands. Either the rust project is shipping rust analyser as part of versioned releases, in which case the shim is needed to permit running the command. Or it is not, in which case we can remove the shim.

I think it is reasonable for things like editor plugins which choose to manage a single version of rust analyser to +not+ depend on PATH to locate it, since there are many such editor plugins, as well as single version distributions like homebrew.

@rbtcollins
Copy link
Contributor

I'm not sure if the bug reported here is 'i get an error', or, 'the wrong version runs': consider that we might teach this one proxy to fall back to path, but then the bug changes to be 'wrong version because I had the component installed'.

I suggest finding out the plans of rust analyser.

If the situation is transitory , I recommend doing nothing.. Users can put their own bin before the cargo bin anyway.

If we add special case complexity, it affects our users as it becomes one more thing they have to learn about when the system behaves in a surprising manner.

@rami3l rami3l modified the milestones: 1.28.0, 1.28.1 Aug 9, 2024
@KorigamiK
Copy link

error: Unknown binary 'rust-analyzer' in official toolchain 'stable-aarch64-apple-darwin'.
To install, run rustup component add rust-analyzer

I can confirm this issue on linux. However I don't seem to get the suggestion to add the component.

Unknown binary 'rust-analyzer' in official toolchain '1.80-x86_64-unknown-linux-gnu'

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

No branches or pull requests

5 participants