-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Filter managed Python distributions by platform before querying when included in request #13936
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ use uv_pep440::{ | |
| use uv_static::EnvVars; | ||
| use uv_warnings::warn_user_once; | ||
|
|
||
| use crate::downloads::PythonDownloadRequest; | ||
| use crate::downloads::{PlatformRequest, PythonDownloadRequest}; | ||
| use crate::implementation::ImplementationName; | ||
| use crate::installation::PythonInstallation; | ||
| use crate::interpreter::Error as InterpreterError; | ||
|
|
@@ -312,6 +312,7 @@ fn python_executables_from_virtual_environments<'a>() | |
| fn python_executables_from_installed<'a>( | ||
| version: &'a VersionRequest, | ||
| implementation: Option<&'a ImplementationName>, | ||
| platform: PlatformRequest, | ||
| preference: PythonPreference, | ||
| ) -> Box<dyn Iterator<Item = Result<(PythonSource, PathBuf), Error>> + 'a> { | ||
| let from_managed_installations = iter::once_with(move || { | ||
|
|
@@ -323,16 +324,19 @@ fn python_executables_from_installed<'a>( | |
| installed_installations.root().user_display() | ||
| ); | ||
| let installations = installed_installations.find_matching_current_platform()?; | ||
| // Check that the Python version satisfies the request to avoid unnecessary interpreter queries later | ||
| // Check that the Python version and platform satisfy the request to avoid unnecessary interpreter queries later | ||
| Ok(installations | ||
| .into_iter() | ||
| .filter(move |installation| { | ||
| if version.matches_version(&installation.version()) { | ||
| true | ||
| } else { | ||
| debug!("Skipping incompatible managed installation `{installation}`"); | ||
| false | ||
| if !version.matches_version(&installation.version()) { | ||
| debug!("Skipping managed installation `{installation}`: does not satisfy `{version}`"); | ||
| return false; | ||
| } | ||
| if !platform.matches(installation.key()) { | ||
| debug!("Skipping managed installation `{installation}`: does not satisfy `{platform}`"); | ||
| return false; | ||
| } | ||
| true | ||
| }) | ||
| .inspect(|installation| debug!("Found managed installation `{installation}`")) | ||
| .map(|installation| (PythonSource::Managed, installation.executable(false)))) | ||
|
|
@@ -415,15 +419,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`]. However, these filters are only applied for performance. We cannot | ||
| /// guarantee that the [`EnvironmentPreference`] is satisfied until we query the interpreter. | ||
| /// Note that Python executables may be excluded by the given [`EnvironmentPreference`], | ||
| /// [`PythonPreference`], and [`PlatformRequest`]. However, these filters are only applied for | ||
| /// performance. We cannot guarantee that the all requests or preferences are satisfied until we | ||
| /// query the interpreter. | ||
| /// | ||
| /// 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>, | ||
| platform: PlatformRequest, | ||
| environments: EnvironmentPreference, | ||
| preference: PythonPreference, | ||
| ) -> Box<dyn Iterator<Item = Result<(PythonSource, PathBuf), Error>> + 'a> { | ||
|
|
@@ -445,7 +451,8 @@ fn python_executables<'a>( | |
| .flatten(); | ||
|
|
||
| let from_virtual_environments = python_executables_from_virtual_environments(); | ||
| let from_installed = python_executables_from_installed(version, implementation, preference); | ||
| let from_installed = | ||
| python_executables_from_installed(version, implementation, platform, preference); | ||
|
|
||
| // Limit the search to the relevant environment preference; this avoids unnecessary work like | ||
| // traversal of the file system. Subsequent filtering should be done by the caller with | ||
|
|
@@ -630,12 +637,17 @@ fn find_all_minor( | |
|
|
||
| /// Lazily iterate over all discoverable Python interpreters. | ||
| /// | ||
| /// Note interpreters may be excluded by the given [`EnvironmentPreference`] and [`PythonPreference`]. | ||
| /// Note interpreters may be excluded by the given [`EnvironmentPreference`], [`PythonPreference`], | ||
| /// [`VersionRequest`], or [`PlatformRequest`]. | ||
| /// | ||
| /// The [`PlatformRequest`] is currently on applied to managed Python installations before querying | ||
zanieb marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| /// the interpreter. The caller is responsible for ensuring it is applied otherwise. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does "the caller is responsible" mean?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you call this function, you cannot assume that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (this is common for pre-filtering operations in this module) |
||
| /// | ||
| /// See [`python_executables`] for more information on discovery. | ||
| fn python_interpreters<'a>( | ||
| version: &'a VersionRequest, | ||
| implementation: Option<&'a ImplementationName>, | ||
| platform: PlatformRequest, | ||
| environments: EnvironmentPreference, | ||
| preference: PythonPreference, | ||
| cache: &'a Cache, | ||
|
|
@@ -644,7 +656,7 @@ fn python_interpreters<'a>( | |
| // Perform filtering on the discovered executables based on their source. This avoids | ||
| // unnecessary interpreter queries, which are generally expensive. We'll filter again | ||
| // with `interpreter_satisfies_environment_preference` after querying. | ||
| python_executables(version, implementation, environments, preference).filter_ok( | ||
| python_executables(version, implementation, platform, environments, preference).filter_ok( | ||
| move |(source, path)| { | ||
| source_satisfies_environment_preference(*source, path, environments) | ||
| }, | ||
|
|
@@ -971,14 +983,22 @@ pub fn find_python_installations<'a>( | |
| } | ||
| PythonRequest::Any => Box::new({ | ||
| debug!("Searching for any Python interpreter in {sources}"); | ||
| python_interpreters(&VersionRequest::Any, None, environments, preference, cache) | ||
| .map_ok(|tuple| Ok(PythonInstallation::from_tuple(tuple))) | ||
| python_interpreters( | ||
| &VersionRequest::Any, | ||
| None, | ||
| PlatformRequest::default(), | ||
| environments, | ||
| preference, | ||
| cache, | ||
| ) | ||
| .map_ok(|tuple| Ok(PythonInstallation::from_tuple(tuple))) | ||
| }), | ||
| PythonRequest::Default => Box::new({ | ||
| debug!("Searching for default Python interpreter in {sources}"); | ||
| python_interpreters( | ||
| &VersionRequest::Default, | ||
| None, | ||
| PlatformRequest::default(), | ||
| environments, | ||
| preference, | ||
| cache, | ||
|
|
@@ -991,15 +1011,23 @@ pub fn find_python_installations<'a>( | |
| } | ||
| Box::new({ | ||
| debug!("Searching for {request} in {sources}"); | ||
| python_interpreters(version, None, environments, preference, cache) | ||
| .map_ok(|tuple| Ok(PythonInstallation::from_tuple(tuple))) | ||
| python_interpreters( | ||
| version, | ||
| None, | ||
| PlatformRequest::default(), | ||
| environments, | ||
| preference, | ||
| cache, | ||
| ) | ||
| .map_ok(|tuple| Ok(PythonInstallation::from_tuple(tuple))) | ||
| }) | ||
| } | ||
| PythonRequest::Implementation(implementation) => Box::new({ | ||
| debug!("Searching for a {request} interpreter in {sources}"); | ||
| python_interpreters( | ||
| &VersionRequest::Default, | ||
| Some(implementation), | ||
| PlatformRequest::default(), | ||
| environments, | ||
| preference, | ||
| cache, | ||
|
|
@@ -1020,6 +1048,7 @@ pub fn find_python_installations<'a>( | |
| python_interpreters( | ||
| version, | ||
| Some(implementation), | ||
| PlatformRequest::default(), | ||
| environments, | ||
| preference, | ||
| cache, | ||
|
|
@@ -1043,6 +1072,7 @@ pub fn find_python_installations<'a>( | |
| python_interpreters( | ||
| request.version().unwrap_or(&VersionRequest::Default), | ||
| request.implementation(), | ||
| request.platform(), | ||
| environments, | ||
| preference, | ||
| cache, | ||
|
|
||
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.
Sounds like with this change it also impacts correctness?
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.
Eh, it prevents us from querying an interpreter that is likely to fail to run on the current platform, but I don't really intend for that to be a responsibility of this function. It's a nice side-effect that this pull request makes that unlikely, but it's more about performance.