Skip to content

Check that Python binary path is a file #21711

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

Closed
wants to merge 1 commit into from

Conversation

aitorciki
Copy link

Current validation checks path's existence and name (basename must start with 'python'), which results in false positives when a virtualenv directory name starts with 'python' (e.g. ~/venv/python-3.11/bin/python results in ~/venv/python-3.11 being validated as the binary path).

Current validation checks path's existence and name (basename must start
with 'python'), which results in false positives when a virtualenv
directory name starts with 'python' (e.g. ~/venv/python-3.11/bin/python
results in ~/venv/python-3.11 being validated as the binary path).
@karthiknadig karthiknadig requested a review from karrtikr August 1, 2023 16:56
@karthiknadig karthiknadig added the bug Issue identified by VS Code Team member as probable bug label Aug 1, 2023
Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

Thanks for attempting a PR, however this behavior is intentional. Using "statSync" is relatively more resource-intensive compared to "existsSync", and synchronous methods can potentially block the event loop. Which is why the check is simplistic for now, as this code is called a bunch of times and is expected to be fast.

Feel free to open a new issue if you feel the need for this based on symptoms you see, and we can possibly explore alternatives depending on whether we want to support these cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants