-
Notifications
You must be signed in to change notification settings - Fork 877
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
RLS support #1005
RLS support #1005
Conversation
I realized that, even in the installer tests, most of the binaries being run are out of the clitest 'exedir', not from CARGO_HOME/bin. So this patch fixes that by modifying the generic test runner to run bins off of the PATH, with PATH configured by default prefixed with 'exedir'. The cli-self-upd tests extend PATH with CARGO_HOME/bin.
Hm, this has a bug in it from the changes to the test suite. |
Is |
The self-update will not on its own create the new proxy. |
Isn't it the new |
Uh, yeah I think you are right about that. Wow. I should sleep on this. |
The lazy upgrade isn't needed, per Diggsey. The test suite changes are too complex to deal with right now, and introduce the risk of the environment interfering with the tests (which is why the tests are all red on the bots but green locally).
OK, I pushed some hasty changes to try to fix the failing tests, and remove the lazy upgrade. Attempting to make the test suite run bins off the PATH ended up causing all kinds of problems with the environment seeping in, so I just rolled that back. rustup is really frustrating to test... |
@Diggsey r? I've really lost perspective on this patch, just throwing crap at it and hoping it works out. |
@bors r LGTM |
📌 Commit 1c9f146 has been approved by |
RLS support I think this is sufficient for basic support. With this PR, rustup will create 'rls' proxies in CARGO_HOME/bin, and will do so lazily when that binary doesn't exist. After this PR one can write ``` rustup component add rls rustup component add rust-src rustup component add rust-analysis ``` to get a working RLS, assuming the 'rls' package is deployed. Next steps are to make `rustup component` accept multiple components, so RLS can be installed with just `rustup compnent add rls rust-src rust-analysis` (cc @durka). I'd suggest that RLS have nice error handling for cases where rust-src and rust-analysis aren't available. It might suggest running the proper rustup commands. rustup probably needs to emit a better error message when somebody tries to run `rls` without installing it. Since the proxy always exists, it will say something like "the binary 'rls' doesn't exist in toolchain X". It would be better for rustup to know which package contains 'rls' and say so. cc @nrc @jonathandturner
💔 Test failed - status-appveyor |
@bors retry weird error:
|
RLS support I think this is sufficient for basic support. With this PR, rustup will create 'rls' proxies in CARGO_HOME/bin, and will do so lazily when that binary doesn't exist. After this PR one can write ``` rustup component add rls rustup component add rust-src rustup component add rust-analysis ``` to get a working RLS, assuming the 'rls' package is deployed. Next steps are to make `rustup component` accept multiple components, so RLS can be installed with just `rustup compnent add rls rust-src rust-analysis` (cc @durka). I'd suggest that RLS have nice error handling for cases where rust-src and rust-analysis aren't available. It might suggest running the proper rustup commands. rustup probably needs to emit a better error message when somebody tries to run `rls` without installing it. Since the proxy always exists, it will say something like "the binary 'rls' doesn't exist in toolchain X". It would be better for rustup to know which package contains 'rls' and say so. cc @nrc @jonathandturner
☀️ Test successful - status-appveyor, status-travis |
I think this is sufficient for basic support.
With this PR, rustup will create 'rls' proxies in CARGO_HOME/bin, and will do so lazily when that binary doesn't exist.
After this PR one can write
to get a working RLS, assuming the 'rls' package is deployed.
Next steps are to make
rustup component
accept multiple components, so RLS can be installed with justrustup compnent add rls rust-src rust-analysis
(cc @durka).I'd suggest that RLS have nice error handling for cases where rust-src and rust-analysis aren't available. It might suggest running the proper rustup commands.
rustup probably needs to emit a better error message when somebody tries to run
rls
without installing it. Since the proxy always exists, it will say something like "the binary 'rls' doesn't exist in toolchain X". It would be better for rustup to know which package contains 'rls' and say so.cc @nrc @jonathandturner