Fix setRecompressAddedZips has no effect on the resulting archive

[plamentotev]

PLEASE NOTE: This merge request introduces SNAPSHOT dependency.

This merge request update the version of Apache Commons Compress
from 1.11 to 1.13. 1.12 introduces backward incompatible changes - BZip2CompressorOutputStream no longer tries to finish the output stream in finalize. This is not a problem because we properly close all BZip2CompressorOutputStream and we don't rely on this behavior anyway. Other notable change is that 1.13 requires Java 7 at run-time, but as far I know so does Plexus Archiver.

If you think this fix is ok I could backport it for Plexus Archiver 2.x

Follows the commit message. I think it pretty much describes the changes.

To check if a given entry is zip file or not the first four bit
should be read. There is an issue when entries are being added
asynchronously. Because it's faster to submit entries
than to compress them, input streams are opened faster than
they are closed. Eventually this could lead to too many
open files error.

There was a workaround for this issue. Wrapper around the
InputStreamSupplier so the first four bytes are read
when the entry is compressed and not when it's submitted.
That solves the too many open files problem,
but unfortunately has no effect on the resulting archive.
The compression method is determined when the entry is
submitted so changing it afterwards has no effect.

Use the newly introduced ZipArchiveEntryRequestSupplier
to supply the whole ZipArchiveEntryRequest
(including the compression method) when the entry is
compressed.

Upgrade Apache Commons Compress as the fix relies on
feature introduced in 1.13.

Closes #53

[plamentotev]

@michael-o since Commons Compress is released could you please take a look? Also is the 2.x branch supported. Looks like the recent releases was not back-ported.

[michael-o]

Just read the PR message and the diff. Here are my comments:

This merge request update the version of Apache Commons Compress
from 1.11 to 1.13. 1.12 introduces backward incompatible changes - BZip2CompressorOutputStream no longer tries to finish the output stream in finalize. This is not a problem because we properly close all BZip2CompressorOutputStream and we don't rely on this behavior anyway.

This is unrelated to this change, isn't it? If so, please factor out in a separate PR. I am not really familiar with the code, so I'd like to see separate changes.

Other notable change is that 1.13 requires Java 7 at run-time, but as far I know so does Plexus Archiver.

Really odd, have a look at this: https://github.com/codehaus-plexus/plexus-archiver/blob/master/pom.xml#L158-L171
This needs to be fixed first.

If you think this fix is ok I could backport it for Plexus Archiver 2.x

No need to waste your time, it hasn't been touched for 1,5 years. Maven Archiver uses Plexus Archiver 3.x.

As for the rest, I will take a look a again as soon as the above has been factored out/addressed.

[plamentotev]

Hi,

Thanks for the comments.

This merge request update the version of Apache Commons Compress
from 1.11 to 1.13. 1.12 introduces backward incompatible changes - BZip2CompressorOutputStream no longer tries to finish the output stream in finalize. This is not a problem because we properly close all BZip2CompressorOutputStream and we don't rely on this behavior anyway.

This is unrelated to this change, isn't it? If so, please factor out in a separate PR. I am not really familiar with the code, so I'd like to see separate changes.

Common Compress 1.13 is required. Apart from upgrading the artifact versions there are no changes related to the upgrade (that is what I was trying to explain in the paragraph). But I suppose it is still beneficial to separate the upgrade in another commit/PR.

Really odd, have a look at this: https://github.com/codehaus-plexus/plexus-archiver/blob/master/pom.xml#L158-L171
This needs to be fixed first.

I did notice that and thought it's left on purpose. So you suggest that first I should try to compile it with Java 1.7?

[michael-o]

Am 2017-01-11 um 18:44 schrieb Plamen Totev:

Hi, Thanks for the comments. >> This merge request update the version of Apache Commons Compress >> from 1.11 to 1.13. 1.12 introduces backward incompatible changes >> - BZip2CompressorOutputStream no longer tries to finish the >> output stream in finalize. This is not a problem because we >> properly close all BZip2CompressorOutputStream and we don't rely >> on this behavior anyway. > This is unrelated to this change, isn't it? If so, please factor > out in a separate PR. I am not really familiar with the code, so > I'd like to see separate changes. Common Compress 1.13 is required. Apart from upgrading the artifact versions there are no changes related to the upgrade (that is what I was trying to explain in the paragraph). But I suppose it is still beneficial to separate the upgrade in another commit/PR.

You comment wasn't completely clear, that's why I have asked. If the upgrade itself does not require code changes, then it is fine, if it does, I'd like to see a new PR for that first. As far as I understood your comment, it is a separate change for the BZip2CompressorOutputStream necessary when an upgrade of Commons Compress is performed.

> Really odd, have a look at this: > https://github.com/codehaus-plexus/plexus-archiver/blob/master/pom.xml#L158-L171 >This needs to be fixed first. I did notice that and thought it's left on purpose. So you suggest that first I should try to compile it with Java 1.7?

Absolutely, it does not make sense otherwise. The problem is that Maven Archiver and Maven Assembly Plugin still require Java 6. I do not know yet how this can be reasonable reconciled. This would actually kill every user below Maven 3.3.x because this requires 7, everything below is still 6. Moreover, I checked git log of 1.12...1.13, I am not really convinced. They have raised Java requirements in two consecutive releases within less than six months just for try-with-resource clauses. I think, this impact is too huge given that the community is quite big. Imho, this has to be reverted.

[plamentotev]

Actually Plexus Archiver and Plexus IO do require Java 7 because they use functionality related to file attributes introduced in Java 7. They should work with Java 6. On run time they are supposed to check the JRE version and if it's bellow Java 7 they should not use those functionalities. Unfortunately not all methods do that check - for example PlexusIoBzip2ResourceCollection#getAttributes do not and will fail under Java 6.

So in short: Plexus Archiver requires Java 7 on build time and it's supposed to work with Java 6 on run-time but actually some methods will fail.

[michael-o]

@plamentotev You are talking about reflection-only code, right? Commons Compress 1.13 uses try-with-resources which is a hard requirement on Java 7. This is not the case with Plexus Archiver.

@hboutemy @stephenc @khmarbaise Is it OK to raise minimum level to Java 7 becuase Commons Compress 1.13 now requires Java 7? The consequences are quite high for Maven Archiver and all depending plugins.

[plamentotev]

@plamentotev You are talking about reflection-only code, right? Commons Compress 1.13 uses try-with-resources which is a hard requirement on Java 7. This is not the case with Plexus Archiver.

No, it's not reflection-only code. Take a look at Plexus IO Java7FileAttributes - https://github.com/codehaus-plexus/plexus-io/blob/master/src/main/java/org/codehaus/plexus/components/io/attributes/Java7FileAttributes.java#L63. In the constructor of the class there is the following line:

Path path = file.toPath();

File#toPath is introduced in Java 7 so any attempt to make instance of Java7FileAttributes using Java 6 will end up with NoSuchMethodError. Unfortunately Plexus Archiver does make instances of Java7FileAttributes and as a result some the the functionality will fail if ran on Java 6. For example take a look at PlexusIoBzip2ResourceCollection#getAttributes:

    @Override protected PlexusIoResourceAttributes getAttributes( File file ) throws IOException
    {
        return new Java7FileAttributes( file, new HashMap<Integer, String>(), new HashMap<Integer, String>() );
    }

this code will not work on Java 6. When getAttributes is called? take a look at the following code. It will not work on java 6:

        final File zipFile = new File( "target/output/pom.zip" );
        ZipArchiver zipArchiver = (ZipArchiver) lookup( Archiver.ROLE, "zip" );
        zipArchiver.setDestFile( zipFile );
        zipArchiver.addArchivedFileSet( bz2File, "prfx/" );
        FileUtils.removePath( zipFile.getPath() );
        zipArchiver.createArchive();

So some of the Plexus Archiver functionality does not work on Java 6.

[michael-o]

I finally understand what you are talking about. Hell, this is so ugly. The class version of Plexus IO is still 49 because the compiler does not check wether employed classes/methods are available in the target Java version at all, but finally fixed in Java 9. That is why Plexus IO and Archiver compile on a lower Java version, they simply don't use new features on a byte code level. Unfortunately, Commons Compress sets both source and target for 1.7 which produces class version 51 and it will fail on a Java 6 VM when run. This is the problem I have. Am I wrong here?

[michael-o]

@hboutemy @stephenc @khmarbaise Pinging again. What you do think about it?

[plamentotev]

Another options is to create a class that does the same thing as ParallelScatterZipCreator from Common Compress 1.13. This way we can avoid requiring Java 7 at the expense of supporting additional class. As a matter of fact ParallelScatterZipCreator is not complex class.

[plamentotev]

@michael-o @hboutemy @stephenc @khmarbaise excuse me for pinging again, but almost two month have passed. I guess it's safe to assume that upgrading to Java 7 is not acceptable. So maybe we should propose common-compress to downgrade to Java 6 or to develop a solution that does not require an upgrade to common-compress 1.13? What do you think?

[olamy]

In 2017 having 1.6 as required version...
Seriously???

[michael-o]

@olamy Look at the entire chain, it has a baseline of Java 6. That's why I have asked other folks who did not react. I am equal to raise to Java 7, others maybe don't.

[michael-o]

Since no one really objected, I will go on next week with the update to Java 7 and more

[plamentotev]

@michael-o great. Tell me if I could help. I think that plexus-io should be updated too and all the code that is required for it to work on Java 6 removed. For example Java7Reflector.

[snicoll]

Can we get a status of this PR please? That's blocking us (Spring Boot) to upgrade the maven assembly plugin. Thanks!

[michael-o]

@snicoll I have received no objections on this on the dev@maven mailing list. It is safe to merge now from my point of view.

[snicoll]

@michael-o cool, thanks. When can we expect a plexus-archiver release with the fix?

[michael-o]

@plamentotev Your branch has four subsequent commits. Please clarify!

[plamentotev]

@michael-o the branch you are refering to is obsolete, you can ignore it. Sorry for the confusion, I'm going to delete it. I think it would be better to update the Plexus IO dependency(once released) before merging this PR. I have a brach at the ready, once Plexus IO is released I'll create PR for that too.

…

On May 9, 2017 11:37, "Michael Osipov" ***@***.***> wrote: @plamentotev <https://github.com/plamentotev> Your branch has four subsequent commits. Please clarify! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#55 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMLobYEUIVcfnNbHgQCtp4rjaQDgrE98ks5r4CXigaJpZM4LXwA2> .

[michael-o]

Plexus IO has been upgraded, you can commence your work. I'll release Plexus IO this week.

[michael-o]

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

[plamentotev]

It is still needed because the root cause is in Plexus Archiver. We needed newer version of Commons Compress to properly fix it.