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

AbstractZipArchiver no longer respects recompressAddedZips #53

Closed
philwebb opened this issue Nov 29, 2016 · 16 comments
Closed

AbstractZipArchiver no longer respects recompressAddedZips #53

philwebb opened this issue Nov 29, 2016 · 16 comments
Assignees
Labels
Milestone

Comments

@philwebb
Copy link

When setting AbstractZipArchiver.recompressAddedZips to false already compressed entries should be added using the ZipEntry.STORED method. Unfortunately this doesn't appear to be the case.

The following can be added to ZipArchiverTest to demonstrate the issue.

    public void testDontRecompressAddedZips() throws Exception {
         final File srcDir = new File( "src/test/resources" );
         final File zipFile = new File( "target/output/dont-recompress-added-zips.zip" );
         ZipArchiver zipArchiver = getZipArchiver( zipFile );
         zipArchiver.setRecompressAddedZips(false);
         String[] includes = { "test.zip" };
         zipArchiver.addDirectory( srcDir, includes, FileUtils.getDefaultExcludes() );
         zipArchiver.setEncoding( "UTF-8" );
         FileUtils.removePath( zipFile.getPath() );
         zipArchiver.createArchive();
         final ZipFile zf = new ZipFile( zipFile );
         assertEquals( ZipEntry.STORED, zf.getEntry("test.zip").getMethod() );
         zf.close();
	}

The root cause appears to be due to changes introduced in 1ddd40b which push the header detection logic down to wrappedRecompressor.

Unfortunately this doesn't work because ConcurrentJarCreator.addArchiveEntry stores the mode at the time that then entry was added and ignores any subsequent updates. You can see this by adding a breakpoint in the constructor of ZipArchiveEntryRequest and another in ZipArchiveEntryRequest.getMethod().

@philwebb
Copy link
Author

Incidentally this bug manifests itself in maven-assembly-plugin v3.0 and is unfortunately quite critical with the creation of Spring Boot fat jars which rely on uncompressed entries.

@plamentotev
Copy link
Member

plamentotev commented Nov 29, 2016 via email

@philwebb
Copy link
Author

@plamentotev Unfortunately I don't think the maven-assembly-plugin surfaces that as an option.

This specific requirement allows us to generate Spring Boot fat jars which are a usually a mix of .class files and nested .jar files. Only the nested jars need to be stored uncompressed and ideally we'd like to keep compression for everything else.

@plamentotev
Copy link
Member

plamentotev commented Nov 29, 2016

@philwebb It provides such option in a bit obscure way. You could set arbitrary option of the archiver using archiverConfig (the assembly plugin will set it through reflection). For example the following configuration of the maven-assembly-plugin should do the trick:

<configuration>
    <recompressZippedFiles>false</recompressZippedFiles>
    <archiverConfig>
        <compress>false</compress>
    </archiverConfig>
</configuration>

It's a bit of a dirty hack as you want any non-zip files to be compresses but I hope it could help you until a proper fix is released.

@philwebb
Copy link
Author

@plamentotev Excellent, thanks for the info!

@plamentotev
Copy link
Member

plamentotev commented Nov 30, 2016

Looks like that some changes in Apache Commons Compress are required in order to fix this issue. I've created an issue about them - https://issues.apache.org/jira/browse/COMPRESS-375

@snicoll
Copy link
Contributor

snicoll commented Dec 19, 2016

@plamentotev the commons-compress issue has been solved, awesome! Do you have a timeline for a fix here? I am asking because it will have to be incorporated in the next maven-assembly-plugin.

@plamentotev
Copy link
Member

@snicoll the fix is almost ready. It just needs a bit of polishing such as more test cases, javadocs, etc. Unfortunately I can't tell you when it could be merged into master as this depends on when the Apache Commons Compress 1.13 will be released.

My role in the Plexus Archiver project is not exactly known even to me 😄. Without going into too much details - I also don't know when a version of Plexus Archiver with the fix could be released. But I did ask about this issue on the Maven Developers list so once I receive an answer I'll let you know.

p.s. If you want you could take a look at the fix - I'm working on it on this branch. It is working but still work in progress.

@michael-o
Copy link
Member

Hi guys, as soon as anyone of you has a decent PR without any regressions, I can merge it.

@plamentotev
Copy link
Member

@michael-o thanks. I've created a PR. I don't think it's ready to be merged yet as it depends on not yet released version of Apache Commons Compress.

@michael-o
Copy link
Member

Commons Compress has been upgraded to 1.14, please revalidate this issue.

@plamentotev
Copy link
Member

It is still needed because the root cause is in Plexus Archiver. We needed newer version of Commons Compress to properly fix it. #55 fixes this issue. It's now re-based and ready to be merged.

@michael-o michael-o self-assigned this May 26, 2017
@michael-o michael-o added the bug label May 26, 2017
@michael-o michael-o added this to the 3.5 milestone May 26, 2017
@snicoll
Copy link
Contributor

snicoll commented May 29, 2017

@philwebb I don't think you've created an issue to make sure the assembly plugin upgrades to plexus archiver 3.5 so I've created MASSEMBLY-854

@snicoll
Copy link
Contributor

snicoll commented Jun 15, 2017

@michael-o @plamentotev the roadmap is complete for 3.5 and this blocks the release of Maven Assembly plugin 3.0.1. Is there a way to get going with this? Thanks!

@michael-o
Copy link
Member

I hoped that Commons Compress 1.15 will be released to be incorporated here. Will not happen I guess this months. I guess I will release without it.

@snicoll
Copy link
Contributor

snicoll commented Jun 23, 2017

@michael-o for the record I've tested the whole chain locally (including the current state of maven assembly plugin using the current state of plexus archiver) and it fixes the original issue reported here. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants