Relocate ASM and JNR classes in shaded JAR

[JoshRosen]

The goal of this patch is to relocate the ASM and JNR classes that are included in the shaded JAR.

This change is motivated by a dependency conflict issue that I ran into while trying to use docker-client in Apache Spark's tests. There's a longer writeup at apache/spark#9503 (comment) which describes our dependency conflict issues, but the TL;DR of it is that we are stuck running Jersey 1.9, which depends on ASM 3, and this causes conflicts with the ASM 5 that JNR uses.

Specifically:

• jersey-server 1.9 depends on asm.asm 3.1, which exposes its classes under the org.objectweb.asm namespace.
• docker-client 3.2.1 transitively depends on jnr-ffi 2.0.3, which depends on org.ow2.asm.asm 5.0.3. This version of ASM also contains classes under the org.objectweb.asm namespace, producing a binary incompatibility conflict which Maven doesn't detect.

Given that the primary motivation for having a shaded artifact seems to be avoiding conflicts with old versions of Jersey, I think that the best solution here is to relocate the ASM and JNR classes in docker-client's shaded JAR.

---

This change is 

[KostyaSha]

👍 Imho all dependent classes must be relocated.

[JoshRosen]

On closer inspection, it looks like the published shaded JAR is currently including ASM classes without relocating them, which seems really bad because it means that dependency conflicts between those bundled-but-non-relocated classes and other copies of ASM that exist on the classpath:

$ wget https://repo1.maven.org/maven2/com/spotify/docker-client/3.2.1/docker-client-3.2.1-shaded.jar
$ jar tf docker-client-3.2.1-shaded.jar | grep Opcodes
org/objectweb/asm/Opcodes.class

Similarly, it looks like the shaded JAR contains all of the un-relocated JNR classes, which is going to cause hard-to-debug classpath ordering issues if a library which depends on docker-client wants to use a different JNR version.

Unless I've overlooked something, this library's current use of shading doesn't make sense. I think that it would be better to either shade and relocate all dependencies or to relocate all shaded dependencies and use Maven to pull in the un-relocated dependencies.

[KostyaSha]

(I'm using docker-java, but shading issue is the same for both libs)
If you want use JNR version from shaded jar, then you should use imports with shaded path. If not - then any jnr class version from classpath.

But then you will get issue if you are using 2 different library versions as they may have different versions of shaded artifacts that has the same package names. Now i think that you need shade library with deps yourself in your application (but note that maven-3.3 has bug https://issues.apache.org/jira/browse/MNG-5807 and we had different related discussion in Jenkins) with different prefixes. In this case shading in library pom.xml will be just an example of shading configuration.

[JoshRosen]

If there's agreement that we should relocate every third-party class that is bundled into the -shaded JAR then I will close this PR and submit another one to pursue that approach.

[JoshRosen]

I spent a bunch of time investigating this today and it looks like there's no easy solutions here.

With a shaded artifact, my goals would be to a) relocate every third-party class included in the JAR, and b) drop those bundled dependencies from the POM while promoting transitive dependencies. In principle, the createDependencyReducedPom and promoteTransitiveDependencies shade options should handle this by generating a new POM. However, these options only take effect if shadedArtifactAttached is false.

Other projects have run into this problem and have come up with a few solutions:

• Kryo decided to only publish the shaded JAR: https://code.google.com/p/kryo/issues/detail?id=116#c7
• HBase publishes shaded JARs as separate artifacts (not using classifiers): https://issues.apache.org/jira/browse/HBASE-13517

The HBase solution seems to work nicely, but it's a large maintenance burden and would also require a shift to a multi-module Maven build. It's also kind of complex; see https://github.com/apache/hbase/blob/44367f55e8bbd252ae824d1ddb626b5ce91fe75d/hbase-shaded/pom.xml

Other people have also run into the limitations of a classifier-based approach: http://maven.40175.n5.nabble.com/shade-plugin-attached-artifact-and-dependency-reduced-pom-td5765599.html

Given all of this, one low-effort way to move forward might be the following:

• Drop the ASM and JNR dependencies from the shaded JAR.
• Instruct users on how to add excludes in order to prevent the Jersey and Jackson JARs from the non-reduced POM from continuing to be pulled in as transitive dependencies when users depend on the shaded-classified artifact.

[JoshRosen]

Also, it would be good to shade Guava: it doesn't have any transitive dependencies and is quite conflict-prone itself.

[KostyaSha]

@stephenc maybe you have some generic opinion for such cases?

[stephenc]

It seems like what you really want to be doing is publishing the shaded artifact under a different artifactId rather than as an attached artifact... https://maven.apache.org/plugins/maven-shade-plugin/shade-mojo.html#shadedArtifactId would seem to be what you want, and with that case (as long as you do not try to consume the shaded artifact from the same reactor that is building it) then you should not hit the POM immutablity issue in Maven 3.3.x (though @jvanzyl is of the opinion that a different packaging type for shaded artifacts is the correct solution... which would again drive you to publish the shaded artifact to a different artifactId from the unshaded one)

@jvanzyl here is a case in point w.r.t. the shade plugin and modifying the project model

[KostyaSha]

@stephenc right! I forgot this classical trick with package names :) Classifier sounds right but it seems in maven classifier usage is not designed well i.e. using it for branch builds instead of maven version set.

IMHO multi-module or other artifactId looks good

[stephenc]

A classifier gets the same dependencies as the main artifact... which is not what you want here, rather you want a different artifactId as the dependencies are different

[JoshRosen]

I've updated this to also relocate the JNR classes. While I feel that publishing a shaded artifact using a dependency-reduced POM would make sense, I'm glad to defer that larger build change to a different PR.

In the meantime, how does this relocation of these shaded classes look? Is there still interest in getting this relocation merged in?

[JoshRosen]

I realized that this is the wrong relocation pattern: while the artifact's organization name is com.github.jnr, the JNR classes are under the jnr.* namespace.

[codecov-io]

Current coverage is 50.45% (diff: 100%)

Merging #272 into master will increase coverage by 3.80%

@@             master       #272   diff @@
==========================================
  Files           130         83     -47   
  Lines          4418       3219   -1199   
  Methods           0          0           
  Messages          0          0           
  Branches        651        456    -195   
==========================================
- Hits           2061       1624    -437   
+ Misses         2161       1445    -716   
+ Partials        196        150     -46   

Powered by Codecov. Last update 687b27f...c1bef52

[davidxia]

Do people still want this? If not, can I close?

[JoshRosen]

We're no longer blocked on this, so I'll close. I still think this is ultimately worth fixing from a best-practices POV, though, since including other libraries' classes without relocating them to a new namespace can be problematic from a class conflict POV.

[mkjensen]

I'm experiencing a conflict between cassandra-driver-core which has a couple of dependencies to com.github.jnr artifacts. I think this would be solved by docker-client relocating com.github.jnr to com.spotify.docker.client.shaded.com.github.jnr like what has been done for other packages as well already.

(Actually, the conflict is missinglink complaining about missing methods depending on whether cassandra-driver-core or docker-client is first on the class path.)

Edit: Sorry, I meant relocating jnr to com.spotify.docker.client.shaded.jnr.

[davidxia]

Thanks for reporting this. If you could make a PR, we can review it. -- David Xia Software Engineer [email protected]