-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix handling of python-preference = system when managed interpreters are on the PATH
#15059
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 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 |
|---|---|---|
|
|
@@ -918,24 +918,7 @@ pub fn satisfies_python_preference( | |
| // If not "only" a kind, any interpreter is okay | ||
| PythonPreference::Managed | PythonPreference::System => true, | ||
| PythonPreference::OnlySystem => { | ||
| let is_system = match source { | ||
| // A managed interpreter is never a system interpreter | ||
| PythonSource::Managed => false, | ||
| // We can't be sure if this is a system interpreter without checking | ||
| PythonSource::ProvidedPath | ||
| | PythonSource::ParentInterpreter | ||
| | PythonSource::ActiveEnvironment | ||
| | PythonSource::CondaPrefix | ||
| | PythonSource::DiscoveredEnvironment | ||
| | PythonSource::SearchPath | ||
| | PythonSource::SearchPathFirst | ||
| | PythonSource::Registry | ||
| | PythonSource::BaseCondaPrefix => !interpreter.is_managed(), | ||
| // Managed interpreters should never be found in the store | ||
| PythonSource::MicrosoftStore => true, | ||
| }; | ||
|
|
||
| if is_system { | ||
| if is_system_interpreter(source, interpreter) { | ||
| true | ||
| } else { | ||
| if is_explicit { | ||
|
|
@@ -956,6 +939,25 @@ pub fn satisfies_python_preference( | |
| } | ||
| } | ||
|
|
||
| pub(crate) fn is_system_interpreter(source: PythonSource, interpreter: &Interpreter) -> bool { | ||
| match source { | ||
| // A managed interpreter is never a system interpreter | ||
| PythonSource::Managed => false, | ||
| // We can't be sure if this is a system interpreter without checking | ||
| PythonSource::ProvidedPath | ||
| | PythonSource::ParentInterpreter | ||
| | PythonSource::ActiveEnvironment | ||
| | PythonSource::CondaPrefix | ||
| | PythonSource::DiscoveredEnvironment | ||
| | PythonSource::SearchPath | ||
| | PythonSource::SearchPathFirst | ||
| | PythonSource::Registry | ||
| | PythonSource::BaseCondaPrefix => !interpreter.is_managed(), | ||
| // Managed interpreters should never be found in the store | ||
| PythonSource::MicrosoftStore => true, | ||
| } | ||
| } | ||
|
|
||
| /// Check if an encountered error is critical and should stop discovery. | ||
| /// | ||
| /// Returns false when an error could be due to a faulty Python installation and we should continue searching for a working one. | ||
|
|
@@ -1259,6 +1261,7 @@ pub(crate) fn find_python_installation( | |
| let installations = | ||
| find_python_installations(request, environments, preference, cache, preview); | ||
| let mut first_prerelease = None; | ||
| let mut first_managed = None; | ||
| let mut first_error = None; | ||
| for result in installations { | ||
| // Iterate until the first critical error or happy result | ||
|
|
@@ -1295,7 +1298,7 @@ pub(crate) fn find_python_installation( | |
| && !installation.source.allows_prereleases() | ||
| && !has_default_executable_name | ||
| { | ||
| debug!("Skipping pre-release {}", installation.key()); | ||
| debug!("Skipping pre-release installation {}", installation.key()); | ||
| if first_prerelease.is_none() { | ||
| first_prerelease = Some(installation.clone()); | ||
| } | ||
|
|
@@ -1315,12 +1318,41 @@ pub(crate) fn find_python_installation( | |
| continue; | ||
| } | ||
|
|
||
| // If it's a managed Python installation, and system interpreters are preferred, skip it | ||
| // for now. | ||
| if matches!(preference, PythonPreference::System) | ||
| && !is_system_interpreter(installation.source, installation.interpreter()) | ||
| { | ||
| debug!( | ||
| "Skipping managed installation {}: system installation preferred", | ||
| installation.key() | ||
| ); | ||
| if first_managed.is_none() { | ||
| first_managed = Some(installation.clone()); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| // If we didn't skip it, this is the installation to use | ||
| return result; | ||
| } | ||
|
|
||
| // If we only found managed installations, and the preference allows them, we should return | ||
| // the first one. | ||
| if let Some(installation) = first_managed { | ||
| debug!( | ||
| "Allowing managed installation {}: no system installations", | ||
| installation.key() | ||
| ); | ||
| return Ok(Ok(installation)); | ||
|
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. Should we log something here since we log skipping this installation earlier?
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. We don't for
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. Added |
||
| } | ||
|
|
||
| // If we only found pre-releases, they're implicitly allowed and we should return the first one. | ||
| if let Some(installation) = first_prerelease { | ||
| debug!( | ||
| "Allowing pre-release installation {}: no stable installations", | ||
| installation.key() | ||
| ); | ||
| return Ok(Ok(installation)); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -758,7 +758,7 @@ fn python_find_managed() { | |
| .with_filtered_python_sources() | ||
| .with_versions_as_managed(&["3.11"]); | ||
|
|
||
| // We find the unmanaged interpreter | ||
| // We find the unmanaged interpreter with managed Python disabled | ||
| uv_snapshot!(context.filters(), context.python_find().arg("--no-managed-python"), @r" | ||
| success: true | ||
| exit_code: 0 | ||
|
|
@@ -777,6 +777,26 @@ fn python_find_managed() { | |
| ----- stderr ----- | ||
| error: No interpreter found for Python 3.11 in [PYTHON SOURCES] | ||
| "); | ||
|
|
||
| // We find the unmanaged interpreter with system Python preferred | ||
| uv_snapshot!(context.filters(), context.python_find().arg("--python-preference").arg("system"), @r" | ||
| success: true | ||
| exit_code: 0 | ||
| ----- stdout ----- | ||
| [PYTHON-3.12] | ||
|
|
||
| ----- stderr ----- | ||
| "); | ||
|
Comment on lines
+781
to
+789
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 test case failed prior to this pull request, it'd discover 3.11. |
||
|
|
||
| // But, if no system Python meets the request, we'll use the managed interpreter | ||
| uv_snapshot!(context.filters(), context.python_find().arg("--python-preference").arg("system").arg("3.11"), @r" | ||
| success: true | ||
| exit_code: 0 | ||
| ----- stdout ----- | ||
| [PYTHON-3.11] | ||
|
|
||
| ----- stderr ----- | ||
| "); | ||
| } | ||
|
|
||
| /// See: <https://github.com/astral-sh/uv/issues/11825> | ||
|
|
||
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.
Moved without change