-
Notifications
You must be signed in to change notification settings - Fork 738
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
Require opt-in to use alternative Python implementations #7650
Changes from all 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 |
---|---|---|
|
@@ -934,21 +934,44 @@ pub(crate) fn find_python_installation( | |
return result; | ||
}; | ||
|
||
// If it's a pre-release, and pre-releases aren't allowed skip it but store it for later | ||
// Check if we need to skip the interpreter because it is "not allowed", e.g., if it is a | ||
// pre-release version or an alternative implementation, using it requires opt-in. | ||
|
||
// If the interpreter has a default executable name, e.g. `python`, and was found on the | ||
// search path, we consider this opt-in to use it. | ||
let has_default_executable_name = installation.interpreter.has_default_executable_name() | ||
&& installation.source == PythonSource::SearchPath; | ||
|
||
// If it's a pre-release and pre-releases aren't allowed, skip it — but store it for later | ||
// since we'll use a pre-release if no other versions are available. | ||
if installation.python_version().pre().is_some() | ||
&& !request.allows_prereleases() | ||
&& !installation.source.allows_prereleases() | ||
&& !has_default_executable_name | ||
{ | ||
debug!("Skipping pre-release {}", installation.key()); | ||
first_prerelease = Some(installation.clone()); | ||
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. Does this need to be under 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. Yes! #7666 |
||
continue; | ||
} | ||
|
||
// 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 | ||
// them. | ||
if installation.is_alternative_implementation() | ||
&& !request.allows_alternative_implementations() | ||
&& !installation.source.allows_alternative_implementations() | ||
&& !has_default_executable_name | ||
{ | ||
debug!("Skipping alternative implementation {}", installation.key()); | ||
continue; | ||
} | ||
|
||
// If we didn't skip it, this is the installation to use | ||
return result; | ||
} | ||
|
||
// If we only found pre-releases, they're implicitly allowed and we should return the first one | ||
// If we only found pre-releases, they're implicitly allowed and we should return the first one. | ||
if let Some(installation) = first_prerelease { | ||
return Ok(Ok(installation)); | ||
} | ||
|
@@ -1205,10 +1228,7 @@ impl PythonRequest { | |
for implementation in | ||
ImplementationName::long_names().chain(ImplementationName::short_names()) | ||
{ | ||
if let Some(remainder) = value | ||
.to_ascii_lowercase() | ||
.strip_prefix(Into::<&str>::into(implementation)) | ||
{ | ||
if let Some(remainder) = value.to_ascii_lowercase().strip_prefix(implementation) { | ||
// e.g. `pypy` | ||
if remainder.is_empty() { | ||
return Self::Implementation( | ||
|
@@ -1369,6 +1389,7 @@ impl PythonRequest { | |
} | ||
} | ||
|
||
/// Whether this request opts-in to a pre-release Python version. | ||
pub(crate) fn allows_prereleases(&self) -> bool { | ||
match self { | ||
Self::Default => false, | ||
|
@@ -1381,6 +1402,19 @@ impl PythonRequest { | |
} | ||
} | ||
|
||
/// Whether this request opts-in to an alternative Python implementation, e.g., PyPy. | ||
pub(crate) fn allows_alternative_implementations(&self) -> bool { | ||
match self { | ||
Self::Default => false, | ||
Self::Any => true, | ||
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. Why is 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. Per #7526 we want to distinguish between discovery that allows any interpreter and discovery of a good default interpreter. |
||
Self::Version(_) => false, | ||
Self::Directory(_) | Self::File(_) | Self::ExecutableName(_) => true, | ||
Self::Implementation(_) => true, | ||
Self::ImplementationVersion(_, _) => true, | ||
Self::Key(request) => request.allows_alternative_implementations(), | ||
} | ||
} | ||
|
||
pub(crate) fn is_explicit_system(&self) -> bool { | ||
matches!(self, Self::File(_) | Self::Directory(_)) | ||
} | ||
|
@@ -1410,7 +1444,7 @@ impl PythonSource { | |
matches!(self, Self::Managed) | ||
} | ||
|
||
/// Whether a pre-release Python installation from the source should be used without opt-in. | ||
/// Whether a pre-release Python installation from this source can be used without opt-in. | ||
pub(crate) fn allows_prereleases(self) -> bool { | ||
match self { | ||
Self::Managed | Self::Registry | Self::MicrosoftStore => false, | ||
|
@@ -1422,6 +1456,18 @@ impl PythonSource { | |
| Self::DiscoveredEnvironment => true, | ||
} | ||
} | ||
|
||
/// Whether an alternative Python implementation from this source can be used without opt-in. | ||
pub(crate) fn allows_alternative_implementations(self) -> bool { | ||
match self { | ||
Self::Managed | Self::Registry | Self::SearchPath | Self::MicrosoftStore => false, | ||
Self::CondaPrefix | ||
| Self::ProvidedPath | ||
| Self::ParentInterpreter | ||
| Self::ActiveEnvironment | ||
| Self::DiscoveredEnvironment => true, | ||
} | ||
} | ||
} | ||
|
||
impl PythonPreference { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1793,7 +1793,7 @@ mod tests { | |
})?; | ||
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. Note we don't have test coverage for this because we don't test Python discovery with managed interpreters. I'll create an issue to track infrastructure for this. |
||
assert!( | ||
matches!(result, Err(PythonNotFound { .. })), | ||
"We should not the pypy interpreter if not named `python` or requested; got {result:?}" | ||
"We should not find the pypy interpreter if not named `python` or requested; got {result:?}" | ||
); | ||
|
||
// But we should find it | ||
|
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.
Oh ok, this makes sense, comment is helpful.