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

Rework tls feature split to avoid unused dependencies #1160

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

axelfaure
Copy link

This is an attempt to rework the way tls related features are split in order to avoid depending on rustls-native-certs even when it is not actually used.

Fixes #1155

@nihohit
Copy link
Contributor

nihohit commented May 1, 2024

Hi @axelfaure, thanks for the contribution and sorry for the slow reply.
This looks great, but I'm not sure that having features that wrap multiple features is maintainable - we don't want to have a feature for each cartesian product of options. IMO we should strive to expose orthogonal features, so that the user could pick-and-choose freely.
@jaymell WDYT?

@nihohit
Copy link
Contributor

nihohit commented May 1, 2024

I see that you wrote in the original issue "I couldn't decorrelate the async framework from the tls backend without creating way more unused dependencies." - can you please explain this?

@axelfaure
Copy link
Author

Hi @nihohit, I fully agree with you and as you mentioned in your second comment that was indeed my initial intention.

Unfortunately I cannot have, let say, a native-tls feature on one hand and a tokio feature on the other without creating additional issue. This is because in that situation, I would need to express that tokio-native-tls dependency should only be present if both features are enabled. To the best of my knowledge this is not possible (I could do it in the rust code of course but not in the Cargo.toml). The same goes for futures-rustls, or tokio-rustls.

As far as I can tell, there seems to be 3 solutions to this problem:

  1. The cartesian product as implemented in this PR,
  2. Creating a lot of seemingly independent features that actually need to be correctly selected such as tokio, rustls, webpki-root and `tokio-rustls. This would create a lot of features making it way more diffucult for the end users to understand what to do (and I am not really sure it would improve maintainability either)
  3. Removing the need for these combined dependencies (tokio-native-tls, futures-rustls, tokio-rustls...). I am really not sure what they are needed for nor if it is possible to get rid of them without rewriting a lot of code. This could hypothetically be the best solution but it could also be quite difficult to achieve. Note that sqlx crate which does expose orthogonal features does seem to do without such dependencies, so this might be the way to go in the long run.

@nihohit
Copy link
Contributor

nihohit commented May 17, 2024

Hi @axelfaure, I really apologize - this change requires a deep review, and ATM I just don't have the time for it. I haven't forgotten, but it will probably take some time before I can properly answer you.

@axelfaure
Copy link
Author

Hi, Don't worry, I totally get it. Take your time.
Fixing the dependencies would be much nicer but in practice since the deps are not used, it does not have actual consequences on our binaries.

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.

Is there a way to use tokio + rustls +webpki-roots ?
2 participants