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

Upgrade rustls to 0.20 #1505

Merged
merged 12 commits into from
Apr 14, 2022
Merged

Conversation

paolobarbolini
Copy link
Contributor

@paolobarbolini paolobarbolini commented Oct 22, 2021

In the process replaces async-rustls, which hasn't been updated yet, with futures-rustls.

@abonander
Copy link
Collaborator

If you rebase it should fix the MySQL build failure.

@paolobarbolini paolobarbolini marked this pull request as ready for review November 26, 2021 15:38
@paolobarbolini
Copy link
Contributor Author

This should be ready to be merged

sqlx-core/Cargo.toml Outdated Show resolved Hide resolved
@abonander abonander added this to the 0.6.0 milestone Dec 29, 2021
@c410-f3r
Copy link
Contributor

c410-f3r commented Jan 30, 2022

@abonander @mehcode

Is there a chance of at least having this PR merged in a separated branch? Rustls 0.20 was released in September last year

@paolobarbolini
Copy link
Contributor Author

paolobarbolini commented Jan 30, 2022

FWIW I have a branch with all of my PRs merged. I only test it on postgres though.

https://github.com/paolobarbolini/sqlx/tree/merged-0.6.0

@paolobarbolini
Copy link
Contributor Author

I wouldn't mind having a 0.6.0 alpha or something so I wouldn't have to maintain a git dependency

@c410-f3r
Copy link
Contributor

c410-f3r commented Feb 1, 2022

Thank you very much @paolobarbolini

@abonander
Copy link
Collaborator

abonander commented Feb 2, 2022

@paolobarbolini if you restore the removed impl then it's not a breaking change and we can put it out in a point-release. It looks like webpki::InvalidDnsNameError still exists in 0.22.

Feel free to mark it #[doc(hidden)] if you like, I don't think we want to advertise it anyway.

@paolobarbolini
Copy link
Contributor Author

@paolobarbolini if you restore the removed impl then it's not a breaking change and we can put it out in a point-release. It looks like webpki::InvalidDnsNameError still exists in 0.22.

Feel free to mark it #[doc(hidden)] if you like, I don't think we want to advertise it anyway.

It would still be a breaking change wouldn't it?

@abonander
Copy link
Collaborator

abonander commented Feb 2, 2022

Ugh, yeah. I really doubt anyone is using it either way.

It'd violate the spirit of semver, but is the breakage justified when you consider the importance of keeping RusTLS up to date? In my mind, it's on the same level of breakage as accidentally reintroducing the damn aHash cycle problem, which I still need to fix.

At the end of the day, the impl in question probably shouldn't have been part of the public API in the first place, as it was only meant for use internally.

@abonander
Copy link
Collaborator

It'd be really stupid, but we could theoretically keep webpki = "0.21" as a renamed dependency just so the impl keeps working as-is, with a TODO explaining why it still exists and that it should be removed in 0.6.0.

@paolobarbolini
Copy link
Contributor Author

It'd be really stupid, but we could theoretically keep webpki = "0.21" as a renamed dependency just so the impl keeps working as-is, with a TODO explaining why it still exists and that it should be removed in 0.6.0.

We could, but then when rustls and webpki upgrade to ring 0.17 we'll still be on 0.16 because of webpki 0.21. I'm thinking that we could silently make the breaking change and see how it goes. sqlx doesn't return any webpki types from the public API, so it'd be hard to find someone calling the impl.

@abonander
Copy link
Collaborator

The idea is that we'd upgrade to webpki = "0.22" and rustls = "0.20" and keep webpki = "0.21" around solely to keep the impl working. but not support it for webpki = "0.22" and then remove webpki = "0.21" and the impl in 0.6.0. Then, if new versions of rustls/webpki come out, we can upgrade them without worrying.

Or I could just stop whinging and kick out a 0.6.0 just for this. There's a part of me that really doesn't like that idea though.

I think it's just because I feel like a backwards-incompatible release should have some meat to it to justify the upgrade.

I guess there's a few other backwards-incompatible public dependency upgrades we could roll in and treat it as more of an incremental release. time = "0.3" for one.

@paolobarbolini
Copy link
Contributor Author

Or I could just stop whinging and kick out a 0.6.0 just for this. There's a part of me that really doesn't like that idea though.

I think it's just because I feel like a backwards-incompatible release should have some meat to it to justify the upgrade.

I guess there's a few other backwards-incompatible public dependency upgrades we could roll in and treat it as more of an incremental release. time = "0.3" for one.

It think the dependency upgrades plus the Rust 1.60 dependencies features thing would be a great excuse to make a 0.6 release. Rust 1.60 comes out in two months though, so we'll still have some time to see if there's anything else we can put in this release.

@djc
Copy link
Contributor

djc commented Mar 28, 2022

What's the current plan here? Would be nice to get this out so I don't have to keep building duplicate rustls stacks.

@abonander abonander merged commit 08296a2 into launchbadge:master Apr 14, 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 this pull request may close these issues.

4 participants