Fill Python requests with platform information during automatic fetches #4810
Conversation
| @@ -88,7 +88,7 @@ impl PythonInstallation { | |||
| // Perform a fetch aggressively if managed Python is preferred | |||
| if matches!(preference, PythonPreference::Managed) && python_fetch.is_automatic() { | |||
| if let Some(request) = PythonDownloadRequest::try_from_request(&request) { | |||
| return Self::fetch(request, client_builder, cache).await; | |||
| return Self::fetch(request.fill(), client_builder, cache).await; | |||
There was a problem hiding this comment.
Alternatively, perhaps we should be doing this a bit further down in Self::fetch?
There was a problem hiding this comment.
Or in ManagedPythonDownload::from_request to make sure we never miss it?
There was a problem hiding this comment.
Why don't we have to do it on line 106?
There was a problem hiding this comment.
We should. Great indication that we should enforce this elsewhere haha
There was a problem hiding this comment.
Could they have distinct types?
There was a problem hiding this comment.
Like fill returns a different type that you can only get by calling fill. That would have the benefit of making it impossible to miss while also giving us the flexibility to fill when we want (vs. coupling with the constructor).
There was a problem hiding this comment.
Yes that sounds reasonable but there are a weird amount of requests types to cast between already and I'd want to think a little more holistically if we add another.
|
@konstin could you check this resolves the issue on Linux please? |
|
The need for a lightweight testing story for Python toolchain commands grows... |
|
Still failing for me: |
|
@konstin that's because I didn't change it in the other place Charlie pointed out |
|
I'm gonna add and merge for now -- we can revisit if we wanna do something better. |
Closes #4800
We do this during
install— it's an important step to ensure the request has the platform information in it.