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

Decouple nightly RLS from Clippy #59761

Closed
Xanewok opened this issue Apr 6, 2019 · 15 comments
Closed

Decouple nightly RLS from Clippy #59761

Xanewok opened this issue Apr 6, 2019 · 15 comments
Labels
P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.

Comments

@Xanewok
Copy link
Member

Xanewok commented Apr 6, 2019

Context

As you must've noticed the RLS and Clippy are not shipped with nightly builds for quite a few releases now. This happens when a change lands to the Rust compiler and a tool cannot be built or its test fail when using the currently built compiler. Currently, the RLS explicitly depends (in Cargo sense) on Clippy, which means that whenever Clippy cannot build, the RLS also fails the build and is thus not included in the nightlies.

In practice, there's a lot more breakage involved for the Clippy since it uses rustc internals quite heavily (e.g. directly inspects AST/HIR structures), while the problem is sidestepped by the RLS by relying on the internal Rust librustc_save_analysis library doing all the heavy lifting and outputting relevant information to JSON.

While specifically the current breakage requires more work/coordination (caused by moving libtest out of tree, please help with #59440 if you know how!) and has caused more fallout than typical breakages, and regardless of the fact that we don't guarantee component availability for the nightly channel, I'd like us to try our best and ship RLS if possible, even without Clippy support.

Implementation

Implementation-wise I believe this can be done by running Clippy tool test first and if it's detected as failed by rustbuild, then it will try to build RLS in the --no-default-features mode (Clippy support is gated by the only default clippy feature). When RLS is built without Clippy support, we can warn the user on start up that that version has no Clippy support enabled because it's not available in that build.

Alternatives

Create a separate rustup-complete channel that's only released with all the tools

While this may be a somewhat viable or desired idea:

  • IIRC the current breakage lasts for 1-2 weeks now which is the 1/3 of a version release cycle (...which makes it more of a fortnightly release)
  • requires more infrastructure/implementation effort in the rustup/rustbuild
  • doesn't solve the case for the regular nightly release, which is often the case for people actively working on the compiler or related tooling (hurts contributability)

Do nothing

We just 🤷‍♂️ and tell people to move off nightly or pin to a specific nightly release.

That's not great because there's still a fair chunk of the ecosystem that's bound to nightly features. If we make them pin to a specific release, then there's UX/discoverability issue.

Right now, when the user either tries to update the preexisting toolchain or install a fresh one with RLS, they fail with an uninformative "rls component is unavailable" message. Then, they need to either learn about or navigate to https://rust-lang.github.io/rustup-components-history/ to learn which was the most recent toolchain with RLS shipped (even then, right now it's impossible since the history shows "missing" for the last 8 ones) or find and use a script that parses the recent nightly build manifests and finds the most recent one. And that's for every tool breakage, which can happen quite often.

Thoughts?

cc @nrc @oli-obk @rust-lang/infra @rust-lang/dev-tools

@Xanewok Xanewok added A-rls T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Apr 6, 2019
@Manishearth
Copy link
Member

FWIW the current breakage is very much not the norm, it's due to some complex things with the libtest stuff and a lack of time from the people who understand rust's build system. It took this long because we kept waiting on others, otherwise clippy breakages get fixed in one or two days.

We're reverting the libtest stuff so that clippy can work again.

Create a separate rustup-complete channel that's only released with all the tools

This is being worked on too.

@Manishearth
Copy link
Member

Overall the UX experience of nightly users who want to use tooling is actively being worked on by @kinnison , the rough plan right now is to have rust publish a "last known good" nightly version for a subset of tools, and teach rustup about it.

Overall we seem to be shifting towards shipping RLS and clippy together, always (instead of people using tools individually leading to more permutations), so the fix proposed here won't really help much once we get there.

@Xanewok
Copy link
Member Author

Xanewok commented Apr 6, 2019

FWIW the current breakage is very much not the norm

I tried to emphasize that in the OP as well, hope that's visible there

I'm also aware of the work done in the Rustup area (which is great!), however I believe there's a lot of merit of doing the best-effort thing here and keeping the RLS separate from Clippy on the build level.

I know there was an idea/conversation/RFC (by @nrc I believe) to introduce rustup "profiles", which is a set of components included in the profile (e.g. tools would include rls, clippy, rust-src, rust-lldb, ...).
This would wholly solve the another toolnightly channel and would also allow to streamline the tooling development, as you're saying is the shift now.

Now that I think about it, it seems weird for the rls component to duplicate clippy and rustfmt components internally. Right now, RLS also supports externally invoked Rustfmt - I wonder if we should try to do the same for Clippy and measure the overhead?

@Manishearth
Copy link
Member

Now that I think about it, it seems weird for the rls component to duplicate clippy and rustfmt components internally. Right now, RLS also supports externally invoked Rustfmt - I wonder if we should try to do the same for Clippy and measure the overhead?

This sounds like a good step forward.

Yeah, I'm all for separating things out, I just want to caution against doing it as a reaction to what is mostly a one-time thing.

@matthiaskrgr
Copy link
Member

Right now, when the user either tries to update the preexisting toolchain or install a fresh one with RLS, they fail with an uninformative "rls component is unavailable" message. Then, they need to either learn about or navigate to https://rust-lang.github.io/rustup-components-history/ to learn which was the most recent toolchain with RLS shipped (even then, right now it's impossible since the history shows "missing" for the last 8 ones) or find and use a script that parses the recent nightly build manifests and finds the most recent one.

Shameless plug: rust-lang/rustup#1501 would probably help here.

@kinnison
Copy link
Contributor

kinnison commented Apr 9, 2019

Overall the UX experience of nightly users who want to use tooling is actively being worked on by @kinnison , the rough plan right now is to have rust publish a "last known good" nightly version for a subset of tools, and teach rustup about it.

We're still thinking about this within wg-rustup. @nrc has some reservations with the early plans, but we're proceeding, slowly, toward something we think is plausible given the reduced bandwidth in wg-rustup.

@aidanhs
Copy link
Member

aidanhs commented Apr 16, 2019

We discussed this in the infra meeting last week - long story short, we'd prefer not to paper over the cracks with a quickfix (rustup-complete) at this point in time as it doesn't feel very high priority and it sounds like wg-rustup may be making (slow) progress on it.

@aidanhs aidanhs removed the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Apr 16, 2019
@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 18, 2019
@pnkfelix
Copy link
Member

tagging with T-compiler to make sure we double-check whether there is something w.r.t. RLS that we should be doing on our end.

@pnkfelix
Copy link
Member

pnkfelix commented May 2, 2019

I'd like to assign a priority label to this work. @Xanewok, would you object to this being considered a P-medium matter?

@Xanewok
Copy link
Member Author

Xanewok commented May 2, 2019

I'd say so - it's not crucial but it'd be good to slowly explore using rustc (and thus also Clippy) out-of-process to reduce the synchronization overhead in the RLS process itself (should considerably reduce latency for medium-sized workspaces)

@pnkfelix
Copy link
Member

discussed very briefly at T-compiler meeting. I think our team needs more concrete work items as to what we need to do here, before we can take action ourselves...

@mati865
Copy link
Contributor

mati865 commented Jun 4, 2019

@Xanewok I made small research and it seems the only required change is to remove https://github.com/rust-lang/rls/blob/bfee4d2a029e58fb793e75ee28af8eb79c2e4bae/Cargo.toml#L80

Without Clippy being default feature it will be included only when Clippy builds successfully:

rust/src/bootstrap/tool.rs

Lines 642 to 644 in c22ce28

if clippy.is_some() {
self.extra_features.push("clippy".to_owned());
}

From Clippy user point of view there is nothing to loose because their nightly update will be blocked on missing Clippy. Therefore they always get RLS with Clippy.

I can test it and open PRs if this approach is still pursued.

cc @oli-obk if this change goes in toolstate will no longer open dozens of issues regarding RLS, instead it should open issues for Clippy.

@Xanewok
Copy link
Member Author

Xanewok commented Jun 7, 2019

Huh, interesting. I believed that needed more machinery some time ago but hey if it's already here... 🤷‍♂️ I'll land the RLS PR and let's see how it goes from here.

@mati865
Copy link
Contributor

mati865 commented Jun 7, 2019

FWIW I tested it locally by breaking Clippy before opening RLS PR and it worked as expected.

@Xanewok Xanewok mentioned this issue Jun 8, 2019
Centril added a commit to Centril/rust that referenced this issue Jun 8, 2019
Update RLS

This bumps the version to 1.37 and also doesn't build clippy by default (should reduce toolstate breakages, see rust-lang#59761 (comment) for more details)

r? @oli-obk
@mati865
Copy link
Contributor

mati865 commented Jun 9, 2019

Once more Clippy broke but this time RLS still works, I think we can close this issue.

@oli-obk oli-obk closed this as completed Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants