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

fix ale_python_auto_virtualenv to correctly set virtualenv env vars #4885

Merged
merged 4 commits into from
Dec 29, 2024

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Dec 28, 2024

First, thank you for ALE!

According to the documentation, ale_python_auto_virtualenv should automatically set environment variables for commands, but previously the variables were not set completely or correctly.

Before:
PATH variable was expanded to include /path/to/venv
After:
PATH variable is expanded to include /path/to/venv/bin
VIRTUAL_ENV variable is set to path/to/venv

This mimics exactly what the activate scripts do, and allows the configuration knob to work as expected.

For example, after this change, jedi-language-server can be installed globally (instead of inside every venv), and it will "just work" (e.g. find references to dependencies in the venv) when editing a file in a project that uses a venv, because the correct variables are set.

According to the documentation, `ale_python_auto_virtualenv` should automatically set environment variables for commands, but previously the variables were not set completely or correctly.

Before:
  `PATH` variable was expanded to include `/path/to/venv`
After:
  `PATH` variable is expanded to include `/path/to/venv/bin`
  `VIRTUAL_ENV` variable is set to `path/to/venv`

This mimics exactly what the `activate` scripts do, and allows the configuration knob to work as expected.

For example, after this change, `jedi-language-server` can be installed globally (instead of inside every venv), and it will "just work" (e.g. find references to dependencies in the venv) when editing a file in a project that uses a venv, because the correct variables are set.
@rmuir
Copy link
Contributor Author

rmuir commented Dec 28, 2024

Example ALEInfo output with this change:

Before:

(started) ['/usr/bin/bash', '-c', 'PATH=''/home/rmuir/workspace/beer/api/.venv''":$PATH" ''jedi-language-server''']

as you can see, PATH is not set correctly as it doesn't include bin directory. the VIRTUAL_ENV variable is missing.

After:

(started) ['/bin/bash', '-c', 'PATH=''/home/rmuir/workspace/beer/api/.venv/bin''":$PATH" VIRTUAL_ENV=''/home/rmuir/workspace/beer/api/.venv'' ''jedi-language-server''']

@rmuir rmuir marked this pull request as draft December 28, 2024 02:05
@rmuir
Copy link
Contributor Author

rmuir commented Dec 28, 2024

Converted to draft until I fix the test to expect the correct data.

@rmuir rmuir marked this pull request as ready for review December 28, 2024 03:15
@rmuir
Copy link
Contributor Author

rmuir commented Dec 28, 2024

I'll leave it as a separate discussion if the default value for this variable should stay as 0, as I don't want to complicate this PR.

IMO, when working with a python virtualenv, you should always set these variables, same as if the user did activate.

You can sometimes get away with just $env/bin/linter, but not always. So it is best to set the PATH and VIRTUAL_ENV to ensure linter functions correctly. A good example is ansible-lint in a virtualenv, it invokes ansible-playbook --syntax-check which will not work with the current approach, as it won't be in the PATH.

Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks.

@hsanson hsanson merged commit 5b2e69a into dense-analysis:master Dec 29, 2024
7 checks passed
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.

2 participants