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

COMPRESS-692: Add support for zstd compression in zip archives #634

Closed

Conversation

mehmet-karaman
Copy link

I've adjusted a few places, which were stating that the zstd method is not supported and places which return the appropriate stream for that method.

The changes in ZstdUtils could be better maybe, If someone has a hint for me I would be happy to implement this too. Thank you in advance.

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

@mehmet-karaman

Thank you for the PR.

Please see my scattered comments.

You MUST run mvn by itself before you push and address all build failures.

Also keep in mind how support for Zstd can be made in a way that also makes sense for other currently unsupported compression methods like XZ. For example would there also be another instance variable in ZipArchiveInputStream for an XZ stream? If that even makes sense...

You did not create a Draft PR, so the expectation here is that running it in the CI will produce green builds ;-)

@garydgregory
Copy link
Member

@mehmet-karaman
You want a mix of tests to make sure you can read what you write but also reading files created with official zstd utility, otherwise you risk the chance that you have the same bug in the reading and writing code.
Ty!

@mehmet-karaman
Copy link
Author

@mehmet-karaman You want a mix of tests to make sure you can read what you write but also reading files created with official zstd utility, otherwise you risk the chance that you have the same bug in the reading and writing code. Ty!

Correct me if I am wrong, but I am just testing the Zstd compression support in Zip files. I am not testing the Zstd compressor or decompressor itself. Another point is, that the zstd terminal utility is just compressing single files or streams and it doesn't have a feature for compressing zip file entries.

@mehmet-karaman mehmet-karaman force-pushed the implemented_zstd_support branch from 8177906 to 448728b Compare January 24, 2025 22:49
@mehmet-karaman mehmet-karaman marked this pull request as draft January 24, 2025 22:49
@mehmet-karaman mehmet-karaman force-pushed the implemented_zstd_support branch from 448728b to 95081b0 Compare January 24, 2025 22:58
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

@mehmet-karaman
If I run the build checks locally I get:

[INFO] --- checkstyle:3.6.0:check (default-cli) @ commons-compress ---
[INFO] There are 50 errors reported by Checkstyle 10.21.1 with /Users/garygregory/git/commons-compress/src/conf/checkstyle/checkstyle.xml ruleset.
[ERROR] src/main/java/org/apache/commons/compress/archivers/zip/ZipUtil.java:[336] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveOutputStream.java:[1081] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/main/java/org/apache/commons/compress/archivers/zip/ZipMethod.java:[144] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStream.java:[782] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStream.java:[958,11] (whitespace) WhitespaceAfter: 'else' is not followed by whitespace.
[ERROR] src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStream.java:[958,11] (whitespace) WhitespaceAround: 'else' is not followed by whitespace.
[ERROR] src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStream.java:[958,15] (whitespace) WhitespaceAround: '{' is not preceded with whitespace.
[ERROR] src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStream.java:[1363] (sizes) LineLength: Line is longer than 160 characters (found 229).
[ERROR] src/main/java/org/apache/commons/compress/compressors/zstandard/ZstdUtils.java:[41] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/main/java/org/apache/commons/compress/compressors/zstandard/ZstdUtils.java:[44,9] (modifier) RedundantModifier: Redundant 'public' modifier.
[ERROR] src/main/java/org/apache/commons/compress/compressors/zstandard/ZstdUtils.java:[53] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/main/java/org/apache/commons/compress/compressors/zstandard/ZstdUtils.java:[163] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/main/java/org/apache/commons/compress/compressors/zstandard/ZstdUtils.java:[170,37] (coding) FinalLocalVariable: Variable 'outStream' should be declared final.
[ERROR] src/main/java/org/apache/commons/compress/compressors/zstandard/ZstdUtils.java:[171,36] (coding) FinalLocalVariable: Variable 'outputStream' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[31,1] (imports) ImportOrder: 'org.apache.commons.compress.AbstractTest' should be separated from previous imports.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[37] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[40,14] (coding) FinalLocalVariable: Variable 'file' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[48,22] (coding) FinalLocalVariable: Variable 'uncompSize' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[49,24] (coding) FinalLocalVariable: Variable 'buf' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[51] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[52,24] (coding) FinalLocalVariable: Variable 'uncompData' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[57] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[60] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[63,16] (coding) FinalLocalVariable: Variable 'zipContentFile' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[64,16] (coding) FinalLocalVariable: Variable 'simpleText' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[65,14] (coding) FinalLocalVariable: Variable 'file' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[69,33] (coding) FinalLocalVariable: Variable 'archiveEntry' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[77] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[82,33] (coding) FinalLocalVariable: Variable 'entry' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[83] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[86,29] (coding) FinalLocalVariable: Variable 'inputStream' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[88] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[89,22] (coding) FinalLocalVariable: Variable 'dataOffset' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[90,21] (coding) FinalLocalVariable: Variable 'uncompressedSize' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[91] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[93] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[94,24] (coding) FinalLocalVariable: Variable 'uncompressedData' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[96] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[99] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[102,28] (coding) FinalLocalVariable: Variable 'compressedData' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[109] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[112,16] (coding) FinalLocalVariable: Variable 'zipContentFile' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[113,16] (coding) FinalLocalVariable: Variable 'simpleText' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[114,14] (coding) FinalLocalVariable: Variable 'file' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[118,33] (coding) FinalLocalVariable: Variable 'archiveEntry' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[126] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[131,33] (coding) FinalLocalVariable: Variable 'entry' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[132] (regexp) RegexpSingleline: Line has trailing spaces.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[135,29] (coding) FinalLocalVariable: Variable 'inputStream' should be declared final.
[ERROR] src/test/java/org/apache/commons/compress/archivers/zip/ZstdCompressorTest.java:[136] (regexp) RegexpSingleline: Line has trailing spaces.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  14.461 s
[INFO] Finished at: 2025-01-25T07:34:32-05:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.6.0:check (default-cli) on project commons-compress: You have 50 Checkstyle violations. -> [Help 1]

@mehmet-karaman
Copy link
Author

I've executed the mvn build according to the CONTRIBUTING.md file and it states that it was build sucessfully ("mvn clean verify") .. Now I see that the README.md file says that "mvn" has to be used for building too. :) Sorry, now i understand what you mean by mvn by itself. (without any goals)

I will fix all the problems and make a force push..

@mehmet-karaman mehmet-karaman force-pushed the implemented_zstd_support branch from 95081b0 to d880720 Compare January 27, 2025 09:01
@mehmet-karaman mehmet-karaman marked this pull request as ready for review January 27, 2025 09:02
@mehmet-karaman
Copy link
Author

I've adjusted all code style glitches remarked by the mvn build. Now I hope everything is alright with this PR :)

@garydgregory
Copy link
Member

I'll update the contributing.md file...

@mehmet-karaman
Copy link
Author

mehmet-karaman commented Jan 27, 2025

Is there anything to do in this PR? Rebase on master etc.?

@garydgregory
Copy link
Member

I will review over the next day or two.

archives, added a Testcase to check the compression / decompression,
method and the content.
@mehmet-karaman
Copy link
Author

I've found a small glitch in a comment, which could cause confusion.. I've meant outputstream but have written input stream (ZstdUtils.InnerNotClosingOutputStream the close-method).. This is adjusted and force pushed now. (mvn was executed successful)

@mehmet-karaman mehmet-karaman force-pushed the implemented_zstd_support branch from d880720 to 23acbac Compare January 30, 2025 11:21
@mehmet-karaman
Copy link
Author

@garydgregory just a friendly reminder :) do you have time to check this PR, please?

@garydgregory garydgregory changed the title COMPRESS-692: Implemented support for zstd compression in zip archives COMPRESS-692: Add support for zstd compression in zip archives Feb 3, 2025
garydgregory added a commit that referenced this pull request Feb 3, 2025
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

I pushed these changes with all the comments I made in the PR. You are credited in git master. TY!

@@ -293,6 +295,8 @@ public static boolean matches(final byte[] signature, final int length) {

private int entriesRead;

private ZstdCompressorInputStream zstdInputStream;
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed, removed in git master.

*/
public static long readAndCompressWrite(InputStream input, OutputStream output) throws IOException {
final InnerNotClosingOutputStream outStream = new InnerNotClosingOutputStream(output);
final ZstdCompressorOutputStream outputStream = new ZstdCompressorOutputStream(outStream, 3, true);
Copy link
Member

Choose a reason for hiding this comment

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

The value 3 looks like a magic number. A comment should state the reason to use 3 instead of another number.

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.

3 participants