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

chore: update http crate #3208

Closed
wants to merge 4 commits into from
Closed

chore: update http crate #3208

wants to merge 4 commits into from

Conversation

morenol
Copy link

@morenol morenol commented Dec 2, 2023

Depends on actix/actix-net#508

PR Type

Updates http crate

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

@robjtede
Copy link
Member

robjtede commented Dec 2, 2023

I'm curious if you have any particular motive to get this merged. From my PoV, the Actix Web project doesn't have any real need to do it.

@morenol
Copy link
Author

morenol commented Dec 2, 2023

Thanks for your feedback! I don't have any particular motive other than having that dependency up to date, so other crates that have actix-* as dependency can also update their http dependency to the latest one

@robjtede robjtede added B-semver-major breaking change requiring a major version bump A-http project: actix-http A-web project: actix-web labels Dec 6, 2023
@robjtede robjtede added this to the actix-web v5.0 milestone Dec 6, 2023
@therealprof
Copy link
Contributor

Not really sure how that happened but:

# cargo tree -i http@1.0.0
http v1.0.0
├── actix-tls v3.3.0
│   ├── actix-http v3.6.0
│   │   ├── actix-web v4.5.1
│   │   │   ├── actix-cors v0.7.0
...

whereas the rest of the actix universe is depending on 0.2. It is a bit unfortunate that one set of actix crates pulls in one dependency in two different versions.

@robjtede
Copy link
Member

robjtede commented Feb 7, 2024

Context for that particular change: actix/actix-net#508

@therealprof
Copy link
Contributor

I think switching to and using a 1.0 version is always a good venture due to the implied stability guarantees.

@juchiast
Copy link

Is there anything blocking this PR?

@juchiast
Copy link

I'm curious if you have any particular motive to get this merged. From my PoV, the Actix Web project doesn't have any real need to do it.

One use case is passing header values extracted from actix's request to other libraries, such as reqwest 0.12 .
Because of incompatible http version, we have to convert HeaderValue to bytes before passing.

@linuxtim
Copy link

linuxtim commented Apr 2, 2024

I'm curious if you have any particular motive to get this merged. From my PoV, the Actix Web project doesn't have any real need to do it.

Inevitably security support for http 0.2.x will cease long before security support for http 1.x does.

@SleeplessOne1917
Copy link
Contributor

If I understand correctly from the milestone, the maintainers are waiting for the v5.0 release to merge this? Hopefully this is done sooner than later, as there is 0 reason for any actively maintained library to still be using http version 0.2.x.

@robjtede
Copy link
Member

There's no security risk of staying on v0.2. It's my opinion that it's not worth a breaking change just for this. The ecosystem churn of releasing Actix Web v5.0 is vast; far outweighing any potential benefits of sticking with this older version until enough good reasons exist to break everyone.

@SleeplessOne1917
Copy link
Contributor

I can see holding this off until the next major version bump since it is, as you said, a breaking change. The thing that drew me to this issue is the one @juchiast mentioned: I'm unable to upgrade the reqwest dependency in one of my projects to 0.12.x without doing a bunch of workarounds. As the documentation for the crate says says:

It’s intended that this crate is the “standard library” for HTTP clients and servers without dictating any particular implementation.

With the crate's status as standard to be used by client and server crates, and with many of those crates updating to use the 1.x version, the incompatibility issues will only increase as time goes on.

@robjtede
Copy link
Member

robjtede commented May 30, 2024

Sorry, but compatibility with the hyper ecosystem isn't one of my concerns.

I disagree that http, despite its name, should be considered as highly as "the standard library" for HTTP in Rust. Also FWIW, while this upgrade is one option for the v5 release, it's also possible I'll move Actix Web off that crate entirely.

@asonix
Copy link
Contributor

asonix commented May 30, 2024

fwiw i already did the reqwest 0.12 upgrade in pict-rs and the only thing I needed to add was a conversion between http1 and http02 status codes, which I did with this code:

pub(crate) fn to_actix_status(status: reqwest::StatusCode) -> actix_web::http::StatusCode {
    actix_web::http::StatusCode::from_u16(status.as_u16()).expect("status codes are always valid")
}

@Thomasdezeeuw
Copy link
Contributor

I'm a little late to discussion.

I'm curious if you have any particular motive to get this merged. From my PoV, the Actix Web project doesn't have any real need to do it.

From a p.o.v. of Actix (maintainer) indeed there might not be much motivation. From an Actix user however there are. Currently we have two versions of the same crate in our dependency tree due to Actix, the leads to (among other this) larder binary and increased build time. This also leads to slight incompatibility between since as highlighted by @asonix (#3208 (comment)).

There's no security risk of staying on v0.2. It's my opinion that it's not worth a breaking change just for this. The ecosystem churn of releasing Actix Web v5.0 is vast; far outweighing any potential benefits of sticking with this older version until enough good reasons exist to break everyone.

This is totally a fair point of view. And thank you for not creating churn for no reason (it's one of my pet peeves).

Also a note on how this pr is implemented; it won't work. If a single dependency decides to enable the http-1 feature it will enable it for all, meaning that it will create a mess of incompatible crates and crate versions. If the switch to http v1 is made it should not be optional or dependent on a feature flag.

@morenol morenol closed this by deleting the head repository Jun 8, 2024
@robjtede robjtede linked an issue Jun 8, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http project: actix-http A-web project: actix-web B-semver-major breaking change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating to HTTP crate v1
8 participants