Support http/https URLs in uv python --python-downloads-json-url
#14687
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,12 +10,11 @@ use std::{env, io}; | |
|
|
||
| use futures::TryStreamExt; | ||
| use itertools::Itertools; | ||
| use owo_colors::OwoColorize; | ||
| use reqwest_retry::{RetryError, RetryPolicy}; | ||
| use serde::Deserialize; | ||
| use thiserror::Error; | ||
| use tokio::io::{AsyncRead, AsyncReadExt, AsyncWriteExt, BufWriter, ReadBuf}; | ||
| use tokio_util::compat::FuturesAsyncReadCompatExt; | ||
| use tokio_util::either::Either; | ||
| use tracing::{debug, instrument}; | ||
|
|
@@ -99,10 +98,10 @@ pub enum Error { | |
| Mirror(&'static str, &'static str), | ||
| #[error("Failed to determine the libc used on the current platform")] | ||
| LibcDetection(#[from] LibcDetectionError), | ||
| #[error("Unable to parse the JSON Python download list at {0}")] | ||
| InvalidPythonDownloadsJSON(String, #[source] serde_json::Error), | ||
| #[error("This version of uv is too old to support the JSON Python download list at {0}")] | ||
| UnsupportedPythonDownloadsJSON(String), | ||
| #[error("An offline Python installation was requested, but {file} (from {url}) is missing in {}", python_builds_dir.user_display())] | ||
| OfflinePythonMissing { | ||
| file: Box<PythonInstallationKey>, | ||
|
|
@@ -368,15 +367,6 @@ impl PythonDownloadRequest { | |
| self.libc.as_ref() | ||
| } | ||
|
|
||
| /// Whether this request is satisfied by an installation key. | ||
| pub fn satisfied_by_key(&self, key: &PythonInstallationKey) -> bool { | ||
| if let Some(os) = &self.os { | ||
|
|
@@ -594,9 +584,11 @@ impl FromStr for PythonDownloadRequest { | |
| } | ||
| } | ||
|
|
||
| const BUILTIN_PYTHON_DOWNLOADS_JSON: &[u8] = include_bytes!("download-metadata-minified.json"); | ||
|
|
||
| pub struct ManagedPythonDownloadList { | ||
| downloads: Vec<ManagedPythonDownload>, | ||
| } | ||
|
|
||
| #[derive(Debug, Deserialize, Clone)] | ||
| struct JsonPythonDownload { | ||
|
|
@@ -625,24 +617,33 @@ pub enum DownloadResult { | |
| Fetched(PathBuf), | ||
| } | ||
|
|
||
| impl ManagedPythonDownloadList { | ||
| /// Iterate over all [`ManagedPythonDownload`]s. | ||
| fn iter_all(&self) -> impl Iterator<Item = &ManagedPythonDownload> { | ||
| self.downloads.iter() | ||
| } | ||
|
|
||
| /// Iterate over all [`ManagedPythonDownload`]s that match the request. | ||
| pub fn iter_matching( | ||
| &self, | ||
| request: &PythonDownloadRequest, | ||
| ) -> impl Iterator<Item = &ManagedPythonDownload> { | ||
| self.iter_all() | ||
| .filter(move |download| request.satisfied_by_download(download)) | ||
| } | ||
|
|
||
| /// Return the first [`ManagedPythonDownload`] matching a request, if any. | ||
| /// | ||
| /// If there is no stable version matching the request, a compatible pre-release version will | ||
| /// be searched for — even if a pre-release was not explicitly requested. | ||
| pub fn find(&self, request: &PythonDownloadRequest) -> Result<&ManagedPythonDownload, Error> { | ||
| if let Some(download) = self.iter_matching(request).next() { | ||
| return Ok(download); | ||
| } | ||
|
|
||
| if !request.allows_prereleases() { | ||
| if let Some(download) = self | ||
| .iter_matching(&request.clone().with_prereleases(true)) | ||
| .next() | ||
| { | ||
| return Ok(download); | ||
|
|
@@ -651,49 +652,88 @@ impl ManagedPythonDownload { | |
|
|
||
| Err(Error::NoDownloadFound(request.clone())) | ||
| } | ||
|
|
||
| /// Load available Python distributions from a provided source or the compiled-in list. | ||
| /// | ||
| /// `python_downloads_json_url` can be either `None`, to use the default list (taken from | ||
| /// `crates/uv-python/download-metadata.json`), or `Some` local path | ||
| /// or file://, http://, or https:// URL. | ||
| /// | ||
| /// Returns an error if the provided list could not be opened, if the JSON is invalid, or if it | ||
| /// does not parse into the expected data structure. | ||
| pub async fn new( | ||
| client: &BaseClient, | ||
| python_downloads_json_url: Option<&str>, | ||
| ) -> Result<Self, Error> { | ||
| // Although read_url() handles file:// URLs and converts them to local file reads, here we | ||
|
Member
There was a problem hiding this comment. I tried improving the URL parsing and reusability situation in #14712, but I didn't get anything that's noticeable better than the current logic |
||
| // want to also support parsing bare filenames like "/tmp/py.json", not just | ||
| // "file:///tmp/py.json". Note that "C:\Temp\py.json" should be considered a filename, even | ||
| // though Url::parse would successfully misparse it as a URL with scheme "C". | ||
| enum Source<'a> { | ||
| BuiltIn, | ||
| Path(Cow<'a, Path>), | ||
| Http(Url), | ||
| } | ||
|
|
||
| let json_source = if let Some(url_or_path) = python_downloads_json_url { | ||
| if let Ok(url) = Url::parse(url_or_path) { | ||
| match url.scheme() { | ||
| "http" | "https" => Source::Http(url), | ||
| "file" => Source::Path(Cow::Owned( | ||
| url.to_file_path().or(Err(Error::InvalidUrlFormat(url)))?, | ||
| )), | ||
| _ => Source::Path(Cow::Borrowed(Path::new(url_or_path))), | ||
| } | ||
| } else { | ||
| Source::Path(Cow::Borrowed(Path::new(url_or_path))) | ||
| } | ||
| } else { | ||
| Source::BuiltIn | ||
| }; | ||
|
|
||
| let buf: Cow<'_, [u8]> = match json_source { | ||
| Source::BuiltIn => BUILTIN_PYTHON_DOWNLOADS_JSON.into(), | ||
| Source::Path(ref path) => fs_err::read(path.as_ref())?.into(), | ||
| Source::Http(ref url) => { | ||
| let (mut reader, size) = read_url(url, client).await?; | ||
| let capacity = size.and_then(|s| s.try_into().ok()).unwrap_or(1_048_576); | ||
| let mut buf = Vec::with_capacity(capacity); | ||
| reader.read_to_end(&mut buf).await?; | ||
| buf.into() | ||
| } | ||
| }; | ||
| let json_downloads: HashMap<String, JsonPythonDownload> = serde_json::from_slice(&buf) | ||
| .map_err( | ||
| // As an explicit compatibility mechanism, if there's a top-level "version" key, it | ||
| // means it's a newer format than we know how to deal with. Before reporting a | ||
| // parse error about the format of JsonPythonDownload, check for that key. We can do | ||
| // this by parsing into a Map<String, IgnoredAny> which allows any valid JSON on the | ||
| // value side. (Because it's zero-sized, Clippy suggests Set<String>, but that won't | ||
| // have the same parsing effect.) | ||
| #[allow(clippy::zero_sized_map_values)] | ||
| |e| { | ||
| let source = match json_source { | ||
| Source::BuiltIn => "EMBEDDED IN THE BINARY".to_owned(), | ||
| Source::Path(path) => path.to_string_lossy().to_string(), | ||
| Source::Http(url) => url.to_string(), | ||
| }; | ||
| if let Ok(keys) = | ||
| serde_json::from_slice::<HashMap<String, serde::de::IgnoredAny>>(&buf) | ||
| && keys.contains_key("version") | ||
konstin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| Error::UnsupportedPythonDownloadsJSON(source) | ||
| } else { | ||
| Error::InvalidPythonDownloadsJSON(source, e) | ||
| } | ||
| }, | ||
| )?; | ||
|
|
||
| let result = parse_json_downloads(json_downloads); | ||
| Ok(Self { downloads: result }) | ||
| } | ||
| } | ||
|
|
||
| impl ManagedPythonDownload { | ||
| pub fn url(&self) -> &'static str { | ||
| self.url | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This should have a docstring. What's the tldr on why this can't just take a
BaseClientBuilder? I'm not worried about the performance, but it'd be a little clearer that it doesn't necessarily need a materialized client.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.
I looked at the callsites and it seems like you have a
BaseClientBuilderyou could provide instead?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.
It could, the only reason is that two of its callers construct a client shortly after calling this, and so if we passed a builder, we'd be calling
.build()twice.. If that's not a problem I can definitely change that (and lazy-construct the client if we're in the HTTP(S) case only). But I was worried I'd have to do something like pass a&mut Option<BaseClient>to pass the constructed client back to the caller to reuse it, or something.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.
I think it's probably fine if we build twice.