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

Re-add support for tonic #54

Closed
wants to merge 3 commits into from

Conversation

sjoerdsimons
Copy link
Contributor

Now that tonic with hyper 1.0 is available re-add the support that was temporarily removed

Fixes #52

@sjoerdsimons
Copy link
Contributor Author

The CI test fails as prost has a MSRV of 1.70; I guess to solve this we can either update the MSRV of this crate to match or only support leave it 1.68 as long as tonic is not supported

Fwiw 1.70 was release in june last year, while 1.68 was march last year

@lcmgh
Copy link

lcmgh commented Aug 16, 2024

Just checked the MSRV policies of other players in the eco system.

Tokio will keep a rolling MSRV (minimum supported rust version) policy of at least 6 months. The current MSRV is 1.70

axum's MSRV is 1.66.

imho it is fair to upgrade this crates one to 1.70 (as over one year passed) but it would be higher than axum's.

@sjoerdsimons sjoerdsimons force-pushed the re-add-tonic branch 5 times, most recently from 3693d8d to 733d9c4 Compare August 16, 2024 18:47
@sjoerdsimons
Copy link
Contributor Author

Just checked the MSRV policies of other players in the eco system.

Tokio will keep a rolling MSRV (minimum supported rust version) policy of at least 6 months. The current MSRV is 1.70

axum's MSRV is 1.66.

imho it is fair to upgrade this crates one to 1.70 (as over one year passed) but it would be higher than axum's.

Actually turns out the jsonwebtoken 9.3 already needs rust 1.73 regardless... So bumped to that now and only changed CI to only check the library build (msrv of the dev-dependency is typically not relevant)

@lcmgh
Copy link

lcmgh commented Aug 19, 2024

kindly ping @cduvray for approval and release

@cduvray
Copy link
Owner

cduvray commented Aug 20, 2024

Hi,
thanks for your PR. I reverted the commit 5f71827, could you please rebase your PR on main, this will make the history more clear .

Ok for MSRV (I think I had errors even with 1.73, du to a dependency, the version 1.75 solves this), but in your commit I think no tests are run for the version MSRV is this what you want? I think tests should be run also for MSRV,

Nothing in the layer implementation actually depends on the Request's
body type. So generalise over the body type, allowing the service
implementation not longer be tied to axum specifically.
Now that tonic support has been re-added, update as required for the
new/latest version with.
The test end up depending on crates requiring more recent rust, but for
msrv really only the library itself is important. Also bump to 1.73 as
that's the minimal required for the jsonwebtoken dependency
@sjoerdsimons
Copy link
Contributor Author

Hi, thanks for your PR. I reverted the commit 5f71827, could you please rebase your PR on main, this will make the history more clear .

Done! Downside ofcourse that bisectability isn't as good but such is life :)

Ok for MSRV (I think I had errors even with 1.73, du to a dependency, the version 1.75 solves this), but in your commit I think no tests are run for the version MSRV is this what you want? I think tests should be run also for MSRV,

Yes the goal was to only build the library with MSRV to ensure it can be compiled with that rust version; The problem with also running tests on MSRV is that all the dev-dependencies then need a compatible MSRV as well, which imho is harder to maintain and less relevant for crate users.

@lcmgh
Copy link

lcmgh commented Aug 20, 2024

The revert that landed on main put axum 6 back into the game: #56

@cduvray
Copy link
Owner

cduvray commented Aug 21, 2024

I've partially merged it (to have a usable main, and to solve #56), I'm not yet sure about CI and MSRV, I have to think about it.

@sjoerdsimons
Copy link
Contributor Author

I've partially merged it (to have a usable main, and to solve #56), I'm not yet sure about CI and MSRV, I have to think about it.

Thanks! If you don't want to take these CI changes at the moment the easiest fix to get CI on main green again is likely to bump up msrv to 1.75 (not tested).

Fwiw this also shows the issue i was trying to fix as fundamentally now your MSRV is driven by your dev-dependencies instead (iirc through wiremock) rather then the actual library dependencies

@cduvray
Copy link
Owner

cduvray commented Aug 26, 2024

Hello, I've rewritten the CI bumping msrv to 75.
I will close this PR as all has been cherry picked (except for CI).

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.

Bring back tonic support
3 participants