Maven Puts Arbitrary Extension Definition in JAR Manifest by Default.

This issue is related to MJAR-39 and MWAR-34 in that since it sets the Specification-Title element by default to the POM's <description> element, it easily can create an invalid Manifest.

I should have marked this as a Blocker since there is no way to prevent Maven from merging these attributes into the final Manifest.

I downloaded SVN in an attempt to make a patch for these issues, but I had problems with the server and cannot afford the needed time to make a fix.

I'll attach three files, which together build a proposed patch. As the patch is comparatively large (although technically there isn't too much happening), it seemed wise to use three patch files, each of which introduce another change.

The patch doesn't attempt to really fix the problem: Fixing it doesn't look so trivial, at least not without taking decisions which are beyond a mere contributors scope. Instead the patch introduces a possible workaround, which seems reasonably conservative.

The patchs idea is as follows: Currently it is not possible to specify values like "Extension-Name", and the like, besides the automatically choosen values. If you do, then an exception is raised, which tells you, that you cannot overwrite a manifest entry. Obviously, this is not desirable. So it seems reasonable to specify, that explicitly specified manifest entries overwrite implicit values.

The patches purpose is as follows:

1.) Introduces a new method getManifest(MavenProject, MavenArchiveConfiguration), which is preferrable
over getManifest(MavenProject, ManifestConfiguration). Moves some functionality from
createArchive(MavenProject, MavenArchiveConfiguration) to the new method. (One would possibly
move more functionality from createArchive to getManifest or leave it all where it is. However, the
new method demonstrates what's required for the change.)

2.) Introduces a new method getManifest(MavenProject, MavenConfiguration, Map), which is internally
used by the other getManifest methods. The map is the map of manifest entries, if available, or an
empty Map. Rationale of the empty Map is that everything should like it was without the Map.

3.) Replaces all explicit calls to addConfiguredAttribute with a new method addManifestAttribute. The
new method checks, whether there already is an explicit value. If so, it prefers the explicit value
over the implicit value.

Note, that my current implementation changes the Map, which it receives as a parameter. This should possibly be changed, for example by replacing the parameter with a copy.

Jochen, please supply patches which are diff'd off the svn source. Your patches are based off of files with weird names (/tmp/MavenArchiver.java.1???) and can't be applied to SVN.

Couple other comments:

1) There's a one or two places where your code doesn't match the Maven style. Please correct if possible.
2) Collections.EMPTY_MAP is immutable AFAIK. It's not clear to me if this Map will be mutated. Please replace it with new HashMap() if the contents are mutable.
3) in your third patch, you do "if containsKey remove". It'd rather see "if containsKey return" as it minimizes redundant work.

Attached you find an updated version of the first patch. This patch is taken against the svn code base.

Creating the patch this way has one disadvantage: As the patches build on each other, I have to wait now until the first patch is accepted before I submit the second.

As for the code style: I am using the Maven Formatter for Eclipse. However, I am hesitating to use automatic reformatting, because it could increase the patch size. Automatic formatting is, IMO, something that the committer should use, and not the contributor. In other words, if I have formatting errors from time to time, please bear with me and change them silently. I'll doing my best to obey a code style which is unusual for me.

Following your suggestion in 3. eliminates the necessity to modify the supplied Map. In other words, an immutable Map (like Collections.EMPTY_MAP) is fine. I'll change the upcoming patches in that sense.

Mike,

Could you please make sure the fix is backported to other plugins generating a MANIFEST entry (Jar, War, Ejb, Ear I guess).

Thanks!

If the three patches are all for one file, please just supply one patch. There's no real reason to break it up into three pieces. Makes it easier for you and me.

Hi folks. Thanks for looking into all this.

I really would like to stress that thes attributes should not be forced into the manifest. They must be defeatable. One can see it make sense liberally re-using these attributes in an enterprise environment. But in any kind of project that does not have and follow an externally-defined specification, these do not make any sense. For instance, in a standalone application, these attributes are their own kind of ludacris.

