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.

Sign up for GitHub

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

Jump to bottom

RLS support #1005

Merged
merged 4 commits into from
Mar 28, 2017
Merged

RLS support #1005

merged 4 commits into from
Mar 28, 2017

Conversation

brson
Copy link
Contributor

@brson brson commented Mar 23, 2017

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

@brson
Add the rls bin to installation
61926e6
@brson
Fix the self-update tests to use the correct version of the bins
e937b10 
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
Copy link
Contributor Author

brson commented Mar 23, 2017

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

@Diggsey
Copy link
Contributor

Diggsey commented Mar 23, 2017 •
edited
Loading

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

@brson
Copy link
Contributor Author

brson commented Mar 23, 2017

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
Copy link
Contributor

Diggsey commented Mar 23, 2017

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

brson commented Mar 23, 2017

Uh, yeah I think you are right about that. Wow. I should sleep on this.

@brson
Roll back test suite changes, roll back lazy rls upgrade
1c9f146 
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
Copy link
Contributor Author

brson commented Mar 27, 2017

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

brson commented Mar 27, 2017

@Diggsey r?

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

@Diggsey
Copy link
Contributor

Diggsey commented Mar 27, 2017

@bors r LGTM

@bors
Copy link
Contributor

bors commented Mar 27, 2017

📌 Commit 1c9f146 has been approved by Diggsey

@bors
Copy link
Contributor

bors commented Mar 27, 2017

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

bors added a commit that referenced this pull request Mar 27, 2017
@bors
Auto merge of #1005 - brson:rls, r=Diggsey
4e0c2a4 
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
Copy link
Contributor

bors commented Mar 27, 2017

💔 Test failed - status-appveyor

@brson
Copy link
Contributor Author

brson commented Mar 28, 2017

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

bors commented Mar 28, 2017

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

bors added a commit that referenced this pull request Mar 28, 2017
@bors
Auto merge of #1005 - brson:rls, r=Diggsey
55ea19a 
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
Copy link
Contributor

bors commented Mar 28, 2017

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

@bors bors merged commit 1c9f146 into rust-lang:master Mar 28, 2017
@bors bors mentioned this pull request Mar 28, 2017
Merged
@durka durka mentioned this pull request Mar 29, 2017
Merged
bors added a commit that referenced this pull request Apr 1, 2017
@bors
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brson @Diggsey @bors @alexcrichton