astral-sh / ruff-vscode

A Visual Studio Code extension with support for the Ruff linter.
Other
946 stars 45 forks source link

Experimental `ruff server` now uses local `ruff` binaries when available #443

Closed snowsignal closed 2 months ago

snowsignal commented 2 months ago

Summary

When experimentalServer is enabled, the Ruff binary path will be determined in the following order:

  1. ruff.path - if this is set, we take the first path in the list and use that as the executable path.
  2. sysconfig.get_path("scripts") / RUFF_EXE - RUFF_EXE is "ruff.exe" on windows and "ruff" otherwise.
  3. shutil.which("ruff")
  4. The bundled binary path, ./bundled/libs/bin/ruff, is used if none of the prior options are available.

Before running the binary, we also check to make sure that it's running a compatible version. At the moment, any version after or including 0.3.3 is supported (except for 0.3.4, since that release had a major bug in the server that broke editor integration). Since 0.3.3 is a rather bare-bones implementation, this minimum supported version will increase as the extension stabilizes.

Test Plan

  1. Run ruff --version in your shell. If it's not ruff 0.3.5 or later, install/upgrade ruff and run ruff --version again to confirm that the ruff executable in your PATH is now >=0.3.5.
  2. Begin debugging the extension. You should see the following log somewhere in the output: Configuration file watcher successfully registered. This should be a confirmation that you're running ruff >=0.3.5 instead of the embedded ruff 0.3.3.
  3. Add an entry to ruff.path in the extension settings, and pass in a ruff binary not in your PATH. A good way to check that you are in fact using this unique binary is by passing in the path to a local debug build after adding an additional log statement in a function like Server::new. If this log statement appears in the output, you know that you're using the custom executable provided in ruff.path.
  4. Now, uninstall the ruff executable that was in your PATH. Confirm that no executable exists in your path by running ruff --version in your shell - you should get your shell's variant of the command not found: ruff error.
  5. At this point, the extension should still be using the custom executable you added to ruff.path. Now, remove that path so that ruff.path is an empty list. The extension should now be using the bundled v0.3.3 binary (a quick way to check this is making sure no Configuration file watcher successfully registered message appears, though you could also confirm this by seeing if the diagnostics show up highlighted in red instead of yellow).
charliermarsh commented 2 months ago

It looks like the only things this is missing vis-a-vis the existing LSP (not that they're required for the Alpha, or even at all -- just confirming my understanding) are:

  1. importStrategy -- allow the user to prioritize the bundled version.
  2. interpreter -- allow the user to specify an alternative Python interpreter.
snowsignal commented 2 months ago

interpreter -- allow the user to specify an alternative Python interpreter.

The interpreter setting should be supported (we read from it here), but we only read from the first path in the list, if it exists.

You're right that we aren't using importStrategy yet. Another limitation with this implementation is that it doesn't use subsequent entries in ruff.path past the first one as fallbacks.

MichaReiser commented 2 months ago

This is an addition to the current LSP but something that I wanted to change for a very long time.

I think we should always use the bundled version if the workspace is not a trusted workspace to avoid executing "untrusted" code:

charliermarsh commented 2 months ago

I think it would be good to handle the "untrusted" workspace case before the release but we can do it as a follow up pr.

I would comfortably say this isn't required for the release, since it's not something we do today in the current LSP, and the focus on the milestone is to achieve parity with the existing LSP.

(It's also not something that's done in any of the official Microsoft Python template-based extensions. Those don't use which, but they do support a direct path, discovery from the interpreter, etc.)

MichaReiser commented 2 months ago

(It's also not something that's done in any of the official Microsoft Python template-based extensions. Those don't use which, but they do support a direct path, discovery from the interpreter, etc.)

That's surprising (and scary). I think we should do this but you convinced me that we should do this after the release.