Skip invalid executables #585

T-256 · 2024-08-16T17:59:41Z

Summary

This PR introduces skip by default on executables that is unable to parse version from <binary> version.
Also, defined minimum-supported-ruff-version that causes to skip outdated versions if hit them in executable discovery.

Done partially #426 and #323 in conditions of native server chosen.

Test Plan

didn't run any tests. I think it should work.

MichaReiser · 2024-08-22T15:30:01Z

Thanks for working on this. @dhruvmanila this overall seems reasonable to me. What do you think?

src/common/version.ts

src/common/server.ts

dhruvmanila · 2024-08-23T03:36:38Z

src/common/server.ts

+      message += `(Reqiuired at least ${versionToString(
+        MINIMUM_SUPPORTED_EXECUTABLE_VERSION,
+      )}, but found ${versionToString(ruffVersion)} instead)`;
+      traceError(message);


I think we should also display this message to the users using vscode.window.showErrorMessage. What do you think?

during findRuffBinaryPath it may skip several binaries, I think it'd be annoying to send all of them as user facing warnings.

This message shouldn't be required because we already check that later on. Refer to my other comment.

🤷‍♂️

dhruvmanila · 2024-08-23T03:37:37Z

src/common/server.ts

+  } catch (ex) {
+    traceInfo(`Skip invalid executable from ${strategy}: ${executable}`);


Should we include the exception message as well? Are we able to capture the one in #426 ?

Yes, makes sense. Since ex maybe multi line error, what is correct format to put it in there? I don't think put it in parantheses be a good style.

traceInfo(`Skip invalid executable from ${strategy}: ${executable}:\n${ex}`);

is this correct to you?

I think this should be fine but I'd need to play around with it to check how it renders in the output window. I can do that when I'm testing before merging the PR.

dhruvmanila · 2024-08-23T03:39:02Z

didn't run any tests. I think it should work.

I can test it out before merging.

Co-authored-by: Dhruv Manilawala <[email protected]>

T-256 · 2024-08-23T15:15:07Z

I can test it out before merging.

Thank you!

dhruvmanila · 2024-08-23T15:20:59Z

@T-256 feel free to ping me once you've made all the changes from your side. I can then own it to test and merge the PR.

T-256 · 2024-08-23T15:41:22Z

I'm done. Ping me to review when you applied your changes, Thank you.

dhruvmanila · 2024-08-23T16:12:02Z

(It's a bit late here, so will look at it tomorrow.)

dhruvmanila · 2024-08-26T06:29:59Z

Unfortunately, I'm still getting the errors from pyenv but it's now occurring when the server is starting and not while it's trying to fetch the version. I think that's occurring because the version fetching isn't occurring in the virtual environment but from outside of it and so pyenv doesn't raise any error.

Complete logs

2024-08-26 11:59:31.953 [info] Name: Ruff
2024-08-26 11:59:31.953 [info] Module: ruff
2024-08-26 11:59:31.953 [info] Python extension loading
2024-08-26 11:59:31.953 [info] Waiting for interpreter from python extension.
2024-08-26 11:59:31.953 [info] Python extension loaded
2024-08-26 11:59:31.953 [info] Using interpreter: /Users/dhruv/.pyenv/versions/3.11.7/envs/playground/bin/python
2024-08-26 11:59:31.970 [info] Checking executable from environment executable: /Users/dhruv/.pyenv/shims/ruff
2024-08-26 11:59:32.396 [info] Using environment executable: /Users/dhruv/.pyenv/shims/ruff
2024-08-26 11:59:32.588 [info] Found Ruff 0.6.2 at /Users/dhruv/.pyenv/shims/ruff
2024-08-26 11:59:32.588 [info] Server run command: /Users/dhruv/.pyenv/shims/ruff server
2024-08-26 11:59:32.589 [info] Server: Start requested.
2024-08-26 11:59:32.591 [info] [Trace - 11:59:32 AM] Sending request 'initialize - (0)'.
2024-08-26 11:59:32.665 [info] pyenv: ruff: command not found

2024-08-26 11:59:32.796 [info] 
The `ruff' command exists in these Python versions:

2024-08-26 11:59:32.797 [info]   3.12.4

2024-08-26 11:59:32.797 [info] 

2024-08-26 11:59:32.797 [info] Note: See 'pyenv help global' for tips on allowing both
      python2 and python3 to be found.

2024-08-26 11:59:32.798 [info] [Error - 11:59:32 AM] Server initialization failed.
2024-08-26 11:59:32.798 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:32.798 [info] [Info  - 11:59:32 AM] Connection to server got closed. Server will restart.
2024-08-26 11:59:32.798 [info] true
2024-08-26 11:59:32.798 [info] [Error - 11:59:32 AM] Ruff client: couldn't create connection to server.
2024-08-26 11:59:32.798 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:32.799 [info] [Trace - 11:59:32 AM] Sending request 'initialize - (0)'.
2024-08-26 11:59:32.868 [info] pyenv: ruff: command not found

2024-08-26 11:59:33.018 [info] 
The `ruff' command exists in these Python versions:

2024-08-26 11:59:33.020 [info]   3.12.4

2024-08-26 11:59:33.020 [info] 
Note: See 'pyenv help global' for tips on allowing both
      python2 and python3 to be found.

2024-08-26 11:59:33.020 [info] [Error - 11:59:33 AM] Server initialization failed.
2024-08-26 11:59:33.021 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:33.021 [info] [Info  - 11:59:33 AM] Connection to server got closed. Server will restart.
2024-08-26 11:59:33.021 [info] true
2024-08-26 11:59:33.021 [info] [Error - 11:59:33 AM] Ruff client: couldn't create connection to server.
2024-08-26 11:59:33.021 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:33.021 [error] Server: Start failed: Error: Pending response rejected since connection got disposed
2024-08-26 11:59:33.021 [info] [Trace - 11:59:33 AM] Sending request 'initialize - (0)'.
2024-08-26 11:59:33.112 [info] pyenv: ruff: command not found

2024-08-26 11:59:33.282 [info] 
The `ruff' command exists in these Python versions:

2024-08-26 11:59:33.284 [info]   3.12.4

2024-08-26 11:59:33.284 [info] 
Note: See 'pyenv help global' for tips on allowing both
      python2 and python3 to be found.

2024-08-26 11:59:33.285 [info] [Error - 11:59:33 AM] Server initialization failed.
2024-08-26 11:59:33.285 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:33.285 [info] [Info  - 11:59:33 AM] Connection to server got closed. Server will restart.
2024-08-26 11:59:33.285 [info] true
2024-08-26 11:59:33.285 [info] [Error - 11:59:33 AM] Ruff client: couldn't create connection to server.
2024-08-26 11:59:33.285 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:33.285 [info] [Error - 11:59:33 AM] Restarting server failed
2024-08-26 11:59:33.285 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:33.286 [info] [Trace - 11:59:33 AM] Sending request 'initialize - (0)'.
2024-08-26 11:59:33.371 [info] pyenv: ruff: command not found

2024-08-26 11:59:33.545 [info] 
The `ruff' command exists in these Python versions:

2024-08-26 11:59:33.548 [info]   3.12.4

2024-08-26 11:59:33.548 [info] 
Note: See 'pyenv help global' for tips on allowing both
      python2 and python3 to be found.

2024-08-26 11:59:33.548 [info] [Error - 11:59:33 AM] Server initialization failed.
2024-08-26 11:59:33.549 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:33.549 [info] [Info  - 11:59:33 AM] Connection to server got closed. Server will restart.
2024-08-26 11:59:33.549 [info] true
2024-08-26 11:59:33.549 [info] [Error - 11:59:33 AM] Ruff client: couldn't create connection to server.
2024-08-26 11:59:33.549 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:33.549 [info] [Error - 11:59:33 AM] Restarting server failed
2024-08-26 11:59:33.549 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:33.549 [info] [Trace - 11:59:33 AM] Sending request 'initialize - (0)'.
2024-08-26 11:59:33.640 [info] pyenv: ruff: command not found

2024-08-26 11:59:33.821 [info] 
The `ruff' command exists in these Python versions:
  3.12.4

Note: See 'pyenv help global' for tips on allowing both
      python2 and python3 to be found.

2024-08-26 11:59:33.822 [info] [Error - 11:59:33 AM] Server initialization failed.
2024-08-26 11:59:33.822 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:33.822 [info] [Error - 11:59:33 AM] The Ruff server crashed 5 times in the last 3 minutes. The server will not be restarted. See the output for more information.
2024-08-26 11:59:33.822 [info] [Error - 11:59:33 AM] Ruff client: couldn't create connection to server.
2024-08-26 11:59:33.822 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097 
2024-08-26 11:59:33.822 [info] [Error - 11:59:33 AM] Restarting server failed
2024-08-26 11:59:33.822 [info]   Message: Pending response rejected since connection got disposed
  Code: -32097

T-256 · 2024-08-26T12:13:10Z

2024-08-26 11:59:32.588 [info] Found Ruff 0.6.2 at /Users/dhruv/.pyenv/shims/ruff

We use ruff --version to validate the found executable is correct. at here I think --version handled by pyenv, though it shows correct version output, but it fails to run ruff server command. should we add extra validation step that not handled by other tools?
possiblities:

internal hidden arg/option.
parse ruff version instead of ruff --version.
execute ruff help and validate from head/exit code.

dhruvmanila · 2024-09-02T06:18:28Z

at here I think --version handled by pyenv, though it shows correct version output, but it fails to run ruff server command.

I'm not sure I follow what you're saying here.

Overall, I'm a bit hesitant to merge this because it doesn't really solve the pyenv shims issue. I might need to spend some time playing around with a couple of solutions to understand what we can do about it.

dhruvmanila · 2025-01-16T04:54:18Z

I looked into this a bit more and am unable to figure out the root cause of why it's failing to start the server. Are you planning to look into the failure?

T-256 · 2025-01-16T15:09:59Z

by looking through this part of your logs:

2024-08-26 11:59:31.953 [info] Using interpreter: /Users/dhruv/.pyenv/versions/3.11.7/envs/playground/bin/python
2024-08-26 11:59:31.970 [info] Checking executable from environment executable: /Users/dhruv/.pyenv/shims/ruff
2024-08-26 11:59:32.396 [info] Using environment executable: /Users/dhruv/.pyenv/shims/ruff
2024-08-26 11:59:32.588 [info] Found Ruff 0.6.2 at /Users/dhruv/.pyenv/shims/ruff
2024-08-26 11:59:32.588 [info] Server run command: /Users/dhruv/.pyenv/shims/ruff server
2024-08-26 11:59:32.589 [info] Server: Start requested.
2024-08-26 11:59:32.591 [info] [Trace - 11:59:32 AM] Sending request 'initialize - (0)'.
2024-08-26 11:59:32.665 [info] pyenv: ruff: command not found

2024-08-26 11:59:32.796 [info] 
The `ruff' command exists in these Python versions:

2024-08-26 11:59:32.797 [info]   3.12.4

2024-08-26 11:59:32.797 [info] 

2024-08-26 11:59:32.797 [info] Note: See 'pyenv help global' for tips on allowing both
      python2 and python3 to be found.

2024-08-26 11:59:32.798 [info] [Error - 11:59:32 AM] Server initialization failed.

Selected pyenv's Python 3.11 as interpreter.
ruff executable is available as pyenv shim.
ruff shim installed on pyenv's Python 3.12.
ruff --version didn't fail as expected at there because I think pyenv handles <pkg> --version from package metadata(?).

Can you test with 3e43982?

dhruvmanila · 2025-01-17T03:21:22Z

If you want a way to reproduce this, then I did the following:

Install pyenv and pyenv-virtualenv
Initialize both plugins for the current shell: eval "$(pyenv init -)" and eval "$(pyenv virtualenv-init -)"
Install any Python version using pyenv: pyenv install 3.13.1
Set the global Python version to this: pyenv global 3.13.1
Install ruff globally: pip install ruff
Create and cd into a temporary directory
Create a virtual environment in it: pyenv virtualenv test (this should now be automatically activated)
Pin the local directory to this virtual environment: pyenv local test
Make sure there's only one ruff executable available which is the pyenv shim
```
❯ which -a ruff
/Users/dhruv/.pyenv/shims/ruff
```

Run ruff --version

❯ ruff --version
pyenv: ruff: command not found

The `ruff' command exists in these Python versions:
  3.13.1

Note: See 'pyenv help global' for tips on allowing both
      python2 and python3 to be found.

Compiling this branch (npm run compile) and open the development version of VS Code in this directory: code --extensionDevelopmentPath=/path/to/ruff-vscode /tmp/ruff-test
Make sure that the extension is using ruff from pyenv shim

T-256 · 2025-01-17T10:08:10Z

@dhruvmanila I understand your issue at here, you want to support pyenv, but it's out scope of this PR.

12. Make sure that the extension is using ruff from pyenv shim

Since we are not yet support pyenv, I think it makes sense to skip them for now.
We can try fully support it in separate PR.

dhruvmanila · 2025-01-21T04:57:23Z

@dhruvmanila I understand your issue at here, you want to support pyenv, but it's out scope of this PR.

Make sure that the extension is using ruff from pyenv shim

Since we are not yet support pyenv, I think it makes sense to skip them for now. We can try fully support it in separate PR.

Yeah, that's fine, thanks. If that's not the case, then can you clarify what this PR is trying to solve? Can you provide a test scenario where this PR helps in comparison to what's on main?

## Summary I noticed this in #585 that we would not be able to look at the error message when the server crashes because the output window got disposed. This is because the disposable is tied up with stopping the server. This PR updates that to include the disposable to the extension context so that it gets disposed only when the extension is deactivated. ## Test Plan Run the scenario from #585 to make sure that the "Ruff Language Server" window is not disposed in case the server crashed.

T-256 · 2025-01-21T13:53:58Z

it solves below scenarios when native server has been chosen:

on main #426 fails to select ruff binary (and start server) without user-facing notification, this PR continues discovering the executable (with show skipping warnings in log) and finally fallbacks to bundled one.

on main #323 selects the ruff binary but fails to start server because the minimum required version not satisfied. this PR continues discovering the other executable that satisfies minimum required version.

dhruvmanila · 2025-01-22T06:02:44Z

Thanks for providing them but now I'm even more confused, please bear with me 😅

on main #426 fails to select ruff binary (and start server) without user-facing notification, this PR continues discovering the executable (with show skipping warnings in log) and finally fallbacks to bundled one.

My main confusion is that you mentioned not to support pyenv here:

Since we are not yet support pyenv, I think it makes sense to skip them for now.

So, this is not really true as is evident by my testing using #585 (comment). Do you have a scenario or a recording which showcases that it's working as you've intended? Do you mind sharing that?

T-256 added 2 commits August 16, 2024 21:26

Skip invalid executables

d235400

mis

3363033

define minimum supported Ruff version

81f9ea2

dhruvmanila reviewed Aug 23, 2024

View reviewed changes

src/common/version.ts Outdated Show resolved Hide resolved

dhruvmanila reviewed Aug 23, 2024

View reviewed changes

src/common/server.ts Outdated Show resolved Hide resolved

dhruvmanila reviewed Aug 23, 2024

View reviewed changes

Update src/common/server.ts

e928ffb

Co-authored-by: Dhruv Manilawala <[email protected]>

dhruvmanila self-requested a review August 23, 2024 14:27

apply review

307cbf2

T-256 mentioned this pull request Aug 29, 2024

VSCode finding wrong versions of Ruff? #595

Closed

Merge branch 'astral-sh:main' into skip-invalid

a072eee

T-256 added 3 commits September 30, 2024 00:33

Merge branch 'astral-sh:main' into skip-invalid

467af1e

Merge branch 'main' into skip-invalid

25bcdd4

logger

bb26de8

T-256 added 5 commits January 16, 2025 17:33

Merge branch 'astral-sh:main' into skip-invalid

5b31c94

Update ci.yaml

bb58750

fmt

9b78802

prettier

09cb3b5

revert ci.yaml

3e43982

dhruvmanila mentioned this pull request Jan 21, 2025

Use extension context for output window disposable #671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip invalid executables #585

Skip invalid executables #585

T-256 commented Aug 16, 2024 •

edited

Loading

MichaReiser commented Aug 22, 2024

dhruvmanila Aug 23, 2024

T-256 Aug 23, 2024

dhruvmanila Aug 23, 2024

T-256 Aug 23, 2024

dhruvmanila Aug 23, 2024

T-256 Aug 23, 2024

dhruvmanila Aug 23, 2024

dhruvmanila commented Aug 23, 2024

T-256 commented Aug 23, 2024

dhruvmanila commented Aug 23, 2024

T-256 commented Aug 23, 2024

dhruvmanila commented Aug 23, 2024

dhruvmanila commented Aug 26, 2024

T-256 commented Aug 26, 2024

dhruvmanila commented Sep 2, 2024

dhruvmanila commented Jan 16, 2025

T-256 commented Jan 16, 2025

dhruvmanila commented Jan 17, 2025 •

edited

Loading

T-256 commented Jan 17, 2025

dhruvmanila commented Jan 21, 2025

T-256 commented Jan 21, 2025

dhruvmanila commented Jan 22, 2025

		} catch (ex) {
		traceInfo(`Skip invalid executable from ${strategy}: ${executable}`);

Skip invalid executables #585

Are you sure you want to change the base?

Skip invalid executables #585

Conversation

T-256 commented Aug 16, 2024 • edited Loading

Summary

Test Plan

MichaReiser commented Aug 22, 2024

dhruvmanila Aug 23, 2024

Choose a reason for hiding this comment

T-256 Aug 23, 2024

Choose a reason for hiding this comment

dhruvmanila Aug 23, 2024

Choose a reason for hiding this comment

T-256 Aug 23, 2024

Choose a reason for hiding this comment

dhruvmanila Aug 23, 2024

Choose a reason for hiding this comment

T-256 Aug 23, 2024

Choose a reason for hiding this comment

dhruvmanila Aug 23, 2024

Choose a reason for hiding this comment

dhruvmanila commented Aug 23, 2024

T-256 commented Aug 23, 2024

dhruvmanila commented Aug 23, 2024

T-256 commented Aug 23, 2024

dhruvmanila commented Aug 23, 2024

dhruvmanila commented Aug 26, 2024

T-256 commented Aug 26, 2024

dhruvmanila commented Sep 2, 2024

dhruvmanila commented Jan 16, 2025

T-256 commented Jan 16, 2025

dhruvmanila commented Jan 17, 2025 • edited Loading

T-256 commented Jan 17, 2025

dhruvmanila commented Jan 21, 2025

T-256 commented Jan 21, 2025

dhruvmanila commented Jan 22, 2025

T-256 commented Aug 16, 2024 •

edited

Loading

dhruvmanila commented Jan 17, 2025 •

edited

Loading