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

Remove jzlib dependency #772

Merged
merged 2 commits into from
Feb 11, 2023
Merged

Remove jzlib dependency #772

merged 2 commits into from
Feb 11, 2023

Conversation

p120ph37
Copy link
Contributor

@p120ph37 p120ph37 commented Mar 8, 2022

The JRE already contains a gzip implementation in java.util.zip.*, so we should be able to use that rather than relying on the external jCraft library.

@p120ph37 p120ph37 requested a review from hierynomus as a code owner March 8, 2022 02:51
@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2022

Codecov Report

Merging #772 (ec74015) into master (dcfa183) will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master     #772      +/-   ##
============================================
+ Coverage     64.88%   64.89%   +0.01%     
+ Complexity     1469     1465       -4     
============================================
  Files           210      210              
  Lines          8537     8523      -14     
  Branches        781      780       -1     
============================================
- Hits           5539     5531       -8     
+ Misses         2591     2582       -9     
- Partials        407      410       +3     
Impacted Files Coverage Δ
...zz/sshj/transport/compression/ZlibCompression.java 0.00% <0.00%> (ø)
...net/schmizz/sshj/transport/TransportException.java 44.44% <0.00%> (-11.12%) ⬇️
...ain/java/net/schmizz/sshj/common/SSHException.java 67.74% <0.00%> (-6.46%) ⬇️
src/main/java/net/schmizz/sshj/SSHClient.java 58.52% <0.00%> (-1.14%) ⬇️
...java/net/schmizz/sshj/transport/TransportImpl.java 64.87% <0.00%> (-0.83%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hierynomus
Copy link
Owner

Unfortunately I do not think this will work. Zip is different from zlib and both algorithms are unable to read eachother's data.

@p120ph37
Copy link
Contributor Author

p120ph37 commented Mar 8, 2022

Yes, a PKZip file is a different format from a Gzip file. But technically both use the "deflate" algorithm internally. Long ago, the JRE didn't contain tools for dealing directly with gzip/zlib style "deflate" streams (Java of course has supported access to PKZIP files since the beginning, since that's what .JARs are). As of Java 6 or so, (apparently as part of adding PNG support), classes were made available inside java.util.zip for handling "deflate" streams in a general-purpose way, compatible with gzip/zlib. See the docs here relating to the specific classes I used: https://docs.oracle.com/javase/6/docs/api/index.html?java/util/zip/package-summary.html
Note it says "compression using the popular ZLIB compression library".

Additionally, I have tested this myself against an OpenSSH Docker container by doing this to force the use of my Zlib implementation (note the absence of the "NoneCompression.Factory"):

ssh.getTransport().getConfig().setCompressionFactories(Arrays.asList(
    new DelayedZlibCompression.Factory()
));

Give it a try -- it works for me 😁

@p120ph37
Copy link
Contributor Author

I dug into the history a little more - it actually looks like zlib deflate/inflate support has been available in java.util.zip since all the way back in Java 1.1, when the JAR format was first introduced!
https://www.cs.princeton.edu/courses/archive/fall97/cs461/jdkdocs/api/java.util.zip.Deflater.html

The only reason the jCraft jzlib project was even created was because the JRE implementation didn't yet support partial-flush mode, as explained here:
http://www.jcraft.com/jzlib/

This deficiency was eventually fixed in Java 7 with the introduction of the 4-argument deflate(byte[] b, int length, int off, int flush), allowing the use of the SYNC_FLUSH flag:
https://docs.oracle.com/javase/7/docs/api/java/util/zip/Deflater.html#deflate(byte[],%20int,%20int,%20int)

The Java bug report about this is here (and confirms that it was fixed as of JDK 7):
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=4206909

So I suppose if you are intending to still support Java 6 for now, maybe you can't merge this yet, but as soon as you are targeting Java 7 and above, this PR should be usable.

@hpoettker
Copy link
Contributor

Is there a chance that this PR moves forward?
I think that would be great! Just like jsch, jzlib seems to be unmaintained. And if jzlib can even be replaced by JDK internals, I think it's a good idea to do so.

Apache Mina Sshd did the same a while ago: apache/mina-sshd@ea72a9e

@p120ph37
Copy link
Contributor Author

It looks like Java 7 may have already been a requirement since this commit: 3526694#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R88

So I'm not aware of any reason this shouldn't be mergeable at this point. @hierynomus , do you have any remaining concerns?

@hierynomus hierynomus merged commit 154a202 into hierynomus:master Feb 11, 2023
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.

4 participants