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

Replace blaze-builder with bsb-http-chunked #109

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sjakobi
Copy link
Contributor

@sjakobi sjakobi commented Mar 12, 2018

I have extracted Blaze.ByteString.Builder.HTTP into a new package.

import System.IO.Streams (InputStream, OutputStream)
import qualified System.IO.Streams as Streams
import System.IO.Streams.Attoparsec (parseFromStream)
import Data.ByteString.Builder.HTTP.Chunked (chunkedTransferEncoding, chunkedTransferTerminator)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change.

@sjakobi
Copy link
Contributor Author

sjakobi commented Mar 13, 2018

The Travis failure seems to be due to threads requiring base < 4.11.

@mightybyte
Copy link
Member

Yeah, I already opened this PR. basvandijk/threads#15

@gregorycollins Thoughts?

@mightybyte
Copy link
Member

@sjakobi Hmmm, when testing with GHC 8.4.1 cabal new-build --allow-newer works on your branch but cabal new-test --allow-newer fails. It looks like this is because the test suite depends on http-streams which depends on http-common which depends on blaze-builder. So we need to fix more than just the threads package.

@sjakobi
Copy link
Contributor Author

sjakobi commented Mar 13, 2018

Oof!

For http-streams, I have already opened aesiniath/http-streams#108. I'll wait for any feedback there until (maybe) I do more work.

http-common looks easy to fix but as with http-streams it's probably better to use a faster strict bytestring builder. I have opened aesiniath/http-common#14

@mightybyte
Copy link
Member

Thanks for doing this work BTW. I was recently thinking that we should probably make the switch from blaze-builder to bytestring. Any chance you can modify bsb-http-chunked to not depend on bytestring-builder any more? It seems like it's mainly a shim package.

@sjakobi
Copy link
Contributor Author

sjakobi commented Mar 13, 2018

Any chance you can modify bsb-http-chunked to not depend on bytestring-builder any more? It seems like it's mainly a shim package.

Can you explain why that's important to you?

I use bytestring-builder only to stay compatible with GHC < 7.8. I can hide the dependency again for recent GHCs like this if you prefer.

@mightybyte
Copy link
Member

mightybyte commented Mar 13, 2018

Well, in general I like to minimize dependencies. I was thinking it wasn't buying us enough, but if it's buying us compatibility with < 7.8, then I think you're right, we should keep it for awhile longer. With snap we have a strong commitment to backwards compatibility. I just ditched 7.4 compatibility for snap-server rather reluctantly, so yeah...let's keep 7.6 compatibility for awhile.

@sjakobi
Copy link
Contributor Author

sjakobi commented Mar 13, 2018

I just ditched 7.4 compatibility for snap-server

Aren't you going to adjust your lower bound on base then?

@mightybyte
Copy link
Member

Oh, good point. I had removed the Travis test, but not bumped the lower bound. Just did that.

@mightybyte
Copy link
Member

mightybyte commented Mar 14, 2018

@sjakobi It looks like your merge has an error in the cabal file.

(The master branch is now building with GHC 8.4 btw.)

@sjakobi
Copy link
Contributor Author

sjakobi commented Mar 14, 2018

I shouldn't have used the GitHub interface to fix the merge conflict… :)

BTW, now that blaze-builder has a new release, I leave it up to you whether you still want to do the switch. I do intend to modernize the internals and maybe speed up chunkedTransferEncoding a bit but that hasn't happened yet.

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.

2 participants