A new user will also be confounded to find a specification for providing appropriate values for these does not exist. And here comes JSR-277 (http://jcp.org/en/jsr/detail?id=277), which is defining a module and versioning specification for Java

You might also note that you can include a Main-Class attribute in the mainfest and still be forced into having the extension attributes; and in such a manifest configuration, the main class is supposed to be the extension's installer program. And in general, who knows what the plugin (the Java browser plugin) or Java Web Start might do with these Jars with extension specifications: which are not well-defined by the Jar author.

For myself: these are going to fully cripple Maven's Jar packaging. I have to twiddle every manifest into shape before putting any Jar in service.

And I would extend these comments to the War plugin.

And on the subject of making a patch:

I got a ways into dropping the needed sources from SVN. But AbstractJarMojo is importing org.codehaus.plexus.archiver.jar.JarArchiver which does not appear to get resolved by any of the dependencies declared in the POMs. I couldn't figure out how the classpath was supposed to be set up.

Thanks again!

Cumulated patch with the following changes, compared to MavenArchiver.patch.(1-3):

• The supplied Map is no longer modified, as suggested by Mike.
• Consequently, Collections.EMPTY_MAP may be used.
• An empty string may now be used for suppressing entries. This should honor Steven's concerns.

@Stephane: The propsed patch is against the Maven Archiver, which is shared by the jar, war, ... plugins. In other words, your concern should be fulfilled automatically.

Including empty tags to squelch the attributes should work out well!

Great work.

Thanks again.

PS:

I'm going to file an RFE in the Sun database to have the Jar, Extension mechanism, and Versioning specifications cleared up and synchronized.

Updated 2.0.x branch patch. I just realized I don't have commit rights to the maven core so I will need to talk a core dev into committing this.

I'm giving up on this because it is simply too difficult to get changes to maven-archiver approved at this point because it is part of maven core and 2.0.x is locked down pretty tight at this point. maven-archiver is moving out of maven core for 2.1 so hopefully it will become a little easier to change and we can get the patch in the 2.1 release.

Hello again...

Well, all these comments are going on ( ! ) and I noticed my previous comment here, and I really am more thinking that these should just be turned off by default: if there's any "switch", they should be off by default and something should turn them on.

The reason I feel that way is that the POM is the project's descriptor: it is not the specification's descriptor. – Maven uses the <description> element as the Specification Title; but this really makes no sense – What is the plan if the POM element values change???

The description element in particular should really contain something more like "reference implementation xyz...": not something considered a title.

Maven really seems to be attempting to find some solution to how to make every Jar in the world a standard optional package; but it just shouldn't. (That's why ClassLoader is not final!!!) These manifest attributes should be rigidly defined and specified by the Jar author and no "defaults" should exist .

So I have to clarify my opinion and I feel Maven should just not to be doing any of this to begin with. It's highly un-standard; and these attributes in particular have a clearly defined usage.

Anyway, thanks for "your patience" – appRECIATED...

Steven Coco.

As MJAR-39 can be closed, I recommend not to apply Mike's suggested patch. I'll attach a modified patch, which removes the changes for MJAR-39.

I noticed some additional comments to review with regards to the manifest:
http://mail-archives.apache.org/mod_mbox/maven-users/200511.mbox/%3C82533828A9FA7E4292F08A1D132304813FA8A5@itomae2km04.AD.QINTRA.COM%3E

BTW, the archiver is now separate and can be released whenever it is required for the JAR plugin.

Brett, I have read your quote and my impression is, that the problem report is in essence a duplicate of MJAR-39.

As for your comment on releasing the maven-archiver: I'd recommend that this be done after Mike's patch to this issue is committed, which I cannot do. Perhaps Mike can do this now?

Whew. Patch applied to shared/maven-archiver.

though this is applied, when I set the jar plugin to use the archiver it still generates the elements

ok, the patch didn't actually remove anything, it just made it possible to override