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

remove deprecated tool rls #126856

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Jun 23, 2024

This tool has been deprecated for two years and now it only gives warning without doing anything useful.

Potential issues for users who still depend on rls:

1- If they are building rls with bootstrap, the build will fail.
2- Having rls in their toolchain will break the toolchain update.

For the issue 1, adding a change tracker warning should be enough, I think? As for issue 2, it should be straightforward to identify and fix the problem (which is removing rls from the toolchain). If rls is not only installed but also actively used, users will need to migrate to rust-analyzer anyway. For the issue 2, we will have to handle this (and any other deprecated components) from the rustup side.

Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/excluding.20rls.20from.20the.20release

@rustbot
Copy link
Collaborator

rustbot commented Jun 23, 2024

r? @clubby789

rustbot has assigned @clubby789.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 23, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 23, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rust-log-analyzer

This comment has been minimized.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

For the issue 2, we will have to handle this (and any other deprecated components) from the rustup side.

I will change the PR label as blocked until rustup have this.

@onur-ozkan onur-ozkan added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2024
@rami3l
Copy link
Member

rami3l commented Jun 28, 2024

@rbtcollins and I see no obvious downsides/blockers on Rustup's side regarding the removal of the "dummy" RLS.

The removal of RLS' shim created by Rustup itself is another issue though, which will be discussed in detail at rust-lang/rustup#3316 instead. Before that, you're still expected to see an rls-named binary on the PATH, and running it will cause Rustup to warn you about a not-yet-installed component.

Just tried the following on my machine:

> rls
error: 'rls' is not installed for the toolchain 'stable-aarch64-apple-darwin'.
To install, run `rustup component add rls`

> eza -lah (which rls)
Permissions Size User   Date Modified Name
lrwxr-xr-x@    - rami3l 13 Feb  2023  ~/.cargo/bin/rls -> /opt/homebrew/bin/rustup-init

@onur-ozkan
Copy link
Member Author

When we remove the dummy rls (by deleting the rls source code from the repository) and stop including it in the release builds, what will users get when they invoke rustup component add rls ?

@rami3l
Copy link
Member

rami3l commented Jun 28, 2024

When we remove the dummy rls (by deleting the rls source code from the repository) and stop including it in the release builds, what will users get when they invoke rustup component add rls ?

@onur-ozkan The same thing has happened before when clippy was missing from several nightlies. No big deal.

This time, just imagine clippy is missing from all the nightlies starting from a certain date, and that's exactly what it'll be like1. Those who want to install rls will get an error when rls is not present in that channel, but we should also not break the rare use case where people really want to install from a channel before rust-analyzer was a thing...

I don't think a new deprecation warning in Rustup is worth our while: the dummy RLS has been a thing for such a long time after all.

Footnotes

  1. I'm simplifying a bit: for nightlies this is not a hard error, and the updater will happily update the current toolchain to the latest one with the component(s) in question; for betas/stables however, this is expected to be a hard error.

@bors
Copy link
Contributor

bors commented Jun 28, 2024

☔ The latest upstream changes (presumably #126701) made this pull request unmergeable. Please resolve the merge conflicts.

@onur-ozkan
Copy link
Member Author

@onur-ozkan The same thing has happened before when clippy was missing from several nightlies. No big deal.

This time, just imagine clippy is missing from all the nightlies starting from a certain date, and that's exactly what it'll be like. Those who want to install rls will get an error when rls is not present in that channel, but we should also not break the rare use case where people really want to install from a channel before rust-analyzer was a thing...

I don't think a new deprecation warning in Rustup is worth our while: the dummy RLS has been a thing for such a long time after all.

Some of us were concerned that when people attempt to do rustup upgrades with rls component in their toolchain (even if they don't use it), this will cause a break. So, if rustup could provide a clear warning (e.g., "rls is no longer available; it will be skipped") without breaking the process, that would be useful and we could ship this PR without any hesitation.

@rami3l
Copy link
Member

rami3l commented Jul 7, 2024

@onur-ozkan I've made rust-lang/rustup#3920 trying to address this concern. Where do you think we should document the removed components though? I initially proposed the Rustup Book but it turned out that the book is updated per Rustup version so that might still cause problems if you remove components in the middle of a release cycle.

https://rust-lang.github.io/rustup-components-history looks better although I have no idea who actually controls it 👀

@onur-ozkan
Copy link
Member Author

I initially proposed the Rustup Book but it turned out that the book is updated per Rustup version so that might still cause problems if you remove components in the middle of a release cycle.

It's already documented there https://rust-lang.github.io/rustup/concepts/components.html#previous-components.

https://rust-lang.github.io/rustup-components-history looks better

The page only shows the last 7 days for nightly. Do you mean you want to add a new section 'Removed Components' there?

although I have no idea who actually controls it 👀

From what I can see on the rustup-components-history, @Mark-Simulacrum is one of its maintainers.

@rami3l
Copy link
Member

rami3l commented Jul 7, 2024

I initially proposed the Rustup Book but it turned out that the book is updated per Rustup version so that might still cause problems if you remove components in the middle of a release cycle.

It's already documented there https://rust-lang.github.io/rustup/concepts/components.html#previous-components.

@onur-ozkan Yes, I'm aware of that, however that doesn't represent the latest list due to its release cycle (plus many maintainers don't bother to update the list of components here, so it's missing quite a few new ones).

We do have an up-to-date list though... located at https://rust-lang.github.io/rustup/devel/concepts/components.html#previous-components and is compiled right from the master branch, but I'm afraid that might cause confusion, as historically https://rust-lang.github.io/rustup/devel has always been undocumented and hidden from public view, since it might contain changes that only apply to the dev builds of Rustup.

OTOH I can change that if you still consider the Rustup Book a better place... In that case I might have to add a hyperlink to https://rust-lang.github.io/rustup/devel/concepts/components.html#previous-components.

https://rust-lang.github.io/rustup-components-history looks better

The page only shows the last 7 days for nightly. Do you mean you want to add a new section 'Removed Components' there?

Yes, my idea is that since the build actually stops on nightly first, doing that will have no problems with visibility.

although I have no idea who actually controls it 👀

From what I can see on the rustup-components-history, @Mark-Simulacrum is one of its maintainers.

Thanks! @Mark-Simulacrum do you happen to have any opinions to share on this one?

@rbtcollins
Copy link
Contributor

I think we're overthinking this.

The original concerns were:

1- If they are building rls with bootstrap, the build will fail.
2- Having rls in their toolchain will break the toolchain update.

For 1) that is a source-only concern.
For 2) rustup won't error, and by definition they must be upgrading on a channel (specific version toolchains won't get updates like this) - which means that to have this suddenly break, the user must have:

  • been using stable, and not upgraded their toolchain for > 2 years
  • be dependent on rls

The problem I see with depending on rustup to show a message is that rustup updates are often done headless - e.g. underneath vscode, or in github actions.

Sure, we can do it, but is this particular case going to show a message to the right people at the right time - and we'll have the complexity of that message in the system forever, since rustup supports installing very rust version ever.

And also, not all rust users use rustup - adding a message to rustup won't inform people using rpm or deb packages about the removal at all.

We've advertised this removal for years now....

IMO: just ship the removal. Add a clear note to the version release notes for the subsequent release.

@rbtcollins
Copy link
Contributor

Oh, one other issue - the approach of blocking this PR on rustup showing announcements is that rustup is self-updated after toolchains are updated. So people that run 'rustup update' when they realise they are missing the current version, will get the version, with the rls removal, and the existing message ... and then rustup will update.

As we have no daemon or cron-check of rustup versions, we have no way to ensure that people are likely to have updated it before updating rust. (We can of course do a new release in advance, but with the behaviour pattern above, this probably needs a full rust release cycle to have a good chance of being distributed in advance).

And relatedly, homebrew, snaps, and other distribution managed versions of rustup will update on their own schedule, so we'd need to coordinate with them to do a backport to their stable versions of their distributions, before rustup will be updated for their users. (Ignoring the related question of -when- their users will update rustup)

@rami3l
Copy link
Member

rami3l commented Jul 8, 2024

I agree with @rbtcollins that rust-lang/rustup#3920 should not become a blocker to removing rls, as it solely exists as a UX improvement.

@ehuss
Copy link
Contributor

ehuss commented Jul 8, 2024

or 2) rustup won't error, and by definition they must be upgrading on a channel (specific version toolchains won't get updates like this) - which means that to have this suddenly break, the user must have:

* been using stable, and not upgraded their toolchain for > 2 years

I'm not quite understanding what this is trying to say. If we remove the rls component, and If someone has the rls component installed, and they try to update, it should error, correct? Why do you say they must not have updated in >2 years?

@rami3l
Copy link
Member

rami3l commented Sep 30, 2024

Now that rust-lang/rustup#3920 has been merged and rust-lang/rustup-components-history#54 is available for review, do we have other blockers regarding this change? :)

@onur-ozkan
Copy link
Member Author

Now that rust-lang/rustup#3920 has been merged and rust-lang/rustup-components-history#54 is available for review, do we have other blockers regarding this change? :)

Seems like we are ready to land this. Thanks a lot for your effort! I will rebase this PR later today.

@rami3l
Copy link
Member

rami3l commented Sep 30, 2024

Now that rust-lang/rustup#3920 has been merged and rust-lang/rustup-components-history#54 is available for review, do we have other blockers regarding this change? :)

Seems like we are ready to land this. Thanks a lot for your effort! I will rebase this PR later today.

Thanks! It just occurred to me that it'd probably be better if we wait for Rustup v1.28 for a while before actually making this change? A beta release is on the horizon: https://github.com/rust-lang/rustup/milestone/8

@onur-ozkan
Copy link
Member Author

Sure, makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants