8000 Add rust-analyzer to rust-toolchain by alevy · Pull Request #4125 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add rust-analyzer to rust-toolchain #4125

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
Merged
merged 1 commit into from
Aug 2, 2024
Merged

Add rust-analyzer to rust-toolchain #4125

merged 1 commit into from
Aug 2, 2024
+1 −1

Conversation

alevy
Copy link
Member
@alevy alevy commented Aug 1, 2024

Pull Request Overview

Adds rust-analzyer to rust-toolchain.toml, which ensures that a compatible version of the LSP server is available when using rustup

Testing Strategy

Open emacs

TODO or Help Wanted

N/A

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

8000 @alevy
Add rust-analyzer to rust-toolchain
2b39fd9 
Adding rust-analzyer to the `rust-toolchain` ensures that a compatible
version of the LSP server is available when using rustup
@alevy alevy added the P-Upkeep This a relatively minor change, or one that is limited in scope, and requires less scrutiny. label Aug 1, 2024
@lschuermann
Copy link
Member

This causes rust-analyzer to be fetched regardless of whether it's used, and adds yet another download (7.2MB) & unpack (18MB) step to things such as CI. I don't use rust-analyzer myself yet, but I thought that tools with support for rustup would fetch the correct version automatically on demand? Is there an advantage to having it pre-installed?

I'm not opposed to merging this, I guess I just don't like the prospect of making users download & install anything that's not directly related to building the OS. Maybe that's the wrong mindset and what's in rust-toolchain.toml in upstream Tock should be for development, and users ought to customize this file for just building the OS.

@lschuermann lschuermann assigned lschuermann and unassigned bradjc Aug 2, 2024
bradjc
bradjc approved these changes Aug 2, 2024
View reviewed changes
Copy link
Contributor
@bradjc bradjc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me.

The reason why adding components to rust-toolchain is important is it ensures that when we update nightlies we choose one that has all of these components enabled. 7.2/18 MB seems like a pretty small price to pay.

lschuermann
lschuermann approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member
@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is probably a net positive. It's a shame that we can't trivially distinguish between build & dev components. 😕

@alevy alevy added this pull request to the merge queue Aug 2, 2024
Merged via the queue into tock:master with commit 6c2166e Aug 2, 2024
12 checks passed
@alevy alevy deleted the rust-analyzer branch August 2, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradjc bradjc bradjc approved these changes

@alexandruradovici alexandruradovici alexandruradovici approved these changes

@lschuermann lschuermann lschuermann approved these changes

Assignees

@lschuermann lschuermann

Labels
P-Upkeep This a relatively minor change, or one that is limited in scope, and requires less scrutiny.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alevy @lschuermann @bradjc @alexandruradovici
0