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

Friction log: optional deps in features list confusing #9459

Closed
wchargin opened this issue May 4, 2021 · 3 comments
Closed

Friction log: optional deps in features list confusing #9459

wchargin opened this issue May 4, 2021 · 3 comments
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-features Area: features — conditional compilation S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@wchargin
Copy link

wchargin commented May 4, 2021

Cargo's user experience confused me earlier today, so I wanted to share
my thoughts.

I have an existing binary that uses reqwests with the default OpenSSL
TLS backend. I wanted to change this to use rustls instead. So I made
the following change to my Cargo.toml:

-reqwest = { version = "0.11.0", features = ["blocking", "json"] }
+reqwest = { version = "0.11.0", features = ["blocking", "json", "rustls"], default-features = false }

I knew about disabling default features, so I did that off the bat, and
when I rebuilt I saw that rustls had been added to my Cargo.lock and
openssl-sys had been removed. So far, so good. But the code failed at
runtime:

Error: reqwest::Error { kind: Request, url: Url { scheme: "https", username: "", password: None, host: Some(Domain("example.com")), port: None, path: "/", query: None, fragment: None }, source: hyper::Error(Connect, "invalid URL, scheme is not http") }

The "invalid URL, scheme is not http" bit suggested to me that Rustls
wasn't actually wired up correctly, and I had only managed to disable
the default TLS backend. I poked at reqwest's Cargo.toml and saw
that there was also a reqwest-tls feature, so I changed my patch to:

-reqwest = { version = "0.11.0", features = ["blocking", "json"] }
+reqwest = { version = "0.11.0", features = ["blocking", "json", "rustls", "rustls-tls"], default-features = false }

…and this worked.

What happened? I had misread the reqwest docs, which clearly state
that the Rustls feature is called rustls-tls (turns out that I didn't
need "rustls" in the features array at all). Enabling "rustls" only
turned on the optional dependency without enabling any of the cfg
guards, so the functionality was still dead. I was extra confused
because I had tried setting features = ["zzz"], and Cargo politely
complained to me that "zzz" was not a feature. This led me to believe
that "rustls" was a feature, though it was not.

So, the final, correct patch was just:

-reqwest = { version = "0.11.0", features = ["blocking", "json"] }
+reqwest = { version = "0.11.0", features = ["blocking", "json", "rustls-tls"], default-features = false }

but it was harder for me to get there than perhaps it could have been.

I found it surprising that the features array also enables things that
are not features, and it occurred to me that if I were designing this
from scratch, I might not make that decision. There could be separate
fields for features = ["foo"] and deps = ["bar"], with appropriately
precise checking for each.

I searched briefly for related discussion on issues in this repo, but
didn't find anything. Other than this issue, Cargo is lovely, as always;
thank you! :-)

@wchargin wchargin added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label May 4, 2021
@Eh2406
Copy link
Contributor

Eh2406 commented May 4, 2021

You describe the problem well! You may want to look at https://doc.rust-lang.org/cargo/reference/unstable.html#namespaced-features

@ehuss
Copy link
Contributor

ehuss commented May 19, 2021

These kinds of experience reports are really helpful, so thanks for writing it up!

I believe this should be addressed by namespaced features (#5565). When that is stabilized, reqwest will have the option to hide the "rustls" feature and prevent the confusion when trying to enable it. Hopefully namespaced features will get stabilized soonish.

I think I'll leave this issue open to reconsider the error message. The current one shown below is not one of my best. At a minimum I think it should include some suggestions on what to use instead. Preferably it could be rewritten to be a little less technical.

the package `foo` depends on `reqwest`, with features: `rustls` but `reqwest`
does not have these features.
 It has an optional dependency with that name, but but that dependency uses
 the "dep:" syntax in the features table, so it does not have an implicit
 feature with that name.

@ehuss ehuss added A-diagnostics Area: Error and warning messages generated by Cargo itself. A-namespaced-features Area: namespaced-features and removed C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` labels May 19, 2021
@ehuss ehuss added A-features Area: features — conditional compilation E-help-wanted and removed A-namespaced-features Area: namespaced-features labels Feb 22, 2022
@weihanglo weihanglo added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed E-help-wanted labels Apr 19, 2023
@epage
Copy link
Contributor

epage commented Jul 11, 2024

Since this was opened, we have stabilized dep: which would allow reqwest to remove the rustls implicit feature so people only see rustls-tls. In Edition 2024, we are removing support for implicit features. As the ecosystem moves to Edition 2024, it will ensure everyone is exposing the correct set of features . Granted, some packages may still need to expose the optional dependencies for backwards compatibility purposes and they would need #7130 to tell people to stop using the,

Closing in favor of #12826

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-features Area: features — conditional compilation S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

No branches or pull requests

5 participants