Allows configuring TLS backend #386
Conversation
|
Hm, I wonder if we shouldn't just switch to native roots and call it a day... What's your opinion @niklasmohrin? |
|
FYI, the default build uses the native roots. I didn't change the build default in this PR. I can see some adjustments required and your inputs. |
|
@dbrgn I have some thoughts written down in the issue, I think that having the functionality is nice and people can still build it without the bloat by disabling features |
Cargo.toml
Outdated
| @@ -25,7 +25,7 @@ app_dirs = { version = "2", package = "app_dirs2" } | |||
| clap = { version = "4", features = ["std", "derive", "help", "usage", "cargo", "error-context", "color", "wrap_help"], default-features = false } | |||
| env_logger = { version = "0.11", optional = true } | |||
| log = "0.4" | |||
| reqwest = { version = "0.12.5", features = ["blocking"], default-features = false } | |||
| reqwest = { version = "0.12.5", features = ["blocking", "native-tls", "rustls-tls", "rustls-tls-native-roots", "rustls-tls-webpki-roots"], default-features = false } | |||
There was a problem hiding this comment.
I had something else in mind: By disabling the native-roots (etc.) features of tealdeer, we would also disable the features for reqwest here, so that the resulting tealdeer binary is as small as possible.
There was a problem hiding this comment.
I see. Fixed. I saw the reqwest 0.12.9 supports additional roots. So I also extended it. What do you think about it?
src/cache.rs
Outdated
| cache_dir: PathBuf, | ||
| enable_styles: bool, | ||
| tls_backend: &'a str, // for setting up reqwest client |
There was a problem hiding this comment.
I think this should use an enum like so:
enum TlsBackend {
// ...
}This way we also avoid the lifetime parameter :)
src/config.rs
Outdated
| /// Allows choosing a TLS backend supported by `reqwest`. Available TLS backends: | ||
| /// # - `native-roots`: Rustls with native roots | ||
| /// # - `webpki-roots`: Rustls with `WebPK` roots | ||
| /// # - `native-tls`: Native TLS (`SChannel` on Windows, Secure Transport on macOS and OpenSSL otherwise) | ||
| /// Read more in `Cargo.toml` |
There was a problem hiding this comment.
This documentation can then probably move to the definition of the TlsBackend enum
src/cache.rs
Outdated
| let _ = Cache::build_client(&Cache { | ||
| cache_dir: dir.into_path(), | ||
| enable_styles: false, | ||
| tls_backend: $backend, | ||
| }); |
There was a problem hiding this comment.
We are not checking whether the returned Result of build_client is Ok here - let's do so
Additionally, I think we can make build_client not take the whole Cache but just the TlsBackend parameter. Then we won't need the tempdir and we can then remove the macro too and write out the three tests
Fixes #384.