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

feat(client): backport split client conn modules #3063

Closed
wants to merge 3 commits into from
Closed

feat(client): backport split client conn modules #3063

wants to merge 3 commits into from

Conversation

kxt
Copy link

@kxt kxt commented Nov 25, 2022

Backport client::conn split modules to ease transition to the 1.0 API and deprecate client::conn::Builder and client::conn::handshake.

Closes #3053

@seanmonstar
Copy link
Member

Thanks for getting started on this! I'll try to give CI a kick frequently, looks like it needs a cargo fmt run, and perhaps allow the warning for the C API? Looks like compile errors, actually...

@seanmonstar
Copy link
Member

I'm trying to get Miri fixed, but looks like there is a legit failure reported, that the unit tests are using the deprecated API (as they should, we shouldn't remove tests), and that triggers the deny(warnings). Could fix that either with a #![allow(deprecated)] at the top of the file, though that means we won't notice any other crates deprecations, or local #[allow(deprecated)] attributes at each usage location.

@kxt
Copy link
Author

kxt commented Dec 11, 2022

Sorry about the previously failing CI tests, I'm not familiar with GH workflows and it took a while for me to set them up working.
The CI tests ran green bar the Miri tests, I don't know what's up with those.

@seanmonstar seanmonstar linked an issue Dec 21, 2022 that may be closed by this pull request
@seanmonstar
Copy link
Member

Sorry to delay a little, I wanted to check on something. We've had some useful discussion about deprecations in #3052. It seems worth it to put all deprecation warnings behind a deprecated cargo feature, that we will have off by default to start, and eventually add to the default-features.

Do you want to add that to this PR? If it's too much to do, I can also do it, just let me know :D

@kxt
Copy link
Author

kxt commented Dec 23, 2022

I can add it in the next few days. However, the deprecated feature sound odd to me, a feature that causes compile errors/warnings without offering a viable alternative. Having the deprecation behind the 1_0-backports feature would make more sense to me. But I haven't read the mentioned clap issue yet, so there might be a good reason to the deprecated feature that I'm missing.

Also, I guess making the deprecated warnings default should be done with a minor version bump (ie. 0.15) per semantic versioning.

@kxt kxt changed the title 3053 backport split conn modules feat(client): backport split client conn modules Jan 3, 2023
This patch backports client/conn/http1 and http2 modules from 1.0 to
ease transition. It allows code still using 0.14.x to switch to the
per-version Connection types available in 1.0.
client::conn::Builder and client:conn:handshake are deprecated as they
are removed in 1.0.

tower_client is updated so it does not use the deprecated API.
@kxt
Copy link
Author

kxt commented Jan 4, 2023

I've added the feature deprecated and renamed 1_0-backports to backports to be in line with #3102. #3052 should be updated with this terminology.

@howardjohn
Copy link
Contributor

I'd be interesting with helper out here if there is any work needed

@kxt
Copy link
Author

kxt commented Jan 31, 2023

I also wonder if I can do anything to move this forward.

@seanmonstar
Copy link
Member

Sorry, there were a couple of new breaking changes that I realized needed to be made to 1.0. We've made them now. It'd be helpful to users to make sure the "backport helpers" match what ends up in 1.0, so we needed to make them first. For instance, in this PR, the builders no longer have their methods prefixed with the HTTP version (so, not http1_writev(), etc).

@kxt
Copy link
Author

kxt commented Feb 11, 2023

Then I guess this PR should be dropped and the issue postponed until it's clear what needs to be backported.

@seanmonstar
Copy link
Member

Alright, thanks so much @kxt for doing all this. Sorry for the delay, I've taken your commit and amended it with the latest changes in 1.0, and split it into 2, so we could have backports and deprecations be separate. That's in #3155 and #3156.

@seanmonstar seanmonstar closed this Mar 6, 2023
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.

Backport the split client conn modules
3 participants