Skip to content
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

disable reqwest default-tls #261

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Conversation

jxs
Copy link
Contributor

@jxs jxs commented Sep 21, 2023

Hi, this allows not pulling the hyper-tls and its openssl-sys dependency and use rustls instead by downstream crates.
Thanks

@jxs jxs force-pushed the remove-hyper-tls-dep branch from 3013fc6 to c1e5331 Compare September 21, 2023 18:15
and allow downstream crates to choose their tls flavor
@jxs jxs force-pushed the remove-hyper-tls-dep branch from c1e5331 to 26429d7 Compare September 21, 2023 18:16
@jxs jxs changed the title remove unrequired hyper-tls dep disable reqwest default-tls Sep 21, 2023
@ralexstokes
Copy link
Owner

hi @jxs thanks for this!

do you have a link to any description of the problem? I know lighthouse had some issue here so maybe you can just link to a relevant PR?

@jxs
Copy link
Contributor Author

jxs commented Sep 23, 2023

Hi Alex.
We are currently trying to reduce Lighthouse's compilation time, hyper-tls and namely openssl-sys (due to static linking of openssl) is one of the biggest bottlenecks, one can confirm by running cargo build --release --timings.

After merging sigp/lighthouse#4650 and aligning reqwest's version we can now switch to rustls and not depend on
hyper-tls. But beacon-api-client depends on hyper-tls by default and in a non modifiable way which makes all downstream crates still need to fetch and compile hyper-tls.
This PR fixes that by allowing downstream crates to chose their tls flavour and for Lighthouse not pull hyper-tls and openssl-sys while still offering it as the default.

@ralexstokes
Copy link
Owner

makes sense! thanks for the explanation

@ralexstokes ralexstokes merged commit 0d085c4 into ralexstokes:main Sep 25, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants