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

Compilation fails with no default features #356

Closed
tamasfe opened this issue Apr 12, 2022 · 3 comments · Fixed by #360
Closed

Compilation fails with no default features #356

tamasfe opened this issue Apr 12, 2022 · 3 comments · Fixed by #360

Comments

@tamasfe
Copy link
Contributor

tamasfe commented Apr 12, 2022

0.15.1 and 0.15.2 introduced feature changes/additions I don't necessarily agree with so I don't know what kind of PR to submit for this.

Having both a resolve-http and reqwest features that slightly differ in semantics makes no sense, since resolve-http includes the reqwest feature, it alone should be enough to enable the HTTP(S) schema resolution feature.

Also I believe exposing optional features of dependencies is just a way to pollute the features of this library and also leads to issues like this one. #343 could have been solved simply by suggesting the addition of reqwest = { version = "*", features = [ "rustls-tls" ] }, the current solution raises the question why only native-tls and rustls are exposed while reqwest has a handful more of TLS features and combinations.

@Stranger6667
Copy link
Owner

Stranger6667 commented Apr 20, 2022

Hi @tamasfe

First of all, thank you for opening this issue, as it makes clear to me that I didn't thoroughly think about those changes, and missed that they might lead to such consequences. Sorry for the complications 🙏

My intention was to avoid obscure errors like #343, and I got some inspiration from how Sentry handles their transports.
If there is no TLS feature in reqwest, then the error message from hyper isn't that helpful - error trying to connect: invalid URL, scheme is not http, and probably it would be better to improve the error message there instead of trying to guess the real cause from the message :( There is a closed issue in the Hyper repo that contains concerns about the error message, but those concerns remain unresolved.

I am inclined to think that for now this problem should be documented in the installation doc section, and those changes reverted, as they cause more harm than actually solve the problem. What do you think? Or is there any reliable way to prevent such cases as #343 upfront?

@tamasfe
Copy link
Contributor Author

tamasfe commented Apr 21, 2022

I am inclined to think that for now this problem should be documented in the installation doc section, and those changes reverted, as they cause more harm than actually solve the problem. What do you think? Or is there any reliable way to prevent such cases as #343 upfront?

I believe the best way to go here is really just documentation indeed. The misleading error message is unfortunate, but I still believe that libraries should not enable more features of dependencies than they need and should not re-export features unless they are affected by them.

If it was just a TLS on/off switch for reqwest, it could probably make sense to re-export it as a feature, but with the amount of TLS options it has, it's a lot easier to let the user configure it however they want instead.

@Stranger6667
Copy link
Owner

Thank you for sharing your thoughts on this! I agree with you.
#360 should resolve this.

Stranger6667 added a commit that referenced this issue Apr 21, 2022
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 a pull request may close this issue.

2 participants