Keep Python script limited to `sysconfig` usage #511

dhruvmanila · 2024-07-03T07:21:13Z

Summary

This PR makes progress towards #476 and #479 by making the following changes:

Update the ruff_server.py script to limit itself to just getting the Ruff binary path using sysconfig
Use the which dependency and move the logic of locating the Ruff binary from PATH to TypeScript
Add log messages similar to ruff-lsp

Test Plan

1

{
  "ruff.nativeServer": true
}

Or

{
  "ruff.nativeServer": true,
  "ruff.importStrategy": "fromEnvironment"
}

Logs:

When the ruff binary is installed in the current virtual environment:

2024-07-03 15:28:35.674 [info] Using the Ruff binary: /Users/dhruv/work/astral/ruff-vscode/.venv/bin/ruff
2024-07-03 15:28:35.675 [info] Server run command: /Users/dhruv/work/astral/ruff-vscode/.venv/bin/ruff server --preview

If there's no ruff binary in the current virtual environment:

2024-07-03 15:25:19.605 [info] Using the Ruff binary: /Users/dhruv/.local/bin/ruff
2024-07-03 15:25:19.606 [info] Server run command: /Users/dhruv/.local/bin/ruff server --preview

If the ruff binary is only available via PATH:

2024-07-03 15:32:12.172 [info] Using environment executable: /opt/homebrew/bin/ruff
2024-07-03 15:32:12.173 [info] Server run command: /opt/homebrew/bin/ruff server --preview

And, if there's no ruff binary anywhere on the system, use the bundled binary:

2024-07-03 15:29:47.948 [info] Falling back to bundled executable: /Users/dhruv/work/astral/ruff-vscode/bundled/libs/bin/ruff
2024-07-03 15:29:47.948 [info] Server run command: /Users/dhruv/work/astral/ruff-vscode/bundled/libs/bin/ruff server --preview

2

{
  "ruff.nativeServer": true,
  "ruff.importStrategy": "useBundled"
}

Logs:

2024-07-03 15:26:28.767 [info] Using bundled executable: /Users/dhruv/work/astral/ruff-vscode/bundled/libs/bin/ruff
2024-07-03 15:26:28.767 [info] Server run command: /Users/dhruv/work/astral/ruff-vscode/bundled/libs/bin/ruff server --preview

3

{
  "ruff.nativeServer": true,
  "ruff.path": ["/Users/dhruv/work/astral/ruff/target/debug/ruff"]
}

Logs:

2024-07-03 15:27:42.342 [info] Using 'path' setting: /Users/dhruv/work/astral/ruff/target/debug/ruff
2024-07-03 15:27:42.342 [info] Server run command: /Users/dhruv/work/astral/ruff/target/debug/ruff server --preview

4

{
  "ruff.nativeServer": true,
  "ruff.interpreter": [
    "/Users/dhruv/work/astral/ruff-lsp/.venv/bin/python"
  ]
}

Logs:

2024-07-03 15:35:23.578 [info] Using the Ruff binary: /Users/dhruv/work/astral/ruff-lsp/.venv/bin/ruff
2024-07-03 15:35:23.578 [info] Server run command: /Users/dhruv/work/astral/ruff-lsp/.venv/bin/ruff server --preview

5

What if the script failed? I modified the script path in TypeScript code and we get the following logs:

2024-07-03 15:42:41.035 [error] Error while trying to find the Ruff binary: Error: Command failed: /Users/dhruv/work/astral/ruff-lsp/.venv/bin/python /Users/dhruv/work/astral/ruff-vscode/bundled/tool/find_ruff_binary_pat.py
/Users/dhruv/work/astral/ruff-lsp/.venv/bin/python: can't open file '/Users/dhruv/work/astral/ruff-vscode/bundled/tool/find_ruff_binary_pat.py': [Errno 2] No such file or directory

2024-07-03 15:42:41.037 [info] Using environment executable: /Users/dhruv/.local/bin/ruff
2024-07-03 15:42:41.040 [info] Server run command: /Users/dhruv/.local/bin/ruff server --preview

T-256

LGTM, Thank you!

src/common/constants.ts

src/common/server.ts

dhruvmanila · 2024-07-03T10:15:59Z

src/common/server.ts

+  try {
+    const stdout = await executeCommand(
+      `${settings.interpreter[0]} ${FIND_RUFF_BINARY_SCRIPT_PATH}`,
+    );
+    ruffBinaryPath = stdout.trim();
+  } catch (err) {
+    traceError(`Error while trying to find the Ruff binary: ${err}`);
+  }


I'm a bit torn on whether we should notify the user or not. The script itself doesn't raise any error so the failure should come from any external means. If it does, we'll log the failure and continue ahead by checking the PATH variable and falling back to using the bundled executable.

It seems fine to notify if the error seems rare and indicative of an unexpected situation.

Yeah, I think that makes sense. I've added a "Show Logs" action button which opens the Ruff logs directly.

MichaReiser · 2024-07-03T13:58:34Z

src/common/server.ts

