grafana / pyroscope-rs

Pyroscope Profiler for Rust. Profile your Rust applications.
Apache License 2.0
132 stars 22 forks source link

Put openssl-sys under feature flag #99

Closed Voxelot closed 1 year ago

Voxelot commented 1 year ago

fixes: https://github.com/pyroscope-io/pyroscope-rs/issues/98

[features]
default = ["rustls-tls"]
rustls-tls = ["reqwest/rustls-tls"]
native-tls = ["reqwest/native-tls"]
CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

Voxelot commented 1 year ago

With current approach I don't see a way to build with libssl, am I missing something? It is not only about root certs, but also about the tls lib itself, right?

Maybe I was not clear previously, let me try to explain myself again.

Honestly now I think it should just be

[features]
default = ["rustls-tls"]
rustls-tls = ["reqwest/rustls-tls"]
native-tls = ["reqwest/native-tls"]

This way by default we use rustls-tls and as transitive dependency rustls-tls-webpki-roots And with native-tls enabled there will be no rustls at all and the binary will be linked with libssl.

Does it make sense?

My mistake, I thought these features were transitively the same. But it turns out that rustls-tls-native-roots doesn't fully enable all the same features as native-tls.

Some of the CI jobs in the last run timed out after 6 hours. Do you think these failures are related to the native certs not being configured correctly?

Voxelot commented 1 year ago

Thanks for getting this merged! When can we expect a new release?

cc @korniltsev

korniltsev commented 1 year ago

https://crates.io/crates/pyroscope/0.5.5 https://crates.io/crates/pyroscope_pprofrs/0.2.5

I've just pushed it

Please give it a try and let me know if everything works