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

Use native-tls/native-certs features of ureq crate #98

Conversation

lazareviczoran
Copy link
Contributor

Hello!

We use this crate on one of our projects, once we bumped it to version 0.10.7, we started getting failures for the license checks in cargo-deny, since openblas-build now uses ureq for downloading the openblas source, which brings in a dependency with a MPL-2.0 license to the Cargo.lock file (it has been brought up in ureq repo as well algesten/ureq#478).

ureq does have some feature flags that provide the possibility to use native-tls, the lib doesn't implement the default tls config when using native-tls feature flag (https://github.com/algesten/ureq/blob/main/src/lib.rs#L377-L397), but it allows to setup an agent explicitly by providing the tls config (as it is done in this test https://github.com/algesten/ureq/blob/main/src/lib.rs#L583-L595).
There was a proposal for adding feature flag in ureq that includes rustls with the native-certs flag so that the default agent can work with the native-certs (algesten/ureq#482), but it was rejected, so currently it is not possible to just tweak the feature flags on ureq and exclude the unwanted sub-dependencies.

The idea of this PR is to add feature flags to openblas-src and openblas-build that allows users to choose which tls config they want to use. This PR introduces 2 feature flags (tls which includes the default tls that ureq uses, and native-certs which uses native-tls and native-certs feature flags in ureq).
The default flags are set to use the tls feature flag, but worth to note is that this would probably be a breaking change when the default features are disabled and neither of these flags are enabled.

Any thoughts on this?
I look forward to you feedback!

@termoshtt
Copy link
Member

Thanks PR!

Using native-certs crate looks better for keeping dependencies list simple.

The default flags are set to use the tls feature flag, but worth to note is that this would probably be a breaking change when the default features are disabled and neither of these flags are enabled.

To avoid this issue, we should use only native-certs case without introducing the feature flag.

@lazareviczoran
Copy link
Contributor Author

Thanks PR!

Using native-certs crate looks better for keeping dependencies list simple.

The default flags are set to use the tls feature flag, but worth to note is that this would probably be a breaking change when the default features are disabled and neither of these flags are enabled.

To avoid this issue, we should use only native-certs case without introducing the feature flag.

Thanks for the feedback 🙌
I've removed the introduced feature flags and updated the PR to use native-certs by default as you suggested.

@lazareviczoran lazareviczoran changed the title Add feature flags to enable native-tls usage of ureq Use native-tls/native-certs features of ureq crate Jan 10, 2023
@termoshtt termoshtt merged commit ca7234a into blas-lapack-rs:master Jan 14, 2023
@termoshtt
Copy link
Member

0.10.8 has been released with this PR https://github.com/blas-lapack-rs/openblas-src/releases/tag/openblas-src-v0.10.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants