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

bhttp 0.5.2 contains a breaking change for MSRV 1.63.0 #69

Open
DanGould opened this issue Jun 25, 2024 · 3 comments
Open

bhttp 0.5.2 contains a breaking change for MSRV 1.63.0 #69

DanGould opened this issue Jun 25, 2024 · 3 comments

Comments

@DanGould
Copy link
Contributor

d5543a3 included in bhttp 0.5.2 has bhttp::ControlData::status() return the new StatusCode type. Previously StatusCode was an alias for u16, and now you need to call StatusCode::code() to get the u16 out of it.

Since this was a patch bump this broke CI and broke builds downstream without pinning the old dependency or updating the source to call StatusCode::code(). So this change will break my old releases for MSRV 1.63.0 without pinning.

I'm not sure why exactly newer versions can do the conversion and the older version can't. Perhaps newer versions of rust are smart enough to deref the inner value but 1.63 is not?

Not a huge deal since this is v0.* software. I figured you'd like to know anyhow. Thanks for maintaining ohttp & bhttp.

@DanGould
Copy link
Contributor Author

DanGould commented Jun 25, 2024

Another odd break is that bhttp::Message::response(status: StatusCode) used to be callable downstream because StatusCode was just a u16 alias, but now bhttp::StatusCode can only be created by calling try_from(value: u64) since its inner u16 is a private tuple member. This seems odd to me. Why u64?

bhttp::Message::response(bhttp::StatusCode::try_from(parts.status.as_u16() as u64)?) is a very odd way to ohttp encapsulate an response made with hyper. Especially compared to the previous bhttp::Message::response(parts.status.as_u16()). Perhaps you intended the library to be used some other way?

DanGould added a commit to DanGould/rust-payjoin that referenced this issue Jun 25, 2024
bhttp 0.5.2 broke MSRV 1.63.0

See: martinthomson/ohttp#69
DanGould added a commit to payjoin/rust-payjoin that referenced this issue Jun 25, 2024
bhttp 0.5.2 broke MSRV 1.63.0. I don't think just pinning in the README
wouldn't fix this because 0.5.2 contains breaking changes.

See: martinthomson/ohttp#69

I noticed all of our CI jobs were breaking and I think this should fix
it. I took this opportunity to bring `payjoin-diretory`, the OHTTP
Gateway, up to date with the same versions of ohttp and bhttp as
`payjoin` too.
@martinthomson
Copy link
Owner

Ugh, my bad. I missed that I had those changes staged. It should have been an 0.6 release for the change though.

The intent was to make StatusCode safe by construction, so that you could construct correct responses with more direct feedback. That should have included a TryFrom<u16> implementation at a minimum (the TryFrom<u64> is an implementation detail, not something that should have been exposed like that.

There will be changes for you, which I guess are unavoidable now in any case (sorry again). You'll need to call Message::response(StatusCode::try_from(parts.status.as_u16())?). With future changes (see #67), that might reduce to something nicer, like Message::response(response.status()) if we teach this code to understand hyper status codes. That's a future project though.

For now, I'm adding the TryFrom<u16> and impl From<StatusCode> for u16, which should smooth the bumps a little better.

@DanGould
Copy link
Contributor Author

In the future if a patch break is found it'd be nice to have that yanked and replaced with a minor version bump. But like I said, not a huge deal. Sounds like you intended an 0.6 release soon enough.

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

No branches or pull requests

2 participants