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

Go to Definition and Peek Definition not working on Windows 10 when using WSL #132

Closed
alzadude opened this issue Feb 21, 2019 · 8 comments
Closed

Comments

@alzadude
Copy link

@alzadude alzadude commented Feb 21, 2019

Hi,

I'm on Windows 10, and I have VSCode configured to use WSL instead of CMD, and so my nrepl gets started with WSL.

However it seems like this breaks Go to Definition (F12) and Peek Definition (Alt+F12)..

For example, when I press F12 on a locally defined function in xxx.clj, I get a message similar to:

Unable to open 'xxx.clj': File not found (file:///mnt/d/Projects/my-project/src/xxx.clj).

Please could this be made to work for WSL? Recent Windows 10 builds have a wslpath command that can convert the paths, I was thinking this might help?

Thanks!

@PEZ

This comment has been minimized.

Copy link
Collaborator

@PEZ PEZ commented Feb 21, 2019

Hello,

I am not using Windows so this one quite tricky for me to address. But some time ago @andresrgz fixed another WSL issue... Do you know how this should be fixed, @andresrgz?

@andresrgz

This comment has been minimized.

Copy link
Contributor

@andresrgz andresrgz commented Feb 22, 2019

Hello,

I am not using Windows so this one quite tricky for me to address. But some time ago @andresrgz fixed another WSL issue... Do you know how this should be fixed, @andresrgz?

I do have an idea on how to fix this. In the definition.ts file, the info.file variable contains a WSL path and should be converted into a valid Windows path before using it.

Since the repl is running within WSL, that's why the paths are being returned that way.

I may have a fix for this some time next week. 🙂 I found this library that will be useful for fixing this problem.

@PEZ

This comment has been minimized.

Copy link
Collaborator

@PEZ PEZ commented Feb 22, 2019

Cool! Please have a go at it. However, if you can find a solution that does not require a binary installed, that would be extra good.

@andresrgz

This comment has been minimized.

Copy link
Contributor

@andresrgz andresrgz commented Mar 3, 2019

@alzadude The PR to fix this has been merged. Maybe you could also help testing it before it gets published? 😄

clojure4vscode-1.3.64-WSL.zip

Just rename the .zip extension to .vsix.

@alzadude

This comment has been minimized.

Copy link
Author

@alzadude alzadude commented Mar 7, 2019

Hi @andresrgz,

I went to start testing, and I noticed that 1.3.64 must have already been "published" since I see that version number in my VSCode installation.

So I tried again, and Go to Definition and Peek Definition are now both working with WSL! 🎉

Some notes:

  1. I saw in your PR about enabling the useWSL setting. I noticed your comments about adding some steps for WSL to the main docs, good idea! Anyway, based on your steps in the PR, I enabled the setting, but I also had to restart VSCode.. I don't know if that should be required?

  2. It only seems to work for other files/namespaces in my project, for other dependencies (and also for definitions in the same file), I get "No definition found". I don't know if that's a different issue?

  3. Regarding the useWSL setting, would it be possible to avoid that by:

    Try the expected path, and if the file is not found, assume WSL, convert the path and try that? This would avoid the need to configure an extra setting?

  4. Regarding the testing procedure for users:

    Just rename the .zip extension to .vsix.

    It might be useful to have a wiki page for end-users on how to test, since they probably won't be familiar with VSCode development,. For example what are these .vsix files, how to install them, what happens if I already have Calva installed, how do I uninstall and get the stable/published Calva back again after I'm done testing etc. 🙂

Thanks for fixing this!

@PEZ

This comment has been minimized.

Copy link
Collaborator

@PEZ PEZ commented Mar 7, 2019

Yes, it is published. Can you look at 2, @andresrgz ?

Regarding the docs. I am pretty sure I have made the wiki editable, so feel free to create pages and edit stuff there. For stuff in the README, PRs are welcome.

@andresrgz

This comment has been minimized.

Copy link
Contributor

@andresrgz andresrgz commented Mar 9, 2019

Thanks for your feedback @alzadude, I will reply to each of your points.

  1. Yes, because of the way I implemented this fix a restart is required. We should probably add a notification to tell the user that VSCode needs to be restarted. I have added this step in the wiki.

  2. I have used this extension in both WSL and Linux, I can confirm that in both it doesn't behave as expected within the code of dependencies. For example, within core.clj there are some symbols in there that return No definition found, but not all of them.

  3. Yes, that could be done too. However I think it's up for discussion if we should include or remove this setting, I have seen other extensions use this approach for WSL users. In fact I based the name of this setting on VSCode's useWSL debugger option as seen here. I think having a boolean flag keeps the code very simple, and also prevents this validation from occurring in macOS and Linux. If @PEZ agrees with removing this setting I will be willing to do it. 😄

  4. I agree, it would be useful to provide some instructions on how to build and install the .vsix file for users wanting to install the latest version without waiting for it to be published.

Thank you for all of your feedback!

@cfehse

This comment has been minimized.

Copy link
Contributor

@cfehse cfehse commented Oct 15, 2019

@alzadude @PEZ @andresrgz

This should all be possible with new vscode remote-wsl extension. I tested this setup today with calva version 2.0.49 and it worked correctly. I close this issue for now.

@cfehse cfehse closed this Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.