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
Shade the output jar, and relocate artifacts likely to cause collisions (notably auto-common) #118
Conversation
So - this is an initial cut. It produces a jar that I think is alright. I've re-hosted parts of guava, but I think the newer producers stuff emits guava types, so before I relocate these I want to ensure that we're not using static class literals in the emission of those types, because we'd start emitting shaded versions. So, this could conflict with other processors, but it should help, and at least be more resistant to upstream deps changes. |
This took way way longer than it should, but I kept testing and testing and couldn't quite get it the way I wanted it. At this point, I think it's good. |
@gk5885 PTAL |
<executions> | ||
<execution> | ||
<phase>package</phase> | ||
<goals><goal>shade</goal></goals> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split this into 2 lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
<artifactSet> | ||
<excludes> | ||
<!-- guava which has a consistent API and whose public types we vend in producers --> | ||
<exclude>com.google.guava</exclude> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this might cause problems. For example, Android gradle plugin has a transient dependency on guava 15. And when some plugin has a dependency on guava 18, it apparently sparks problems.
I do not think such a problem will arise in processors classpath immediately, but such a situation might happen in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. We've had this happen multiple times when used on the Gradle
buildscript classpath.
On Thu, Apr 2, 2015, 7:22 AM Roman Mazur notifications@github.com wrote:
In compiler/pom.xml
#118 (comment):
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-shade-
plugin
<version>2.3</version>
<executions>
<execution>
<phase>package</phase>
<goals>
<goal>shade</goal>
</goals>
<configuration>
<minimizeJar>true</minimizeJar>
<artifactSet>
<excludes>
<!-- guava which has a consistent API and whose public types we vend in producers -->
<exclude>com.google.guava</exclude>
Actually this might cause problems. For example, Android gradle plugin has
a transient dependency on guava 15. And when some plugin has a dependency
on guava 18, it apparently sparks problems.
I do not think such a problem will arise in processors classpath
immediately, but such a situation might happen in future.—
Reply to this email directly or view it on GitHub
https://github.com/google/dagger/pull/118/files#r27648909.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, Immutables.org shades Guava but uses a trick (example use) to avoid the maven-shade-plugin from rewriting Guava class name literals used in generated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - @tbroyer - we probably need to move away from literals anyway, and I have an internal commit I was toying with to do just that.
Given that this pull improves the situation of google processors working together, and improves upon no shading, I'm inclined to move forward with this and refine.
I don't think this pull with "cause" problems that aren't already present. Am I missing something? Right now there's no shading.
Yeah, as an incremental step forward, this LGTM |
Shade the output jar, and relocate artifacts likely to cause collisions (notably auto-common)
No description provided.