-
Notifications
You must be signed in to change notification settings - Fork 732
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
Allow discovery of interpereters in virtual environments activated by a shim #4032
base: main
Are you sure you want to change the base?
Conversation
Considering if this is the correct approach still. We should also test with the |
I find this implementation awkward as I only want this to be used if it's a virtual environment interpreter but we can't easily enforce that in |
6741fc8
to
e4554ab
Compare
source, | ||
InterpreterSource::ProvidedPath | InterpreterSource::ParentInterpreter | ||
) { | ||
match result { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes to the match statement, just indented
e4554ab
to
2bb4bf4
Compare
I'd appreciate input on a better approach here. |
Just confirming: still relevant to review? I know there was some back-and-forth on the issue but didn't read it deeply. |
@charliermarsh yeah this is still necessary if we want to support detecting virtual environments that are only activated by an executable shim. I'm not in love with the implementation though. |
.chain( | ||
sources.contains(InterpreterSource::ShimEnvironment).then(|| | ||
python_executables_from_search_path(version, implementation).next() | ||
.map(|path| Ok((InterpreterSource::ShimEnvironment, path))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we know this is a shim?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't. We're going to naively say the first executable in the path might be a shim then verify it during the interpreter query portion.
.filter(move |result| match result { | ||
.filter(move |result| { | ||
// Always skip shim environments that are not virtual environments; this filter is deferred | ||
// from `python_executables` to avoid querying the interpreter there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a shim environment that isn't a virtual environment? What would appear here to fail this filter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're just taking the first executable and calling it a "shim environment" in python_executables
this filtering is where we check if the executable is actually a shim which invokes inside a virtual environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this and the other review comment are my concern with this implementation, it's misleading but we do it to avoid querying the interpreter during python_executables
)
Closes #4009
Closes #2109