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

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

Merged
merged 8 commits into from
Apr 11, 2024

Conversation

snowsignal
Copy link
Contributor

@snowsignal snowsignal commented Apr 10, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

MichaReiser commented Apr 11, 2024

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:

  • ruffBin: Could be set in the workspace configuration and point to a file local in the workspace
  • sysconfig: Could point to a virtual env in the workspace
  • shutil.which: Could resolve to a file local to the workspace.
  • bundled: That should be safe

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

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.

A future improvement could be to handle the case where no python installation was detected.

We should make sure to test this change on Windows. Do you have access to a windows machine?

bundled/tool/ruff_server.py Show resolved Hide resolved
src/common/server.ts Outdated Show resolved Hide resolved
src/common/server.ts Show resolved Hide resolved
@charliermarsh
Copy link
Member

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
Copy link
Member

(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.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

LGTM though I think you wanted to bump the Ruff version too.

@snowsignal snowsignal merged commit a64505d into pre-release Apr 11, 2024
@snowsignal snowsignal deleted the jane/pre-release/user-local-executable branch April 11, 2024 19:15
snowsignal added a commit that referenced this pull request May 13, 2024
…ble (#443)

## 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).
@@ -0,0 +1,92 @@
import os
Copy link

@T-256 T-256 May 23, 2024

Choose a reason for hiding this comment

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

IF we could convert this file into typescript, then extension wouldn't need Python installation/activation on target system at all.
Currently Ruff extension needs python interpreter to start the server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, our eventual plan is to move this logic to Typescript 😄

Copy link

Choose a reason for hiding this comment

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

Created #479 to track it.

Copy link
Member

Choose a reason for hiding this comment

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

Would be an awesome change.

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.

4 participants