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(server): deprecate server conn structs #3161

Merged

Conversation

oddgrd
Copy link
Contributor

@oddgrd oddgrd commented Mar 7, 2023

This is based on #3102 which needs to be merged before this.

I had some difficulty with deprecations inside the pin-project-lite macros, it doesn't seem to be possible to set #[cfg_attr(feature = "deprecated", allow(deprecated))]on the fields of a struct inside pin_project!. That server::conn::Connection is inside pin_project! makes it difficult to deprecate as well. 🤔 I'll need to come back to this tomorrow. For example, in hyper/server/server.rs:

pin_project! {
    /// A listening HTTP server that accepts connections in both HTTP1 and HTTP2 by default.
    ///
    /// `Server` is a `Future` mapping a bound listener with a set of service
    /// handlers. It is built using the [`Builder`](Builder), and the future
    /// completes when the server has been shutdown. It should be run by an
    /// `Executor`.
    pub struct Server<I, S, E = Exec> {
        #[pin]
        incoming: I,
        make_service: S,
        #[cfg_attr(feature = "deprecated", allow(deprecated))] // <- This doesn't work.
        protocol: Http_<E>,
    }
}

@seanmonstar
Copy link
Member

What if you just add the allow on struct Server, instead? Seems fine, since it's only allowing when our own feature is enabled, normal CI will catch any other deprecated stuff we're using from libstd.

@oddgrd oddgrd force-pushed the feat/deprecate-server-conn-structs branch from 4256e85 to 1bf6bee Compare March 9, 2023 14:41
@oddgrd
Copy link
Contributor Author

oddgrd commented Mar 9, 2023

CI passes but it's still showing warnings with the deprecated feature enabled and the allow on the struct itself, unfortunately.

@seanmonstar
Copy link
Member

That seems weird... (Also, does that mean we need an additional CI job to notice that?)

@seanmonstar
Copy link
Member

I'll make a PR to add the CI job.

@seanmonstar
Copy link
Member

Ok, added in #3162.

@oddgrd
Copy link
Contributor Author

oddgrd commented Mar 9, 2023

Yes, I am very confused about this. Here is a cargo check output:

cargo check --features deprecated,full
    Checking hyper v0.14.24 (/home/oddgrd/dev/hyper)
warning: use of deprecated struct `server::conn::Http`: This struct will be replaced with `server::conn::http1::Builder` and `server::conn::http2::Builder` in 1.0, enable the "backports" feature to use them now.
  --> src/server/server.rs:42:19
   |
42 |         protocol: Http_<E>,
   |                   ^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

warning: use of deprecated struct `server::conn::Http`: This struct will be replaced with `server::conn::http1::Builder` and `server::conn::http2::Builder` in 1.0, enable the "backports" feature to use them now.
   --> src/server/server.rs:786:19
    |
786 |         protocol: Http_<E>,
    |                   ^^^^^

warning: `hyper` (lib) generated 2 warnings
    Finished dev [unoptimized + debuginfo] target(s) in 0.73s

@seanmonstar seanmonstar merged commit 02fe20f into hyperium:0.14.x Mar 9, 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.

2 participants