Skip to content

Fix rust analyzer #911

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

Closed
wants to merge 7 commits into from
Closed

Fix rust analyzer #911

wants to merge 7 commits into from

Conversation

jackos
Copy link
Contributor

@jackos jackos commented Jan 11, 2022

Fix rustlings exercises to work with rust-analyzer if it's installed.

It generates a rust-project.json and links to the default rustup toolchain if it exists.

If there are any errors e.g. rustup or rust-analyzer don't exist, the fix is skipped with a message saying so.

When there is a success it will generate the file and will no longer run.

New argument --skipfix (-x for short) so that the file isn't generated e.g. when developing the main binary you don't want this file in the project root or rust-analyzer won't work.

Added tests for the functionality.

@manyinsects
Copy link
Member

Looks good! Thanks for finally solving this long-standing issue. One thing: You've committed your own rust-toolchain.json file, which, as far as I can tell, should be deleted and put into .gitignore, otherwise it'll mess with people's installations (for example, you've got your own sysroot in there).

@jackos
Copy link
Contributor Author

jackos commented Jan 17, 2022

You've committed your own rust-toolchain.json file

Thanks @diannasoreil all fixed

@x-hgg-x
Copy link
Contributor

x-hgg-x commented Jan 18, 2022

I have another solution for this (see my PR #917).

pub fn get_sysroot_src(&mut self) -> Result<(), Box<dyn Error>> {
let mut sysroot_src = home::rustup_home()?.to_string_lossy().to_string();

let output = Command::new("rustup").arg("default").output()?;
Copy link
Member

Choose a reason for hiding this comment

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

You can use rustc --print sysroot instead. This also works when you don't have rustup installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I changed this in new pr: #1026

@lnicola
Copy link
Member

lnicola commented Mar 22, 2022

#917 does seem a bit nicer.

@manyinsects
Copy link
Member

Superseded by #917.

@manyinsects
Copy link
Member

I'm considering reversing course on this, removing the solution I ended up going with (or at least un-documenting it), and putting this functionality into a new subcommand (e.g. rustlings lsp). I still don't want this to be run by default, but I do like that it's essentially a one-time fixup, and that it works regardless of editors (something I neglected in the other solution). @jackos would you be up for porting this functionality into a subcommand? Feel free to open a new pull request :)

@jackos
Copy link
Contributor Author

jackos commented Jun 16, 2022

@jackos would you be up for porting this functionality into a subcommand? Feel free to open a new pull request :)

@diannasoreil No worries, PR added here: #1026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants