search

ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
892 stars 232 forks source link

1.0.0 Release #137

Closed scohen closed 6 years ago

scohen commented 6 years ago

With the exception of cbbb4d5523f8738b237593f9516c3a237d0cc2f4, the master version of gun at bda5630b8770da0f6e99f1617777f3c37fca957c is a suitable 1.0.0 release that's compatible with Cowboy 2.1, Cowlib 2.0 and Ranch 1.4.

Please release this version so people can use this excellent HTTP library with Cowboy and Ranch.

essen commented 6 years ago

Yes eventually when the stars align and I got time.

scohen commented 6 years ago

@essen I think you're there already. I'd help if I could, but you're the one in charge of that. Tagging something like https://github.com/discordapp/gun/commit/da4fb20c67d77ecb2307faf06cf69a80dbeaf029 as 1.0.0-pre.5 would be a tremendous help.

essen commented 6 years ago

Just depend on the commit for now, I'm not sure what's the problem. There's plenty of things left to do before an 1.0.

scohen commented 6 years ago

The problem is your later commits break backwards compatibility with released versions of cowlib, cowboy and ranch. Maintaining a fork like this is problematic because it diverges from the mainline development of gun.

essen commented 6 years ago

Show me a patch of what you think should be done, because I don't get what difference it would make.

scohen commented 6 years ago

@essen That's literally your current master with git revert cbbb4d5 applied. Commit cbbb4d5, which adds support for chunked trailers needs cowlib > https://github.com/ninenines/cowlib/commit/7728c624028db160edad2646f34d069d16af0869 to work with gun's master.

The problem I was seeing is that the commit changes the return value of stream_chunked and gun then fails with a match error on https://github.com/ninenines/gun/blob/b297499e13ce24806cc354ea601292b30cbb979f/src/gun_http.erl#L141

Maybe this isn't a problem if cowboy 2.1 and ranch 1.4 are compatible with cowlib 2.1. However, their rebar.config and erlang.mk say they're not.

essen commented 6 years ago

They are compatible. Even Gun should be, if not it's a bug. Gun shouldn't do anything with trailers if the client didn't send a te: trailers header.

scohen commented 6 years ago

Yeah, I saw that. Maybe I got spooked by the dependencies in the project, I might be able to force-override them. I can update and retry my tests.

Maybe the fix is to just relax Cowboy and Ranch's deps cowlib deps? Or maybe just ignore them in downstream projects (though that doesn't sound great).

So you know, we're using Gun to implement a Hackney replacement, and want to release it as open source once it's proven. Getting these deps right will put people's minds at ease.

essen commented 6 years ago

I plan to remove the dependency on Ranch and I suppose the Cowlib dependency can be pinned now.