Skip to content

Allow discovery of interpereters in virtual environments activated by a shim #4032

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.

Sign up for GitHub

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
zanieb wants to merge 1 commit into from
Closed

Allow discovery of interpereters in virtual environments activated by a shim #4032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 65 additions & 41 deletions crates/uv-interpreter/src/discovery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ pub enum InterpreterSource {
ActiveEnvironment,
/// A conda environment was active e.g. via `CONDA_PREFIX`
CondaPrefix,
/// A shim `python` executable that invokes in a PEP 405 compliant virtual environment
ShimEnvironment,
/// An environment was discovered e.g. via `.venv`
DiscoveredEnvironment,
/// An executable was found in the search path i.e. `PATH`
Expand Down Expand Up @@ -175,6 +177,7 @@ pub enum Error {
///
/// - The spawning interpreter
/// - The active environment
/// - A shim executable that invokes a virtual environment
/// - A discovered environment (e.g. `.venv`)
/// - Installed managed toolchains
/// - The search path (i.e. PATH)
Expand All @@ -183,6 +186,8 @@ pub enum Error {
/// Each location is only queried if the previous location is exhausted.
/// Locations may be omitted using `sources`, sources that are not selected will not be queried.
///
/// Executables from `InterpreterSource::ShimExecutable` **must** be checked to ensure they are virtual environments.
///
/// If a [`VersionRequest`] is provided, we will skip executables that we know do not satisfy the request
/// and (as discussed in [`python_executables_from_search_path`]) additional version specific executables may
/// be included. However, the caller MUST query the returned executables to ensure they satisfy the request;
Expand Down Expand Up @@ -219,7 +224,15 @@ fn python_executables<'a>(
.map(|path| Ok((InterpreterSource::CondaPrefix, path)))
).into_iter().flatten()
)
// (4) A discovered environment
// (4) The first `python` executable on the `PATH`
// N.B. We must filter this interpreter later to ensure it is a virtual environment
.chain(
sources.contains(InterpreterSource::ShimEnvironment).then(||
python_executables_from_search_path(version, implementation).next()
.map(|path| Ok((InterpreterSource::ShimEnvironment, path)))
Copy link
Member

@charliermarsh charliermarsh Jun 8, 2024

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?

Copy link
Member Author

@zanieb zanieb Jun 8, 2024

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.

).into_iter().flatten()
)
// (5) A discovered environment
.chain(
sources.contains(InterpreterSource::DiscoveredEnvironment).then(||
std::iter::once(
Expand All @@ -234,7 +247,7 @@ fn python_executables<'a>(
).flatten_ok()
).into_iter().flatten()
)
// (5) Managed toolchains
// (6) Managed toolchains
.chain(
sources.contains(InterpreterSource::ManagedToolchain).then(move ||
std::iter::once(
Expand All @@ -255,14 +268,14 @@ fn python_executables<'a>(
).flatten_ok()
).into_iter().flatten()
)
// (6) The search path
// (7) The search path
.chain(
sources.contains(InterpreterSource::SearchPath).then(move ||
python_executables_from_search_path(version, implementation)
.map(|path| Ok((InterpreterSource::SearchPath, path))),
).into_iter().flatten()
)
// (7) The `py` launcher (windows only)
// (8) The `py` launcher (windows only)
// TODO(konstin): Implement <https://peps.python.org/pep-0514/> to read python installations from the registry instead.
.chain(
(sources.contains(InterpreterSource::PyLauncher) && cfg!(windows)).then(||
Expand Down Expand Up @@ -378,52 +391,61 @@ fn python_interpreters<'a>(
.inspect_err(|err| debug!("{err}")),
Err(err) => Err(err),
})
.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
Copy link
Member

@charliermarsh charliermarsh Jun 8, 2024

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?

Copy link
Member Author

@zanieb zanieb Jun 8, 2024

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.

Copy link
Member Author

@zanieb zanieb Jun 8, 2024

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)

