Fix selection of free-threaded interpreters during default Python discovery (#8239)

Closes https://github.com/astral-sh/uv/issues/8228

e.g., on this branch

```
❯ uv python install 3.13t 3.13
❯ cargo build
❯ cargo run -q --bin uvx -- --from build python -c "import sys; print(sys.base_prefix)"
/Users/zb/.local/share/uv/python/cpython-3.13.0-macos-aarch64-none
❯ cargo run -q --bin uvx -- -p 3.13 --from build python -c "import sys; print(sys.base_prefix)"
/Users/zb/.local/share/uv/python/cpython-3.13.0-macos-aarch64-none
❯ cargo run -q --bin uvx -- -p 3.13t --from build python -c "import sys; print(sys.base_prefix)"
/Users/zb/.local/share/uv/python/cpython-3.13.0+freethreaded-macos-aarch64-none
```

and on main

```
❯ cargo build
❯ cargo run -q --bin uvx -- --from build python -c "import sys; print(sys.base_prefix)"
Installed 3 packages in 12ms
/Users/zb/.local/share/uv/python/cpython-3.13.0+freethreaded-macos-aarch64-none
```

I want to add more test coverage around this, but I've noticed the
free-threaded discovery tests are a bit off as-is and it'll be a bigger
task. I think the recent bugs around discovery indicate we should invest
more into that test framework.
This commit is contained in:
Zanie Blue 2024-10-16 14:44:32 -05:00 committed by GitHub
parent 94a2686385
commit b851ced09e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 115 additions and 67 deletions

View file

@ -289,7 +289,7 @@ fn python_executables_from_environments<'a>(
/// This function does not guarantee that the executables are valid Python interpreters.
/// See [`python_interpreters_from_executables`].
fn python_executables_from_installed<'a>(
version: Option<&'a VersionRequest>,
version: &'a VersionRequest,
implementation: Option<&'a ImplementationName>,
preference: PythonPreference,
) -> Box<dyn Iterator<Item = Result<(PythonSource, PathBuf), Error>> + 'a> {
@ -306,11 +306,7 @@ fn python_executables_from_installed<'a>(
Ok(installations
.into_iter()
.filter(move |installation| {
if version.is_none()
|| version.is_some_and(|version| {
version.matches_version(&installation.version())
})
{
if version.matches_version(&installation.version()) {
true
} else {
debug!("Skipping incompatible managed installation `{installation}`");
@ -334,12 +330,8 @@ fn python_executables_from_installed<'a>(
{
// Skip interpreter probing if we already know the version doesn't match.
let version_filter = move |entry: &WindowsPython| {
if let Some(version_request) = version {
if let Some(version) = &entry.version {
version_request.matches_version(version)
} else {
true
}
if let Some(found) = &entry.version {
version.matches_version(found)
} else {
true
}
@ -395,7 +387,7 @@ fn python_executables_from_installed<'a>(
/// See [`python_executables_from_installed`] and [`python_executables_from_environments`]
/// for more information on discovery.
fn python_executables<'a>(
version: Option<&'a VersionRequest>,
version: &'a VersionRequest,
implementation: Option<&'a ImplementationName>,
environments: EnvironmentPreference,
preference: PythonPreference,
@ -440,15 +432,14 @@ fn python_executables<'a>(
/// If a `version` is not provided, we will only look for default executable names e.g.
/// `python3` and `python` — `python3.9` and similar will not be included.
fn python_executables_from_search_path<'a>(
version: Option<&'a VersionRequest>,
version: &'a VersionRequest,
implementation: Option<&'a ImplementationName>,
) -> impl Iterator<Item = PathBuf> + 'a {
// `UV_TEST_PYTHON_PATH` can be used to override `PATH` to limit Python executable availability in the test suite
let search_path = env::var_os(EnvVars::UV_TEST_PYTHON_PATH)
.unwrap_or(env::var_os(EnvVars::PATH).unwrap_or_default());
let version_request = version.unwrap_or(&VersionRequest::Default);
let possible_names: Vec<_> = version_request
let possible_names: Vec<_> = version
.executable_names(implementation)
.into_iter()
.map(|name| name.to_string())
@ -485,7 +476,7 @@ fn python_executables_from_search_path<'a>(
// parameters, and the dir is local while we return the iterator.
.collect::<Vec<_>>()
})
.chain(find_all_minor(implementation, version_request, &dir_clone))
.chain(find_all_minor(implementation, version, &dir_clone))
.filter(|path| !is_windows_store_shim(path))
.inspect(|path| trace!("Found possible Python executable: {}", path.display()))
.chain(
@ -577,7 +568,7 @@ fn find_all_minor(
///
/// See [`python_executables`] for more information on discovery.
fn python_interpreters<'a>(
version: Option<&'a VersionRequest>,
version: &'a VersionRequest,
implementation: Option<&'a ImplementationName>,
environments: EnvironmentPreference,
preference: PythonPreference,
@ -588,6 +579,7 @@ fn python_interpreters<'a>(
cache,
)
.filter(move |result| result_satisfies_environment_preference(result, environments))
.filter(move |result| result_satisfies_version_request(result, version))
}
/// Lazily convert Python executables into interpreters.
@ -661,6 +653,25 @@ fn satisfies_environment_preference(
}
}
/// Utility for applying [`VersionRequest::matches_interpreter`] to a result type.
fn result_satisfies_version_request(
result: &Result<(PythonSource, Interpreter), Error>,
request: &VersionRequest,
) -> bool {
result.as_ref().ok().map_or(true, |(source, interpreter)| {
let request = request.clone().into_request_for_source(*source);
if request.matches_interpreter(interpreter) {
true
} else {
debug!(
"Skipping interpreter at `{}` from {source}: does not satisfy request `{request}`",
interpreter.sys_executable().user_display()
);
false
}
})
}
/// Utility for applying [`satisfies_environment_preference`] to a result type.
fn result_satisfies_environment_preference(
result: &Result<(PythonSource, Interpreter), Error>,
@ -821,23 +832,18 @@ pub fn find_python_installations<'a>(
}
PythonRequest::Any => Box::new({
debug!("Searching for any Python interpreter in {preference}");
python_interpreters(
Some(&VersionRequest::Any),
None,
environments,
preference,
cache,
python_interpreters(&VersionRequest::Any, None, environments, preference, cache).map(
|result| {
result
.map(PythonInstallation::from_tuple)
.map(FindPythonResult::Ok)
},
)
.map(|result| {
result
.map(PythonInstallation::from_tuple)
.map(FindPythonResult::Ok)
})
}),
PythonRequest::Default => Box::new({
debug!("Searching for default Python interpreter in {preference}");
python_interpreters(
Some(&VersionRequest::Default),
&VersionRequest::Default,
None,
environments,
preference,
@ -855,32 +861,33 @@ pub fn find_python_installations<'a>(
};
Box::new({
debug!("Searching for {request} in {preference}");
python_interpreters(Some(version), None, environments, preference, cache)
.filter(|result| match result {
Err(_) => true,
Ok((_source, interpreter)) => version.matches_interpreter(interpreter),
})
.map(|result| {
result
.map(PythonInstallation::from_tuple)
.map(FindPythonResult::Ok)
})
})
}
PythonRequest::Implementation(implementation) => Box::new({
debug!("Searching for a {request} interpreter in {preference}");
python_interpreters(None, Some(implementation), environments, preference, cache)
.filter(|result| match result {
Err(_) => true,
Ok((_source, interpreter)) => interpreter
.implementation_name()
.eq_ignore_ascii_case(implementation.into()),
})
.map(|result| {
python_interpreters(version, None, environments, preference, cache).map(|result| {
result
.map(PythonInstallation::from_tuple)
.map(FindPythonResult::Ok)
})
})
}
PythonRequest::Implementation(implementation) => Box::new({
debug!("Searching for a {request} interpreter in {preference}");
python_interpreters(
&VersionRequest::Default,
Some(implementation),
environments,
preference,
cache,
)
.filter(|result| match result {
Err(_) => true,
Ok((_source, interpreter)) => interpreter
.implementation_name()
.eq_ignore_ascii_case(implementation.into()),
})
.map(|result| {
result
.map(PythonInstallation::from_tuple)
.map(FindPythonResult::Ok)
})
}),
PythonRequest::ImplementationVersion(implementation, version) => {
if let Err(err) = version.check_supported() {
@ -889,7 +896,7 @@ pub fn find_python_installations<'a>(
Box::new({
debug!("Searching for {request} in {preference}");
python_interpreters(
Some(version),
version,
Some(implementation),
environments,
preference,
@ -897,12 +904,9 @@ pub fn find_python_installations<'a>(
)
.filter(|result| match result {
Err(_) => true,
Ok((_source, interpreter)) => {
version.matches_interpreter(interpreter)
&& interpreter
.implementation_name()
.eq_ignore_ascii_case(implementation.into())
}
Ok((_source, interpreter)) => interpreter
.implementation_name()
.eq_ignore_ascii_case(implementation.into()),
})
.map(|result| {
result
@ -920,7 +924,7 @@ pub fn find_python_installations<'a>(
Box::new({
debug!("Searching for {request} in {preference}");
python_interpreters(
request.version(),
request.version().unwrap_or(&VersionRequest::Default),
request.implementation(),
environments,
preference,
@ -987,7 +991,7 @@ pub(crate) fn find_python_installation(
// If it's an alternative implementation and alternative implementations aren't allowed,
// skip it. Note we avoid querying these interpreters at all if they're on the search path
// and are not requested, but other sources such as the managed installations will include
// and are not requested, but other sources such as the managed installations can include
// them.
if installation.is_alternative_implementation()
&& !request.allows_alternative_implementations()
@ -1643,6 +1647,7 @@ impl std::fmt::Display for ExecutableName {
}
impl VersionRequest {
/// Return possible executable names for the given version request.
pub(crate) fn executable_names(
&self,
implementation: Option<&ImplementationName>,
@ -1726,6 +1731,7 @@ impl VersionRequest {
names
}
/// Return the major version segment of the request, if any.
pub(crate) fn major(&self) -> Option<u8> {
match self {
Self::Any | Self::Default | Self::Range(_, _) => None,
@ -1736,6 +1742,7 @@ impl VersionRequest {
}
}
/// Return the minor version segment of the request, if any.
pub(crate) fn minor(&self) -> Option<u8> {
match self {
Self::Any | Self::Default | Self::Range(_, _) => None,
@ -1746,6 +1753,7 @@ impl VersionRequest {
}
}
/// Return the patch version segment of the request, if any.
pub(crate) fn patch(&self) -> Option<u8> {
match self {
Self::Any | Self::Default | Self::Range(_, _) => None,
@ -1756,6 +1764,9 @@ impl VersionRequest {
}
}
/// Check if the request is for a version supported by uv.
///
/// If not, an `Err` is returned with an explanatory message.
pub(crate) fn check_supported(&self) -> Result<(), String> {
match self {
Self::Any | Self::Default => (),
@ -1804,7 +1815,30 @@ impl VersionRequest {
Ok(())
}
/// Check if a interpreter matches the requested Python version.
/// Change this request into a request appropriate for the given [`PythonSource`].
///
/// For example, if [`VersionRequest::Default`] is requested, it will be changed to
/// [`VersionRequest::Any`] for sources that should allow non-default interpreters like
/// free-threaded variants.
#[must_use]
pub(crate) fn into_request_for_source(self, source: PythonSource) -> Self {
match self {
Self::Default => match source {
PythonSource::ParentInterpreter
| PythonSource::CondaPrefix
| PythonSource::ProvidedPath
| PythonSource::DiscoveredEnvironment
| PythonSource::ActiveEnvironment => Self::Any,
PythonSource::SearchPath
| PythonSource::Registry
| PythonSource::MicrosoftStore
| PythonSource::Managed => Self::Default,
},
_ => self,
}
}
/// Check if a interpreter matches the request.
pub(crate) fn matches_interpreter(&self, interpreter: &Interpreter) -> bool {
match self {
Self::Any => true,
@ -1844,6 +1878,10 @@ impl VersionRequest {
}
}
/// Check if a version is compatible with the request.
///
/// WARNING: Use [`VersionRequest::matches_interpreter`] too. This method is only suitable to
/// avoid querying interpreters if it's clear it cannot fulfull the request.
pub(crate) fn matches_version(&self, version: &PythonVersion) -> bool {
match self {
Self::Any | Self::Default => true,
@ -1863,6 +1901,10 @@ impl VersionRequest {
}
}
/// Check if major and minor version segments are compatible with the request.
///
/// WARNING: Use [`VersionRequest::matches_interpreter`] too. This method is only suitable to
/// avoid querying interpreters if it's clear it cannot fulfull the request.
fn matches_major_minor(&self, major: u8, minor: u8) -> bool {
match self {
Self::Any | Self::Default => true,
@ -1882,6 +1924,11 @@ impl VersionRequest {
}
}
/// Check if major, minor, patch, and prerelease version segments are compatible with the
/// request.
///
/// WARNING: Use [`VersionRequest::matches_interpreter`] too. This method is only suitable to
/// avoid querying interpreters if it's clear it cannot fulfull the request.
pub(crate) fn matches_major_minor_patch_prerelease(
&self,
major: u8,
@ -1910,7 +1957,7 @@ impl VersionRequest {
}
}
/// Return true if a patch version is present in the request.
/// Whether a patch version segment is present in the request.
fn has_patch(&self) -> bool {
match self {
Self::Any | Self::Default => false,
@ -1924,7 +1971,7 @@ impl VersionRequest {
/// Return a new [`VersionRequest`] without the patch version if possible.
///
/// If the patch version is not present, it is returned unchanged.
/// If the patch version is not present, the request is returned unchanged.
#[must_use]
fn without_patch(self) -> Self {
match self {
@ -1955,6 +2002,7 @@ impl VersionRequest {
}
}
/// Whether this request is for a free-threaded Python variant.
pub(crate) fn is_freethreaded(&self) -> bool {
match self {
Self::Any | Self::Default => false,
@ -1989,7 +2037,7 @@ impl VersionRequest {
}
}
/// Return the required [`PythonVariant`] of the request.
/// Return the [`PythonVariant`] of the request, if any.
pub(crate) fn variant(&self) -> Option<PythonVariant> {
match self {
Self::Any => None,