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

Replacing hyperx's header parsing #110

Closed
b-ncMN opened this issue Sep 9, 2021 · 18 comments
Closed

Replacing hyperx's header parsing #110

b-ncMN opened this issue Sep 9, 2021 · 18 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@b-ncMN
Copy link

b-ncMN commented Sep 9, 2021

Hi,

I am trying to use this crate in my project alongside rocket-rs, only there seems to be an issue with the dependencies

stdout :     Updating crates.io index
error: failed to select a version for `httparse`.
    ... required by package `hyperx v1.3.0`
    ... which satisfies dependency `hyperx = "^1.3.0"` of package `octocrab v0.12.0`
    ... which satisfies dependency `octocrab = "^0.12.0"` of package `version-summarizer v0.1.0 (/home/infrandomness/Documents/Dev/Rust/version-summarizer)`
versions that meet the requirements `>=1.0, <1.4` are: 1.3.6, 1.3.5, 1.3.4, 1.3.3, 1.3.2, 1.3.1, 1.3.0, 1.2.5, 1.2.4, 1.2.3, 1.2.2, 1.2.1, 1.2.0, 1.1.2, 1.1.1, 1.1.0, 1.0.0

all possible versions conflict with previously selected packages.

  previously selected package `httparse v1.4.0`
    ... which satisfies dependency `httparse = "^1.4"` of package `hyper v0.14.9`
    ... which satisfies dependency `hyper = "^0.14.2"` of package `hyper-tls v0.5.0`
    ... which satisfies dependency `hyper-tls = "^0.5"` of package `reqwest v0.11.4`
    ... which satisfies dependency `reqwest = "^0.11.3"` of package `octocrab v0.12.0`
    ... which satisfies dependency `octocrab = "^0.12.0"` of package `version-summarizer v0.1.0 (/home/infrandomness/Documents/Dev/Rust/version-summarizer)`

failed to select a version for `httparse` which could resolve this conflict

This happens when I have both rocket (latest version as of today) and octocrab

@XAMPPRocky
Copy link
Owner

XAMPPRocky commented Sep 9, 2021

Thank you for your issue! I'm aware of the incompatibility, however until headers or an equivalent is more fully developed and can actually replace the hyperx's functionality in octocrab (which requires safe and correct handling of ETag, If-None-Match, and Link headers), I'd prefer to keep it.

@XAMPPRocky XAMPPRocky added bug Something isn't working help wanted Extra attention is needed labels Sep 9, 2021
@alarsyo
Copy link

alarsyo commented Sep 14, 2021

Hi! I'm hitting a similar problem.

For my use case at least, it turns out the fix would be quite simple: I'm writing a webhook server, so I'm only using octocrab for its models, for deserialization of webhook payloads.

Would you consider splitting this part into an octocrab_models crate that wouldn't depend on hyperx (I don't think this part needs it, correct me if I'm wrong!) ?

@alarsyo
Copy link

alarsyo commented Sep 14, 2021

I can write a PR, if it's something you'd consider, but don't have any time for right now 😄

@alarsyo
Copy link

alarsyo commented Sep 14, 2021

This has the side benefit of not having to build reqwest and other dependencies when only using the models in some tool

@XAMPPRocky
Copy link
Owner

Would you consider splitting this part into an octocrab_models crate that wouldn't depend on hyperx (I don't think this part needs it, correct me if I'm wrong!) ?

I don't want to split crates, as that would be extra maintenance. I like to release after every change if possible at the moment.

This has the side benefit of not having to build reqwest and other dependencies when only using the models in some tool

I'd prefer to provide this as a main benefit to octocrab itself by removing reqwest entirely. See #99

@alarsyo
Copy link

alarsyo commented Sep 14, 2021

I'd prefer to provide this as a main benefit to octocrab itself by removing reqwest entirely. See #99

That makes sense.

I'm aware of the incompatibility, however until headers or an equivalent is more fully developed and can actually replace the hyperx's functionality in octocrab (which requires safe and correct handling of ETag, If-None-Match, and Link headers), I'd prefer to keep it.

From the headers you mention, it seems like headers only lacks support for Link currently, and there's a PR for it. Hopefully that allows stripping the hyperx dependency soon enough

@chinedufn
Copy link
Contributor

chinedufn commented Oct 5, 2021

I'm in a similar boat here where using octocrab is preventing me from using another crate.

Personally, I think that is it better for the Rust ecosystem to have a separate crate with just the models, then octocrab would depend on that.
And then others could use it directly if they didn't want octocrab's request logic.

But, I understand the point about the extra maintenance. Although I suspect that for a crate that should very rarely change once it's "done" (all current GitHub APIs are supported), the maintenance burden would be negligible. Assuming the crate lived in the same Cargo workspace.


Anyway.. The PR that @alarsyo linked looks stale. So I'm not banking on only using headers being ready anytime soon.

Could we simply copy Link from hyperx into a mod temp_hyperx?
In principle this type should never change, so I think vendoring it is fine and won't introduce any additional maintenance burden.

  1. Octocrab starts working for everyone again

  2. No real maintenance burden since the hyperx types shouldn't ever need to change

  3. When headers is ready we just delete the temp_hyperx module.

Downside is a some gunk in the crate, but I think that's better than the crate simply not working for people.

@XAMPPRocky
Copy link
Owner

Anyway.. The PR that @alarsyo linked looks stale. So I'm not banking on headers being ready anytime soon.

