From 4bf6a6bc50ce15e9c05d51fc511811f5e859f723 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Fri, 31 Jan 2025 13:39:24 -0600 Subject: [PATCH] Filter discovered Python executables by source before querying --- crates/uv-python/src/discovery.rs | 127 ++++++++++++++++++++++++------ crates/uv/tests/it/pip_install.rs | 6 +- 2 files changed, 106 insertions(+), 27 deletions(-) diff --git a/crates/uv-python/src/discovery.rs b/crates/uv-python/src/discovery.rs index bf60b6f3bb932..92e39b946a613 100644 --- a/crates/uv-python/src/discovery.rs +++ b/crates/uv-python/src/discovery.rs @@ -397,16 +397,17 @@ fn python_executables_from_installed<'a>( /// Lazily iterate over all discoverable Python executables. /// -/// Note that Python executables may be excluded by the given [`EnvironmentPreference`] and [`PythonPreference`]. +/// Note that Python executables may be excluded by the given [`EnvironmentPreference`] and +/// [`PythonPreference`]. /// -/// See [`python_executables_from_installed`] and [`python_executables_from_environments`] +/// See [`python_executables_from_installed`] and [`python_executables_from_virtual_environments`] /// for more information on discovery. fn python_executables<'a>( version: &'a VersionRequest, implementation: Option<&'a ImplementationName>, environments: EnvironmentPreference, preference: PythonPreference, -) -> Box> + 'a> { +) -> impl Iterator> + 'a { // Always read from `UV_INTERNAL__PARENT_INTERPRETER` — it could be a system interpreter let from_parent_interpreter = iter::once_with(|| { env::var_os(EnvVars::UV_INTERNAL__PARENT_INTERPRETER) @@ -429,22 +430,16 @@ fn python_executables<'a>( // Limit the search to the relevant environment preference; we later validate that they match // the preference but queries are expensive and we query less interpreters this way. - match environments { - EnvironmentPreference::OnlyVirtual => { - Box::new(from_parent_interpreter.chain(from_virtual_environments)) - } - EnvironmentPreference::ExplicitSystem | EnvironmentPreference::Any => Box::new( - from_parent_interpreter - .chain(from_virtual_environments) - .chain(from_base_conda_environment) - .chain(from_installed), - ), - EnvironmentPreference::OnlySystem => Box::new( - from_parent_interpreter - .chain(from_base_conda_environment) - .chain(from_installed), - ), - } + from_parent_interpreter + .chain(from_virtual_environments) + .chain(from_base_conda_environment) + .chain(from_installed) + .filter(move |result| { + result.is_err() + || result.as_ref().is_ok_and(|(source, path)| { + source_satisfies_environment_preference(*source, path, environments) + }) + }) } /// Lazily iterate over Python executables in the `PATH`. @@ -630,8 +625,13 @@ fn python_interpreters_from_executables<'a>( }) } -/// Returns true if a Python interpreter matches the [`EnvironmentPreference`]. -fn satisfies_environment_preference( +/// Whether a [`Interpreter`] matches the [`EnvironmentPreference`]. +/// +/// This is the correct way to determine if an interpreter matches the preference. In contrast, +/// [`source_satisfies_environment_preference`] only checks if a [`PythonSource`] **could** satisfy +/// preference as a pre-filtering step. We cannot definitely know if a Python interpreter is in +/// a virtual environment until we query it. +fn interpreter_satisfies_environment_preference( source: PythonSource, interpreter: &Interpreter, preference: EnvironmentPreference, @@ -680,6 +680,55 @@ fn satisfies_environment_preference( } } +/// Returns true if a [`PythonSource`] could satisfy the [`EnvironmentPreference`]. +/// +/// This is useful as a pre-filtering step. Use of [`interpreter_satisfies_environment_preference`] +/// is required to determine if an [`Interpreter`] satisfies the preference. +/// +/// The interpreter path is only used for debug messages. +fn source_satisfies_environment_preference( + source: PythonSource, + interpreter_path: &Path, + preference: EnvironmentPreference, +) -> bool { + match preference { + EnvironmentPreference::Any => true, + EnvironmentPreference::OnlyVirtual => { + if source.is_maybe_virtualenv() { + true + } else { + debug!( + "Ignoring Python interpreter at `{}`: only virtual environments allowed", + interpreter_path.display() + ); + false + } + } + EnvironmentPreference::ExplicitSystem => { + if source.is_maybe_virtualenv() { + true + } else { + debug!( + "Ignoring Python interpreter at `{}`: system interpreter not explicitly requested", + interpreter_path.display() + ); + false + } + } + EnvironmentPreference::OnlySystem => { + if source.is_maybe_system() { + true + } else { + debug!( + "Ignoring Python interpreter at `{}`: system interpreter required", + interpreter_path.display() + ); + false + } + } + } +} + /// Utility for applying [`VersionRequest::matches_interpreter`] to a result type. fn result_satisfies_version_request( result: &Result<(PythonSource, Interpreter), Error>, @@ -705,7 +754,7 @@ fn result_satisfies_environment_preference( preference: EnvironmentPreference, ) -> bool { result.as_ref().ok().map_or(true, |(source, interpreter)| { - satisfies_environment_preference(*source, interpreter, preference) + interpreter_satisfies_environment_preference(*source, interpreter, preference) }) } @@ -1575,6 +1624,40 @@ impl PythonSource { | Self::DiscoveredEnvironment => true, } } + + /// Whether this source **could** be a virtual environment. + /// + /// This excludes the [`PythonSource::SearchPath`] although it could be in a virtual + /// environment; pragmatically, that's not common and saves us from querying a bunch of system + /// interpreters for no reason. It seems dubious to consider an interpreter in the `PATH` as a + /// target virtual environment if it's not discovered through our virtual environment-specific + /// patterns. + pub(crate) fn is_maybe_virtualenv(self) -> bool { + match self { + Self::ProvidedPath + | Self::ActiveEnvironment + | Self::DiscoveredEnvironment + | Self::CondaPrefix + | Self::BaseCondaPrefix + | Self::ParentInterpreter => true, + Self::Managed | Self::SearchPath | Self::Registry | Self::MicrosoftStore => false, + } + } + + /// Whether this source **could** be a system interpreter. + pub(crate) fn is_maybe_system(self) -> bool { + match self { + Self::CondaPrefix + | Self::BaseCondaPrefix + | Self::ParentInterpreter + | Self::ProvidedPath + | Self::Managed + | Self::SearchPath + | Self::Registry + | Self::MicrosoftStore => true, + Self::ActiveEnvironment | Self::DiscoveredEnvironment => false, + } + } } impl PythonPreference { diff --git a/crates/uv/tests/it/pip_install.rs b/crates/uv/tests/it/pip_install.rs index fb66018986c11..8ffc5db586c6b 100644 --- a/crates/uv/tests/it/pip_install.rs +++ b/crates/uv/tests/it/pip_install.rs @@ -7542,11 +7542,7 @@ fn install_incompatible_python_version_interpreter_broken_in_path() -> Result<() ----- stdout ----- ----- stderr ----- - error: Failed to inspect Python interpreter from search path at `/Users/zb/.local/share/uv/tests/.tmpDRf1pq/bin/python3` - Caused by: Querying Python at `/Users/zb/.local/share/uv/tests/.tmpDRf1pq/bin/python3` failed with exit status exit status: 1 - - [stderr] - error: intentionally broken python executable + error: No virtual environment found for Python 3.12; run `uv venv` to create an environment, or pass `--system` to install into a non-virtual environment "### );