-
Notifications
You must be signed in to change notification settings - Fork 3k
Add UV_UPLOAD_HTTP_TIMEOUT and respect UV_HTTP_TIMEOUT in uploads
#16040
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 2 commits
af3ade3
39e23f7
8402c0e
0324748
271141c
a6fa49a
31cc0bf
9315c19
ec6cd47
f353984
ffe794f
348a10e
f2a5058
57f0d87
252f359
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 |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| use std::ops::Deref; | ||
| use std::path::{Path, PathBuf}; | ||
|
|
||
| use std::time::Duration; | ||
|
Member
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 import should be in the group above.
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. I think we cannot group path and time
Member
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 should be whitespace between the std imports and the uv imports
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. Added |
||
| use uv_dirs::{system_config_file, user_config_dir}; | ||
| use uv_fs::Simplified; | ||
| use uv_static::EnvVars; | ||
|
|
@@ -551,7 +551,8 @@ pub enum Error { | |
| #[error("Failed to parse: `{}`", _0.user_display())] | ||
| UvToml(PathBuf, #[source] Box<toml::de::Error>), | ||
|
|
||
| #[error("Failed to parse: `{}`. The `{}` field is not allowed in a `uv.toml` file. `{}` is only applicable in the context of a project, and should be placed in a `pyproject.toml` file instead.", _0.user_display(), _1, _1)] | ||
| #[error("Failed to parse: `{}`. The `{}` field is not allowed in a `uv.toml` file. `{}` is only applicable in the context of a project, and should be placed in a `pyproject.toml` file instead.", _0.user_display(), _1, _1 | ||
| )] | ||
| PyprojectOnlyField(PathBuf, &'static str), | ||
|
|
||
| #[error("Failed to parse environment variable `{name}` with invalid value `{value}`: {err}")] | ||
|
|
@@ -572,6 +573,7 @@ pub struct EnvironmentOptions { | |
| pub python_install_registry: Option<bool>, | ||
| pub install_mirrors: PythonInstallMirrors, | ||
| pub log_context: Option<bool>, | ||
| pub http_timeout: Option<Duration>, | ||
| } | ||
|
|
||
| impl EnvironmentOptions { | ||
|
|
@@ -594,6 +596,14 @@ impl EnvironmentOptions { | |
| )?, | ||
| }, | ||
| log_context: parse_boolish_environment_variable(EnvVars::UV_LOG_CONTEXT)?, | ||
| // Timeout options, matching https://doc.rust-lang.org/nightly/cargo/reference/config.html#httptimeout | ||
| // `UV_REQUEST_TIMEOUT` is provided for backwards compatibility with v0.1.6 | ||
| http_timeout: parse_integer_environment_variable(EnvVars::UV_HTTP_TIMEOUT)? | ||
| .or(parse_integer_environment_variable( | ||
| EnvVars::UV_REQUEST_TIMEOUT, | ||
| )?) | ||
| .or(parse_integer_environment_variable(EnvVars::HTTP_TIMEOUT)?) | ||
| .map(Duration::from_secs), | ||
| }) | ||
| } | ||
| } | ||
|
|
@@ -670,3 +680,25 @@ fn parse_string_environment_variable(name: &'static str) -> Result<Option<String | |
| }, | ||
| } | ||
| } | ||
|
|
||
| /// Parse a integer environment variable. | ||
| fn parse_integer_environment_variable(name: &'static str) -> Result<Option<u64>, Error> { | ||
|
Member
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. I think as written, this will return an error on an empty string instead of
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. Fixed |
||
| match std::env::var(name) { | ||
| Ok(v) => match v.parse::<u64>() { | ||
|
Member
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. I would probably not nest these matches, I'd do an early return if the environment variable is not present or not unicode.
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. refactored |
||
| Ok(v) => Ok(Some(v)), | ||
| Err(err) => Err(Error::InvalidEnvironmentVariable { | ||
| name: name.to_string(), | ||
| value: err.to_string(), | ||
|
Member
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. Doesn't this return a https://doc.rust-lang.org/std/num/struct.ParseIntError.html we should use as the error message not a value?
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. yes, fixed
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. Added test for this |
||
| err: "expected an integer".to_string(), | ||
| }), | ||
| }, | ||
| Err(e) => match e { | ||
| std::env::VarError::NotPresent => Ok(None), | ||
| std::env::VarError::NotUnicode(err) => Err(Error::InvalidEnvironmentVariable { | ||
| name: name.to_string(), | ||
| value: err.to_string_lossy().to_string(), | ||
| err: "expected an integer".to_string(), | ||
| }), | ||
| }, | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,11 +97,14 @@ async fn pyx_logout( | |
| printer: Printer, | ||
| ) -> Result<ExitStatus> { | ||
| // Initialize the client. | ||
| let client = BaseClientBuilder::default() | ||
| .connectivity(network_settings.connectivity) | ||
| .native_tls(network_settings.native_tls) | ||
| .allow_insecure_host(network_settings.allow_insecure_host.clone()) | ||
| .build(); | ||
| let client = BaseClientBuilder::new( | ||
| network_settings.connectivity, | ||
| network_settings.native_tls, | ||
| network_settings.allow_insecure_host.clone(), | ||
| Preview::default(), | ||
|
Member
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 should be using the user-provided
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. added |
||
| network_settings.timeout, | ||
| ) | ||
| .build(); | ||
|
|
||
| // Retrieve the token store. | ||
| let Some(tokens) = store.read().await? else { | ||
|
|
||
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.
We should probably remove
self.default_timeoutentirely and unwrap to that during settings resolution.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 did not do this because we use 15min default for upload_client
is it ok to override timeout at all at upload_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 deleted
default_timeoutbut it will be quite easy to revert it if it is better to have default timeout