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

incorporate netty fixes and publish to maven central #8

Closed
carrot-garden opened this issue Feb 1, 2013 · 10 comments
Closed

incorporate netty fixes and publish to maven central #8

carrot-garden opened this issue Feb 1, 2013 · 10 comments

Comments

@carrot-garden
Copy link

Atsuhiko Yamanaka:

  1. currently netty uses custom build of jzlib with some fixes:
    https://github.com/netty/netty/tree/master/common/src/main/java/io/netty/util/internal/jzlib

  2. do you think you will be able to accept pull request to incorporate these fixes
    and release updated jzlib to maven central?

thank you.

Andrei.

@ymnk
Copy link
Owner

ymnk commented Feb 5, 2013

Sorry for my delay.
Before making a response, I had needed to understand the changes, but it was not so trivial and not so easy for me. It seems the changes are based on the older jzlib vesion and include many not so essential changes.

Anyway, from my understandings, I guess the essential changes by netty is the introduction of JZlib.W_ZLIB_OR_NONE. The original jzlib needs an explicit parameter(nowrap; true or not) to inflate data, but the advantage of netty's jzlib is that it can handle any deflated data gracefully with JZlib.W_ZLIB_OR_NONE. Is my guess right?

I have implemented a similar functionality. Please refer to a commit 99d6334 and incorporate_netty_fixes ,
and check tests out for how-to use it,
https://github.com/ymnk/jzlib/blob/incorporate_netty_fixes/src/test/scala/WrapperTypeTest.scala

@ymnk
Copy link
Owner

ymnk commented Feb 5, 2013

Oops, I forget to leave a comment for the second question,

  1. do you think you will be able to accept pull request to incorporate these fixes
    and release updated jzlib to maven central?

Yes, it will be deployed to the maven central repo, if we get the mutually acceptable version for you and us.

@trustin
Copy link

trustin commented Feb 5, 2013

@ymnk's change looks fine. Thanks for porting it. Sorry that I reformatted the code way too much.

@ymnk
Copy link
Owner

ymnk commented Feb 5, 2013

> @ymnk's change looks fine. Thanks for porting it. Sorry that I reformatted the code way too much.

No, I have understood the reason for changes to fix redundancies and wrong formats, etc. They are right changes.
After netty is ready for switching to the updated jzlib, I'll also apply such re-formating and refactoring.

@normanmaurer
Copy link

@trustin what you think about switch to it for netty 4 now ?

@trustin
Copy link

trustin commented Feb 5, 2013

@normanmaurer, I'm fine with that. :-)

@carrot-garden
Copy link
Author

@ymnk thank you for accepting this.

@trustin do we have any jzlib unit tests that should be contributed here?

@normanmaurer
Copy link

@ymnk did you release it yet ?

@ymnk
Copy link
Owner

ymnk commented Feb 18, 2013

Not yet. We have been wanting for the success of tests by netty, commented at netty/netty#1012 (comment)

@ymnk
Copy link
Owner

ymnk commented Feb 20, 2013

It has been released as https://github.com/ymnk/jzlib/tree/1.1.2 , and deployed to the maven central repository.

@ymnk ymnk closed this as completed Feb 20, 2013
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

3 participants