+    traceError(`Error while trying to find the Ruff binary: ${err}`);
+  }
+
+  if (ruffBinaryPath && ruffBinaryPath.length > 0) {


Suggested change

if (ruffBinaryPath && ruffBinaryPath.length > 0) {

if (ruffBinaryPath?.length > 0) {

Hmm, it's giving me:

typescript: 'ruffBinaryPath.length' is possibly 'undefined'. [18048]

I'm going to leave it as is for now

MichaReiser · 2024-07-03T14:00:50Z

bundled/tool/ruff_server.py

This is probably a dumb question and I think I saw a related PR today.
Are we now checking the ruff version in the typescript code for both the native and legacy LSP?

We check whether the Ruff executable we found supports the native server (ruff server) by looking at the version. This is removed here from the Python script and added in TypeScript in #512. I'm not sure what you mean by "version check in legacy LSP"?

charliermarsh

Where / when do we validate the Ruff version?

dhruvmanila · 2024-07-04T03:12:17Z

Where / when do we validate the Ruff version?

I'm assuming you mean by the validation for the native server which is in #512 in TypeScript code.

## Summary This PR adds back the message to suggest the user that the current Ruff binary doesn't support the native server. This was removed from the Python script in #511. fixes: #476 ## Test Plan Install an older Ruff version to test this out using `pipx install --force "ruff==0.3.3"`. <img width="1432" alt="Screenshot 2024-07-03 at 14 50 41" src="https://github.com/astral-sh/ruff-vscode/assets/67177269/267e2bab-e050-4cdf-98ff-fb140b7d63ee"> And, we've another log message which displays the Ruff version similar to `ruff-lsp`: ``` 2024-07-03 16:00:03.089 [info] Using the Ruff binary: /Users/dhruv/work/astral/ruff-lsp/.venv/bin/ruff 2024-07-03 16:00:03.094 [info] Found Ruff 0.4.10 at /Users/dhruv/work/astral/ruff-lsp/.venv/bin/ruff 2024-07-03 16:00:03.095 [info] Server run command: /Users/dhruv/work/astral/ruff-lsp/.venv/bin/ruff server --preview ```

dhruvmanila mentioned this pull request Jul 3, 2024

Try all path, consider useBundled import strategy #509

Merged

T-256 approved these changes Jul 3, 2024

View reviewed changes

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

src/common/server.ts Show resolved Hide resolved

dhruvmanila force-pushed the dhruv/reduce-python branch 2 times, most recently from 9173f37 to 0043fcc Compare July 3, 2024 10:10

dhruvmanila commented Jul 3, 2024

View reviewed changes

dhruvmanila marked this pull request as ready for review July 3, 2024 10:16

MichaReiser requested a review from charliermarsh July 3, 2024 10:18

dhruvmanila mentioned this pull request Jul 3, 2024

Better error message for unsupported Ruff version #512

Merged

MichaReiser approved these changes Jul 3, 2024

View reviewed changes

charliermarsh approved these changes Jul 3, 2024

View reviewed changes

Base automatically changed from dhruv/bundled to main July 4, 2024 03:05

Keep Python script limited to sysconfig usage

90d4358

dhruvmanila force-pushed the dhruv/reduce-python branch from 0043fcc to 90d4358 Compare July 4, 2024 03:06

Docs, display error message

0c8e478

dhruvmanila merged commit fbb0467 into main Jul 4, 2024
6 checks passed

dhruvmanila deleted the dhruv/reduce-python branch July 4, 2024 03:49

dhruvmanila mentioned this pull request Jul 17, 2024

Improve logging in VSCode extension #526

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep Python script limited to `sysconfig` usage #511

Keep Python script limited to `sysconfig` usage #511

dhruvmanila commented Jul 3, 2024 •

edited

Loading

T-256 left a comment

dhruvmanila Jul 3, 2024

zanieb Jul 3, 2024

dhruvmanila Jul 4, 2024

MichaReiser Jul 3, 2024

dhruvmanila Jul 4, 2024

MichaReiser Jul 3, 2024

dhruvmanila Jul 4, 2024

charliermarsh left a comment

dhruvmanila commented Jul 4, 2024

	if (ruffBinaryPath && ruffBinaryPath.length > 0) {
	if (ruffBinaryPath?.length > 0) {

Keep Python script limited to sysconfig usage #511

Keep Python script limited to sysconfig usage #511

Conversation

dhruvmanila commented Jul 3, 2024 • edited Loading

Summary

Test Plan

1

2

3

4

5

T-256 left a comment

Choose a reason for hiding this comment

dhruvmanila Jul 3, 2024

Choose a reason for hiding this comment

zanieb Jul 3, 2024

Choose a reason for hiding this comment

dhruvmanila Jul 4, 2024

Choose a reason for hiding this comment

MichaReiser Jul 3, 2024

Choose a reason for hiding this comment

dhruvmanila Jul 4, 2024

Choose a reason for hiding this comment

MichaReiser Jul 3, 2024

Choose a reason for hiding this comment

dhruvmanila Jul 4, 2024

Choose a reason for hiding this comment

charliermarsh left a comment

Choose a reason for hiding this comment

dhruvmanila commented Jul 4, 2024

Keep Python script limited to `sysconfig` usage #511

Keep Python script limited to `sysconfig` usage #511

dhruvmanila commented Jul 3, 2024 •

edited

Loading