-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Allow pip compile to fall back to find_best for --python
#17218
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 |
|---|---|---|
|
|
@@ -303,10 +303,11 @@ pub(crate) async fn pip_compile( | |
| install_mirrors.python_downloads_json_url.as_deref(), | ||
| ) | ||
| .await?; | ||
| let interpreter = if let Some(python) = python.as_ref() { | ||
|
|
||
| let (installation, python_missing_download) = if let Some(python) = python.as_ref() { | ||
| let request = PythonRequest::parse(python); | ||
| let reporter = PythonDownloadReporter::single(printer); | ||
| PythonInstallation::find_or_download( | ||
| match PythonInstallation::find_or_download( | ||
| Some(&request), | ||
| environment_preference, | ||
| python_preference, | ||
|
|
@@ -320,32 +321,52 @@ pub(crate) async fn pip_compile( | |
| preview, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(interpreter) => (Some(Ok(interpreter)), None), | ||
| Err(uv_python::Error::MissingPython(..)) => (None, Some(python)), | ||
| Err(uv_python::Error::Discovery(err)) if !err.is_critical() => (None, Some(python)), | ||
|
Comment on lines
+326
to
+327
Contributor
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. These are taken from |
||
| err => (Some(err), None), | ||
| } | ||
| } else { | ||
| // TODO(zanieb): The split here hints at a problem with the request abstraction; we should | ||
| // be able to use `PythonInstallation::find(...)` here. | ||
| let request = if let Some(version) = python_version.as_ref() { | ||
| // TODO(zanieb): We should consolidate `VersionRequest` and `PythonVersion` | ||
| PythonRequest::Version(VersionRequest::from(version)) | ||
| } else { | ||
| PythonRequest::default() | ||
| }; | ||
| PythonInstallation::find_best( | ||
| &request, | ||
| environment_preference, | ||
| python_preference, | ||
| &download_list, | ||
| &cache, | ||
| preview, | ||
| ) | ||
| }? | ||
| .into_interpreter(); | ||
| (None, None) | ||
| }; | ||
| let interpreter = installation | ||
| .unwrap_or_else(|| { | ||
| // TODO(zanieb): The split here hints at a problem with the request abstraction; we should | ||
| // be able to use `PythonInstallation::find(...)` here. | ||
| let request = if let Some(version) = python.as_ref() { | ||
| PythonRequest::parse(version) | ||
| } else if let Some(version) = python_version.as_ref() { | ||
| // TODO(zanieb): We should consolidate `VersionRequest` and `PythonVersion` | ||
| PythonRequest::Version(VersionRequest::from(version)) | ||
| } else { | ||
| PythonRequest::default() | ||
| }; | ||
| PythonInstallation::find_best( | ||
| &request, | ||
| environment_preference, | ||
| python_preference, | ||
| &download_list, | ||
| &cache, | ||
| preview, | ||
| ) | ||
| })? | ||
| .into_interpreter(); | ||
|
|
||
| debug!( | ||
| "Using Python {} interpreter at {} for builds", | ||
| interpreter.python_version(), | ||
| interpreter.sys_executable().user_display().cyan() | ||
| ); | ||
|
|
||
| if let Some(python_missing_download) = python_missing_download { | ||
| warn_user!( | ||
| "The requested Python version {} was not found and could not be downloaded; {} will be used to build dependencies instead.", | ||
|
Contributor
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. Maybe this should be |
||
| python_missing_download, | ||
| interpreter.python_version(), | ||
| ); | ||
| } | ||
|
|
||
|
Comment on lines
+362
to
+369
Contributor
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. There's an argument to be made here that this should print the original error too... Let me know. |
||
| if let Some(python_version) = python_version.as_ref() { | ||
| // If the requested version does not match the version we're using warn the user | ||
| // _unless_ they have not specified a patch version and that is the only difference | ||
|
|
||
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.
The reason why
python_missing_downloadis anOptioninstead of a bool is to avoid needing to check a bool andlet Some(python) = python.as_ref()a second time.The reason why this exists at all is to detect the case when the attempt to download failed in the first place, I couldn't find some way of testing if an
interpretersatisfied a specific version. If there is a way to do that, let me know, then this code can simplified.