if let Ok((InterpreterSource::ShimEnvironment, interpreter)) = result {
if !interpreter.is_virtualenv() {
return false;
}
};
// Filter the returned interpreters to conform to the system request
Ok((source, interpreter)) => match (
system,
// Conda environments are not conformant virtual environments but we should not treat them as system interpreters
interpreter.is_virtualenv() || matches!(source, InterpreterSource::CondaPrefix),
) {
(SystemPython::Allowed, _) => true,
(SystemPython::Explicit, false) => {
if matches!(
source,
InterpreterSource::ProvidedPath | InterpreterSource::ParentInterpreter
) {
match result {
Copy link
Member Author

@zanieb zanieb Jun 5, 2024

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

Ok((source, interpreter)) => match (
system,
// Conda environments are not conformant virtual environments but we should not treat them as system interpreters
interpreter.is_virtualenv() || matches!(source, InterpreterSource::CondaPrefix),
) {
(SystemPython::Allowed, _) => true,
(SystemPython::Explicit, false) => {
if matches!(
source,
InterpreterSource::ProvidedPath | InterpreterSource::ParentInterpreter
) {
debug!(
"Allowing system Python interpreter at `{}`",
interpreter.sys_executable().display()
);
true
} else {
debug!(
"Ignoring Python interpreter at `{}`: system interpreter not explicit",
interpreter.sys_executable().display()
);
false
}
}
(SystemPython::Explicit, true) => true,
(SystemPython::Disallowed, false) => {
debug!(
"Allowing system Python interpreter at `{}`",
"Ignoring Python interpreter at `{}`: system interpreter not allowed",
interpreter.sys_executable().display()
);
true
} else {
false
}
(SystemPython::Disallowed, true) => true,
(SystemPython::Required, true) => {
debug!(
"Ignoring Python interpreter at `{}`: system interpreter not explicit",
"Ignoring Python interpreter at `{}`: system interpreter required",
interpreter.sys_executable().display()
);
false
}
}
(SystemPython::Explicit, true) => true,
(SystemPython::Disallowed, false) => {
debug!(
"Ignoring Python interpreter at `{}`: system interpreter not allowed",
interpreter.sys_executable().display()
);
false
}
(SystemPython::Disallowed, true) => true,
(SystemPython::Required, true) => {
debug!(
"Ignoring Python interpreter at `{}`: system interpreter required",
interpreter.sys_executable().display()
);
false
}
(SystemPython::Required, false) => true,
},
// Do not drop any errors
Err(_) => true,
(SystemPython::Required, false) => true,
},
// Do not drop any errors
Err(_) => true,
}
})
}

Expand Down Expand Up @@ -1161,6 +1183,7 @@ impl SourceSelector {
InterpreterSource::DiscoveredEnvironment,
InterpreterSource::ActiveEnvironment,
InterpreterSource::CondaPrefix,
InterpreterSource::ShimEnvironment,
]
.contains(&source),
Self::Custom(sources) => sources.contains(&source),
Expand Down Expand Up @@ -1231,6 +1254,7 @@ impl fmt::Display for InterpreterSource {
Self::PyLauncher => f.write_str("`py` launcher output"),
Self::ManagedToolchain => f.write_str("managed toolchains"),
Self::ParentInterpreter => f.write_str("parent interpreter"),
Self::ShimEnvironment => f.write_str("shim virtual environment"),
}
}
}
Expand Down
23 changes: 23 additions & 0 deletions crates/uv-interpreter/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,29 @@ mod tests {
Ok(())
}

#[test]
fn find_environment_from_virtual_environment_with_python_executable_on_path() -> Result<()> {
let context = TestContext::new()?;
let venv = context.tempdir.child(".venv");
TestContext::mock_venv(&venv, "3.12.0")?;

let environment =
context.run_with_vars(&[("PATH", Some(venv.child("bin").as_os_str()))], || {
PythonEnvironment::find(
None,
SystemPython::Allowed,
PreviewMode::Disabled,
&context.cache,
)
})?;
assert_eq!(
environment.interpreter().python_full_version().to_string(),
"3.12.0",
"We should find the active environment via the path"
);

Ok(())
}
#[test]
fn find_environment_from_conda_prefix() -> Result<()> {
let context = TestContext::new()?;
Expand Down