directories are not included in jar #3

Closed
jussimalinen opened this Issue Oct 4, 2012 · 9 comments

Projects

None yet

2 participants

@jussimalinen

If you list a normal jar created by maven you will see directories as their own entries:

jar tf Foo.jar
META-INF/
META-INF/MANIFEST.MF

after jarjar the directories are removed:

jar tf Foo.jar
META-INF/MANIFEST.MF

This is a problem when you try to read resources from the jar. ourClassLoader.getResources("META-INF") will not find anything after jarjar, because the folder META-INF does not exist (although the files do exist... very odd situation.)

This breaks our project https://github.com/robotframework/SwingLibrary

I managed to fix this with jussimalinen/jarjar-maven-plugin@58bbb1c As IOUtil.copyZipWithoutEmptyDirectories should already strips the empty directories after the jarjar, the archiver.setIncludeEmptyDirs( false ); should not be necessary maybe?

I havent figured yet out how would I write tests for this yet, so I havent included a pull request. How would you suggest testing this, as the change is at the Mojo execution? One option would be to include maven-verifier as a test phase dependency and to execute the Mojo with it in one test.

@mcculls
Member
mcculls commented Oct 4, 2012

IMHO anything that scans jars should ideally not rely on directory entries because not every archive tool creates them. However, I understand that it make things simpler when it comes to using getResources, so I'm ok with removing the IncludeEmptyDirs setting (or at least make it configurable).

@mcculls
Member
mcculls commented Oct 4, 2012

BTW, I can't recreate the original scenario where the META-INF/ entry was missing, but META-INF/MANIFEST.MF exists.
Can you provide an example project that demonstrates the issue? I can then turn that into a regression test.

Ignore the above comment, the project I was testing locally had an additional step that ended up putting back the directory entries.

@mcculls mcculls was assigned Oct 4, 2012
@jussimalinen

Right, note that my change of removing " archiver.setIncludeEmptyDirs( false ); " will result in directories with files in them becoming available. Empty directories are still removed. This is done by IOUtil.copyZipWithoutEmptyDirectories later in the process.

So I am not sure if this really needs a config option? I mean, I can see that there could be a "includeDirectories" option, but couldn't that just be the default behaviour? I cant see why anyone would like to remove all the directories that have files in them, can it really increase jar size or cause other problems?

I suppose there could be an option to include empty dirs, but I am happy with the current behavior. (Empty dirs are always removed.)

@mcculls
Member
mcculls commented Oct 5, 2012

There is actually a difference in behaviour for some projects when "archiver.setIncludeEmptyDirs( false );" is removed, specifically those that use repackaging rules. In certain cases applying these rules to empty entries in the uber-jar will introduce bogus non-directory entries that are then not cleared up by IOUtil.copyZipWithoutEmptyDirectories.

This should be fixed by an additional check in the JarJar code to stop it from processing empty entries, but this shows that there is a difference between removing empty entries before jarjar'ing and removing them afterwards.

@jussimalinen

So not so easy then... What kind of rules can cause this?

I wrote integration tests for directories based on the way maven-verifier is used in Robot Framework Maven Plugin. You can see the results here jussimalinen/jarjar-maven-plugin@2b097ee

It is not the cleanest solution in the world. It means 2 new test phase dependencies and two new plugins for testing. Also the plugin is installed locally in pre-integration-test phase (so that it could be used by the project run by the verifier.)

Another option might be to build the integration tests based on http://maven.apache.org/plugins/maven-invoker-plugin/ I am quite new to Maven plugin development, so I am not too sure about the best practices yet...

Anyway, getting some integration test system in use would make working with the mojo easier. That would make it possible to demonstrate a problem simply by creating a sample project and a verification check that fails.

@jussimalinen

Alright, I gave maven-invoker-plugin a try and the results are quite nice I think: jussimalinen/jarjar-maven-plugin@b657ef8

With this version executing mvn integration-test will run the package target on all projects under src/it. Then verify.bsh script is run on each project to verify the results.

The verify.bsh is written on beanshell language, which was new to me, but seems easy enough. (Groovy is also supported.) There is only one extra dependency and it is very easy to create new test projects. The jarjar-maven-plugin is only installed to a temporary repository, so that local repository is not polluted when running tests. The plugin version number is also updated automatically on integration test projects.

What do you think about this approach? Sorry if I am beeing too pushy with proposing these new tests. I can understand if you dont think integration tests as necessary, but for me (as I dont know the project and all the different ways it can be used) it is rather difficult to contribute, when I dont have a tests set to verify that I am not breaking existing functionality... Just would make my life easier as a possible contributor. :)

@mcculls
Member
mcculls commented Oct 7, 2012

No worries, adding integration tests is definitely a good idea - if you create a pull-request then I'll merge it in, thanks.

@jussimalinen

Alright: #4

Note that this pull request removes ""archiver.setIncludeEmptyDirs( false );"

If you prefer a pull request without that change, let me know.

@mcculls
Member
mcculls commented Oct 9, 2012

Merged in pull-request

@mcculls mcculls closed this Oct 9, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment