-
Notifications
You must be signed in to change notification settings - Fork 205
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
VFS-676 : change dependency from java.util.zip to Commons Compress #91
base: master
Are you sure you want to change the base?
Conversation
@PeterAlfredLee, As it stands, this PR breaks binary compatibility, so would be a -1 for 2.x. We should only break BC in a new major version. |
As the issue VFS-676 said, I'm trying to use
This PR doesn't break the existing APIs - as you can see, no existing tests need to be modified (but only to ignore 2 test cases). |
@PeterAlfredLee WRT to BC please see https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html As a reference, the current report is here: https://commons.apache.org/proper/commons-compress/japicmp.html |
Thank you for your explaining about BC, and I understand it now. :) The report is here : japicmp.zip As you can see, only some |
IMO seems the biggest bc-breaking in this pr is changing JarFileObject's super class from ZipFileObject to AbstractFileObject, thus codes like this will break:
|
Agreed. And as I said - seems we got no other options if we want to use |
We can add a different provider, then each developer can configure the provider for the jar schema as needed. |
Before someone goes that route, it seems fair to try to update to Commons Compress without breaking BC. |
Darn, I did not mean to close the PR. |
Maybe what we need is a Commons Compress provider..., then all that Commons Compress can do we can surface from a single provider. Thoughts? |
Good idea, that reminds me the good old days when I be at ms...forking a provider for odatav4 from what for odatav2. maybe we shall put it as a submodule like vfs-jackrabbit? |
gary, that was my idea so yes. since we already have multiple providers for http thats fine for compression too. It might not wortk for the util.VFSClassLoader util class as that is not related to an classloader, but this one could be changed to introspect the Jar file for signature checking (i think it had some dependency in there but i can be wrong) |
Commons-compress is already a dependency, so it would not hurt to put the alternative provider in core (in this case) |
Indeed it's a good idea. Thank you guys for your suggestions! Will update in this PR in the following days ... hoping I could finish this whthin this week.
I think it should belong VFS. |
And about the naming ... do you get some ideas?
The existing multiple http providers are http, http4, http5 which are corresponding to three different http versions. Then what should we have the name of the new provider? |
Btw, that's the reason behind read-only zip support? It that just because of zip file provider implementation limitations or there's something else? |
1955e15
to
6d2ef48
Compare
Just pushed the new provider. The scheme's name is |
This PR is to change the currently using zip dependency from
java.util.zip
toCommons Compress
. Including these changes :JarFIleObject
no longer extends fromZipFileObject
, and theJarFileSystem
no longer extends fromZipFileSystem
. Thejar
module inCommons Compress
could not achieve fit what we need in vfs, cause it did not implement some methods likegetCertificates
that are need in vfs. So I have decoupledJarFIleObject/JarFileSystem
fromZipFileObject/ZipFileSystem
, and moved the methods needed fromZipFileObject/ZipFileSystem
toJarFIleObject/JarFileSystem
. This change makesjar
andzip
two separate modules.ZipFile
inCommons Compress
open zip archivers usingFiles.newByteChannel
, while theZipFile
in 'java.util.zip' is using a native method. This means the zip archivers can be safely deleted after reading byZipFIle
- and some tests are failing now and I have ignored them, saytestCannotDeleteWhileStreaming
andtestCannotDeleteWhileStreaming2
.