-
Notifications
You must be signed in to change notification settings - Fork 1k
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
custom sources for Python distribution binaries #10939
base: main
Are you sure you want to change the base?
Conversation
crates/uv-python/src/downloads.rs
Outdated
// linked), so we pretend they don't exist. https://github.com/astral-sh/uv/issues/4242 | ||
.filter(|download| download.key.libc != Libc::Some(target_lexicon::Environment::Musl)) | ||
PYTHON_DOWNLOADS.get_or_init(|| { | ||
if let Ok(json_path) = std::env::var("UV_PYTHON_DOWNLOADS_JSON_PATH") { |
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.
Note this variable will need to be defined in crates/uv-static/src/env_vars.rs
I wonder if we should call it _URL
or _URI
? We can assume a local path and error with an informative message about the lack of remote file support for now, but we should have a forward-looking name.
We also need to be considerate about how the fetch is implemented, e.g., if we're making a network request it should probably be async which would mean this this function needs to be async (which I would be disappointed by). What kind of change would we need to make to avoid that?
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 don't think we need remote file support to add this feature — but I would like to know how invasive it would be)
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.
A remote file also introduces the complexity of caching 🤔 though perhaps we could avoid that since installs don't happen that often.
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.
crates/uv-python/src/downloads.rs
Outdated
let json_downloads: HashMap<String, JsonPythonDownload> = serde_json::from_reader(file) | ||
.unwrap_or_else(|e| { | ||
panic!("Failed to parse JSON file at {json_path}: {e}"); | ||
}); |
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.
Maybe a bit of a silly question, but is there a reason we shouldn't deserialize directly into ManagedPythonDownload
? Why the intermediate type?
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.
TBH, that's what the AI suggested, but I had the same thought as you, so I tried to derive the Deserialize
on ManagedPythonDownload
and then on PythonInstallationKey
. I understood that target_lexicon
's enums do not implement the Deserialize
trait, and even if I overcome this, the conversion from JSON will not be as is. So the only way to improve it (the way I see it) is to implement Deserialize
on our own and define the JSON structs inside the implementation.
If you see a better way, please let me know
crates/uv-python/src/downloads.rs
Outdated
|
||
let json_downloads: HashMap<String, JsonPythonDownload> = serde_json::from_reader(file) | ||
.unwrap_or_else(|e| { | ||
panic!("Failed to parse JSON file at {json_path}: {e}"); |
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.
A panic is probably not acceptable here. We'd need to handle this gracefully, i.e., by surfacing the error to the user.
…URL` and moved it to `crates/uv-static/src/env_vars.rs`
76c6528
to
1af245c
Compare
Summary
Add an option to overwrite the list of available Python downloads from a local JSON file by using the environment variable
UV_PYTHON_DOWNLOADS_JSON_URL
as an experimental support for providing custom sources for Python distribution binaries #8015
related #10203
I probably should make the JSON to be fetched from a remote URL instead of a local file.
please let me know what you think and I will modify the code accordingly.
Test Plan
normal run
empty JSON file
JSON file with valid version
Remote Path