MSHADE-373: adds a property to skip manifest transformer application when transforming a source file.

[raphw]

Manifest transformers in the shade plugin are often used to add meta-data for OSGi use since the shade and OSGi plugins are not overly compatible. If the same transformer is applied to the source plugin's manifest, the resolution will fail due to a duplicate bundle id.

It is therefore necessary to being able to opt-out of the manifest transformer when shading a source file. An example of this problem is documented in raphw/byte-buddy#894.

https://issues.apache.org/jira/browse/MSHADE-373

[rmannibucau]

Hi @raphw ,

I wonder if it wouldn't be more consistent and correct to handle it at transformer level.
We already started to work to support OSGi relocation (org.apache.maven.plugins.shade.resource.ManifestResourceTransformer#defaultAttributes) so I guess it is the thing to enhance instead of adding a global property which only works in association with some transformer setup anyway.

So here is how I'm seeing it:

1. you want to keep the bnd manifest -> you don't set manifest transformer
2. you want a new custom manifest (jars must have one anyway) -> you set manifest transformer with a custom entries configuration. In this case the change is about exposing manifestDiscovered with a setter to let it be configured in the transformer (setSkipManifest(boolean v) {manifestDiscovered=v;})

Does it makes sense for your case?

[raphw]

I don't think that that's my problem: I shade the actual jar and the source jar. Doing so, I want to add a class-bundle name which I do not want to be part of the original, unshaded jar. I neither want it to be part of the source jar since the bundle name identifies the jar file uniquely within OSGi. If a tool (Exclipse in this case) depends on both jars, the actual jar and the source jar, on it's OSGi class path, this resolution will fail since both jars specify the same unique name. I need to be able to inject this unique name into the shaded jar file without also injecting it into the shaded source jar.

Is there a better way to do this? The problem is reproduced here: https://github.com/raphw/byte-buddy/blob/master/byte-buddy/pom.xml and breaks Eclipse.

[rmannibucau]

@raphw hmm, ok, let me try to reformulate because if I get it right I tend to see the "skip" fix as a quick workaround but an inaccurate (hope next part explains why). So my understanding is that createSourcesJar leads to a duplicated manifests in 2 different jars and this is an issue for you because eclipse uses both jars in an OSGi container.

My understanding of this issue leads me to these two lacks in current plugin we can fix:

1. You can force a manifest entry to be ignore (I assume we can have a custom value to say 'ignore if I'm equals to X" - org.apache.maven.shade.ignore for example - and then it can be forced in manifest transformer and enables to drop manifest entries from the original manifest (thinking to OSGi -> SE fatjar case for example).
2. Implicit source shade has no manifest customization which means in org.apache.maven.plugins.shade.mojo.ShadeMojo#createSourcesJar we fully inherit from the main shade and therefore the manifest is 1-1 with the original artifact. I would add a flag in ManifestTransformer to mimic the mojo config ("main", "source", "test", "test source") with each one having a "manifestEntries" customization set which would enable to select the right "manifestEntries" for each shade and to users (you) to override some values. It just means we must filter per shade "phase" the right manifest but this is not something crazy IMHO. Finally, for backward compatibility, if there is no transformer for phase X we would keep using the main one.

Here is a pseudo config illustrating that:

<transformer implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer">
    <mainClass>net.bytebuddy.build.Plugin$Engine$Default</mainClass>
</transformer>
<transformer implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer" forShade="sources">
    <manifestEntries></manifestEntries> <!-- set what you need -->
</transformer>
<transformer implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer" forShade="test" />
<transformer implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer" forShade="test-sources" />

Does it make sense or did I get it wrong?

[raphw]

Yes, your approach would probably be an even better solution since it still allows to shade the sources jar's manifest file. My particular issue is the Bundle-SymbolicName entry which is supposed to be unique. If it is also contained in the source.jar, the jar will be interpreted as a conflicting OSGi bundle if loaded by an OSGi class loader, even if only the sources are required.

When using the maven-bundle-plugin in conjuction with the maven-source-plugin, the first plugin only adds manifest entries to the manifest of the actual jar file. But when using the maven-bundle-plugin with the maven-shade-plugin, this is not the case if the shade plugin is also creating a shaded source jar where the manifest will be included with the OSGi headers. It is this scenario that I would like to avoid.

[rmannibucau]

Ok so 2 questions/points:

1. Do you update the pr? Im also happy to help if needed
2. A workaround is probably to do 2 shade executions and to use artifact set to include only sources and set createSources to false, itenables to define a custom manifest transformer only for source artifact

[raphw]

I can do it but wonder how one would capture the forShade="sources" attribute of the XML in the configuration. Would this not need to be a property? Is this even possible in Maven?

[rmannibucau]

Yes we can but just code it as another child (an enum or steing in the transformer directly. It is enough IMHO.

[raphw]

From looking into the code, I don't quite understand where to begin. As it is now, the ManifestResourceTransformer has no idea of the targeted artifact. My first idea would be to add a list for getter and setters named forShade in ResourceTransformer. Then, when creating the shade request, I'd check the eligable targets. Is that how you thought of it?

[raphw]

@rmannibucau Is this what you had in mind? I would not know how to test this appropriately, unfortunately. Any hits? Is this the right approach?

[rmannibucau]

@raphw i was thinking to do it for manifest transformer specificly since we cant break backward compatibility for transformer api - we had issues in last 2 versions and it was a very bad idea so hope we avoid it now. High level i was envisionning a filterManifestTransformer(transformers, phase) which would copy the list keeping only a single manifest transformer if any depending the phase - we can probably fail or log an error if ambiguous since we extract a single manifest transformer elsewhere already. Not as generic as your solution but does not need a compatibility layer which requires to add proxying logic we dont have yet and we can probably avoid. About the list vs String I think String can be enough but no strong opposition to a list.
Finally to test you can check the IT if the project, using invoker plugin we can validate it e2e, other check shader test, we can probably write lighter tests there to ease work (IT are still needed but can become global e2e tests instead of primary tests then).
Hope it makes sense.

[raphw]

Alright, I adjusted the PR and added an integration test. Let me know if you want med to do anything more.

[rmannibucau]

@raphw I propose these changes raphw#1, wdyt? Can you try it on bytebuddy?

[raphw]

Dp you have an idea when this patch is being released?