Someone opened a new PR adding the Link header back yesterday. Good things come to those who wait. 🙂 hyperium/headers#91 So I'm going to track the progress of that PR before vendoring the code into octocrab.

I understand your frustration, however it's not fair characterisation that incompatibility with a certain framework version is "not working for everyone". Cargo has great support for git dependencies if you need it for your project sooner.

@chinedufn
Copy link
Contributor

chinedufn commented Oct 5, 2021

I'm don't use Rocket so it isn't just Rocket.

But I'm very sorry. Instead of "everyone" I should have said "people who have httpparse >1.4 in their dependency tree

I didn't mean to come off in the way that I did. I was just trying to explain my thought process and make a case for why I thought we should consider vendoring.
I should have been more conscious of how that might sound.

I'm sorry!

@XAMPPRocky XAMPPRocky changed the title Cannot use with rocket-rs Replacing hyperx's header parsing Aug 14, 2022
@lpotthast
Copy link
Contributor

Hi, it also doesn't work when using axum:

error: failed to select a version for `percent-encoding`.
    ... required by package `hyperx v1.3.0`
    ... which satisfies dependency `hyperx = "^1.3.0"` of package `octocrab v0.17.0`
    
versions that meet the requirements `>=2.1.0, <2.2` are: 2.1.0

all possible versions conflict with previously selected packages.

  previously selected package `percent-encoding v2.2.0`
    ... which satisfies dependency `percent-encoding = "^2.1"` of package `axum v0.5.16`

Kind of a deal-breaker for me sadly... I hope this can be fixed in the future. Great work btw 👍

@jplatte
Copy link

jplatte commented Oct 27, 2022

By the way, inspired by wanting to use octocrab a while ago, I created hyperium/headers#113 – I talked to Sean about it who did one review, he doesn't have time to come back to it until after Hyper 1.0 but I think after that it shouldn't take much longer anymore. So #258 still makes sense as a short-term solution if one is needed, but I hope in a month or so, this can be solved without the Link header parsing being inside this crate :)

@XAMPPRocky
Copy link
Owner

@jplatte Thank you so much for your work on this! 🙏 I'm happy to wait a little longer, glad we can soon have a implementation that benefits everyone. 🙂

@jplatte
Copy link

jplatte commented Mar 7, 2023

I think it might make sense to merge #258 as a temporary solution, the headers PR is progressing at a glacial pace 🥲

@XAMPPRocky
Copy link
Owner

@jplatte To be honest with you. I've found the situation so frustrating that I think the best solution at the moment is to fork those crates and move on from them.

I'm currently on holidays and have a busy work schedule afterwards so there will be no movement for awhile. But right now, my current thought process is that in the next few weeks I will publish a version including the fix to Octocrab, followed another version that replaces all usage of hyperium crates that aren't hyper.

I'm tired of dealing with single person maintainers of cornerstone libraries like http whose inability or refusal to handover stewardship of projects to a community of more active contributors causing downstream friction for my users.

@lswith
Copy link
Contributor

lswith commented Mar 15, 2023

We're hitting this same issue using https://github.com/kube-rs/kube:

    Updating crates.io index
error: failed to select a version for `percent-encoding`.
    ... required by package `hyperx v1.3.0`
    ... which satisfies dependency `hyperx = "^1.3.0"` of package `octocrab v0.18.1`
...
versions that meet the requirements `>=2.1.0, <2.2` are: 2.1.0

all possible versions conflict with previously selected packages.

  previously selected package `percent-encoding v2.2.0`
    ... which satisfies dependency `percent-encoding = "^2.2.0"` of package `form_urlencoded v1.1.0`
    ... which satisfies dependency `form_urlencoded = "^1.0.1"` of package `kube-core v0.76.0`
    ... which satisfies dependency `kube-core = "=0.76.0"` of package `kube v0.76.0`
    ... which satisfies dependency `kube = "^0.76.0"` of package ...

failed to select a version for `percent-encoding` which could resolve this conflict

@drshade
Copy link

drshade commented Mar 21, 2023

Same issue using AWS SDK crates, which all rely on aws-http:

error: failed to select a version for `percent-encoding`.
    ... required by package `hyperx v1.3.0`
    ... which satisfies dependency `hyperx = "^1.3.0"` of package `octocrab v0.18.1`
    ... which satisfies dependency `octocrab = "^0.18.1"` of package `[...]`
versions that meet the requirements `>=2.1.0, <2.2` are: 2.1.0

all possible versions conflict with previously selected packages.

  previously selected package `percent-encoding v2.2.0`
    ... which satisfies dependency `percent-encoding = "^2.1.0"` of package `aws-http v0.51.0`
    ... which satisfies dependency `aws-http = "^0.51.0"` of package `aws-config v0.51.0`
    ... which satisfies dependency `aws-config = "^0.51.0"` of package `[...]`

failed to select a version for `percent-encoding` which could resolve this conflict

@lpotthast
Copy link
Contributor

Sad to see that compile-breaking issues like this are kept open for months. Is there anything planned? Can we help in any way?

@XAMPPRocky
Copy link
Owner

This has been resolved in 0.19.0


Is there anything planned? Can we help in any way?

I did mention there would be a release in the comment above. As always, the easiest and most effective method of help is to sponsor me on GitHub as it directly affects how much time I can spend on personal projects like Octocrab. https://github.com/sponsors/XAMPPRocky

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

8 participants