feat(toolchain): consider external rust-analyzer when calling a proxy
#4324
Conversation
rami3l
force-pushed
the
feat/smarter-ra-shim
branch
from
6fa6a79 to
9fd24ff
Compare
rami3l
force-pushed
the
feat/smarter-ra-shim
branch
4 times, most recently
from
19cc68b to
15569c1
Compare
rami3l
force-pushed
the
feat/smarter-ra-shim
branch
from
15569c1 to
a45dd3f
Compare
| // Try to find the first `rust-analyzer` under the `$PATH` that is both | ||
| // an existing file and not the same file as `me`, i.e. not a rustup proxy. | ||
| Ok(env::split_paths(&path).find_map(|mut p| { | ||
| p.push(binary); |
There was a problem hiding this comment.
We might want to check the executable bit of this path. And maybe show the first nonexecutable file path when nothing found.
There was a problem hiding this comment.
I'm not too sure about that. If something is in PATH the expectation is that it's executable. It's up to the OS to determine if it actually is or not and I don't think it's useful to second guess it.
There was a problem hiding this comment.
I agree this should look more like which in that only executables can be considered candidates (provided that it's as platform-agnostic as it gets). However I'm not very sure about the second half...
There was a problem hiding this comment.
And most shells have the behavior, as well as execvp on Linux.
The other way is falling back to call rust-analyzer directly and let OS do whatever it wants, so we don't need to mimic the PATH lookup logic.
edit: the fallback won't work. It'll become an infinite loop… 😆
There was a problem hiding this comment.
From experience I'm very leery of look-before-you-leap because even if you ignore toctou, there's opportunities for things to go wrong on weird filesystems or setups.
Btw, I just tried in bash and it just runs a script without the executable bit (well, it fails, but it tries) and does not read the rest of the PATH.
There was a problem hiding this comment.
Btw, I just tried in bash and it just runs a script without the executable bit (well, it fails, but it tries) and does not read the rest of the PATH.
I just tried the same on my Mac and I believe that's not the case at least on my machine: a simple touch rust-analyzer can change the interpretation of calling that command depending on chmod +x and chmod -x.
There was a problem hiding this comment.
Seems like there's a difference between bash and which:
$ which biscuits
/home/chris/.cargo/bin/biscuits
$ biscuits
-bash: /home/chris/temp/biscuits: Permission denied
There was a problem hiding this comment.
@ChrisDenton
Did you have another biscuits executable with a lower precedence in your PATH?
That was not my case with bash 5.2.37 on both macOS 15.4.1 and Linux 5.10.235. Not sure what was different 🤔
There was a problem hiding this comment.
Oh I see. /home/chris/.cargo/bin/biscuits is the executable
There was a problem hiding this comment.
I'm not quite sure, but anyway this is what I've tried, and it seems to be coherent with what which gives (this ~/.moon/bin has higher precedence than the rust-analyzer proxy, of course):
> printf '#!/bin/sh\necho Hello World' > ~/.moon/bin/rust-analyzer
> chmod +x ~/.moon/bin/rust-analyzer
> rust-analyzer
Hello World
> chmod -x ~/.moon/bin/rust-analyzer
> rust-analyzer
error: Unknown binary 'rust-analyzer' in official toolchain 'stable-aarch64-apple-darwin'.Maybe that's because I'm using No, same behavior using fish?zsh as well.
rami3l
force-pushed
the
feat/smarter-ra-shim
branch
from
a45dd3f to
a3330ad
Compare
Closes #3846.
Following the reasoning in #3846 (comment), the current implementation prioritizes rustup-hosted RA and only delegates to the outside as a final rescue. This way, it will also close #3299.
This approach has also been approved by @davidbarsky in #t-rustup > More frequent rust-analyzer updates in rustup @ 💬 as a temporary measure before the new update cadence for RA on our release server is settled.
Concerns
cargofallback.