RLS support by brson · Pull Request #1005 · rust-lang/rustup

brson · 2017-03-23T01:01:58Z

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

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.

brson · 2017-03-23T01:07:15Z

Hm, this has a bug in it from the changes to the test suite.

Diggsey · 2017-03-23T01:11:51Z

Is add_rls_proxy strictly necessary? Won't the new proxy be automatically created anyway when rustup updates? (it always calls install_bins)

brson · 2017-03-23T01:21:16Z

Is add_rls_proxy strictly necessary? Won't the new proxy be automatically created anyway when rustup updates (it always calls install_bins)

The self-update will not on its own create the new proxy. install_bins is run on upgrade but it is the old install bins that does not know about rls.

Diggsey · 2017-03-23T01:29:23Z

Isn't it the new install_bins that gets run? The previous rustup calls the new one with --self-replace, and that one updates the hardlinks.

brson · 2017-03-23T02:10:43Z

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).

brson · 2017-03-27T02:07:15Z

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...

brson · 2017-03-27T17:22:54Z

@Diggsey r?

I've really lost perspective on this patch, just throwing crap at it and hoping it works out.

Diggsey · 2017-03-27T17:32:03Z

@bors r+ LGTM

bors · 2017-03-27T17:32:04Z

📌 Commit 1c9f146 has been approved by Diggsey

bors · 2017-03-27T17:32:09Z

⌛ Testing commit 1c9f146 with merge 4e0c2a4...

@durka

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

bors · 2017-03-27T18:38:16Z

💔 Test failed - status-appveyor

brson · 2017-03-28T01:32:48Z

@bors retry

weird error:

---- multirust_upgrade_works_with_proxy stdout ----
thread 'multirust_upgrade_works_with_proxy' panicked at 'assertion failed: fs::symlink_metadata(&multirust_dir).unwrap().file_type().is_symlink()', tests\cli-rustup.rs:739

bors · 2017-03-28T02:42:03Z

⌛ Testing commit 1c9f146 with merge 55ea19a...

@durka

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

bors · 2017-03-28T04:08:37Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Diggsey
Pushing 55ea19a to master...

Add/remove multiple components+targets Same as #986 but for components and targets. After this I don't see any more places to add `.multiple(true)`. cc #1005

brson added 2 commits March 23, 2017 00:54

Add the rls bin to installation
61926e6

Fix test suite bug
19ec923

alexcrichton approved these changes Mar 23, 2017
View reviewed changes

bors merged commit 1c9f146 into rust-lang:masterMar 28, 2017

bors mentioned this pull request Mar 28, 2017
Check for unexpected cargo/rustc before install #705
Merged

durka mentioned this pull request Mar 29, 2017
Add/remove multiple components+targets #1016
Merged

bors added a commit that referenced this pull request Apr 1, 2017
Auto merge of#1016- durka:component-multiple, r=Diggsey
af52e4d
Add/remove multiple components+targets Same as #986 but for components and targets. After this I don't see any more places to add `.multiple(true)`. cc #1005

Uh oh!

Uh oh!

Conversation

brson commented Mar 23, 2017

Uh oh!

brson commented Mar 23, 2017

Uh oh!

Diggsey commented Mar 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brson commented Mar 23, 2017

Uh oh!

Diggsey commented Mar 23, 2017

Uh oh!

brson commented Mar 23, 2017

Uh oh!

brson commented Mar 27, 2017

Uh oh!

brson commented Mar 27, 2017

Uh oh!

Diggsey commented Mar 27, 2017

Uh oh!

bors commented Mar 27, 2017

Uh oh!

bors commented Mar 27, 2017

Uh oh!

bors commented Mar 27, 2017

Uh oh!

brson commented Mar 28, 2017

Uh oh!

bors commented Mar 28, 2017

Uh oh!

bors commented Mar 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Diggsey commented Mar 23, 2017•
edited
Loading