Shade dependencies

[Zubnix]

Currently dagger compiler has a (compile time) dependency on guava. Ideally this should be shaded as not to conflict with any other runtime provided guava dependency by the user.

Use case:
User forgets to add it's own guava dependency -> project finds dagger compiler guava version -> user uses wrong guava version

[tbroyer]

Ideally, dagger-compiler would not be on the classpath but on the processorpath, so the Guava dependency would be isolated, and a missing Guava dependency on the classpath would cause compilation failures.

Indeed Maven is among the few build tools that don't allow you to set the processorpath¹, and will include dagger-compiler and its dependencies (assuming you've configured it as optional or provided) even in the test classpath, so you won't even be able to detect the issue when running tests (only way would be to have tests in a separate Maven module; yay Maven!)

¹ see https://issues.apache.org/jira/browse/MCOMPILER-203, https://issues.apache.org/jira/browse/MCOMPILER-134, and https://issues.apache.org/jira/browse/MCOMPILER-75 (and yes, you read that right, this is an open issue for 7 years or so)

[netdpb]

I don't think we should contort Dagger to work around a known bug in Maven that only affects some users.

[cpovirk]

Does this end up affecting everyone who wants to compile with a newer version of Guava than Dagger is using (and use APIs from that newer version)? I'm changing Caliper to depend on Guava 19.0, but the compile fails because it's using the dagger-compiler (18.0?) copies of Guava classes.

Oddly, this seems to be the case even for Dagger 2.1-SNAPSHOT, which I would expect to include Guava 19.0-rc2. I'm wondering if the old Guava sneaks in through one of the compiler's other deps. To prevent this, I tried spraying pom.xml and compiler/pom.xml with exclusions, but this doesn't seem to have helped.

(Google Dagger people may also recall a conflict with another annotation processor (http://go/nbver). That won't normally happen in the Google depot, but presumably it can happen externally.)

[tbroyer]

@cpovirk Yes, this is the problem. For some reason dagger-compiler includes Guava classes non-relocated, instead of having a compile-time dependency on it (as originally reported here).
BTW it also includes google-java-format, Eclipse JDT and jsr305; I think this is the reason Guava ends up in the JAR: it's in the google-java-format JAR along with those other "dependencies".

[cpovirk]

@tbroyer Ah, it never occurred to me that one of the jar would contain it directly. Thanks!

I've filed google/google-java-format#20

[tbroyer]

Just a note that MCOMPILER-203 has been fixed in maven-compiler-plugin:3.5 so you can now have a proper -processorpath separate from your -classpath (and the Guava version used by Dagger can no longer conflict or interfere in any way with your own Guava dependency, or lack thereof); see #293

[gk5885]

Another issue brought up a similar issue with javapoet, so we should lump that into this discussion as well.

[ronshapiro]

Since we're not expecting to interop with any other libraries on the processor path, I think we should just shade everything to be safe. We can glob the top-level "packages" (com, org, etc) and just prepend dagger.shaded with them all.

[tbroyer]

Well, not shading some parts (e.g. Guava) would make a small JAR where those parts can be shared with other processors. I'm OK with the proposal to shade everything (or almost everything) "by default", but that should ideally be temporary only (pending a new release of google-java-format for instance; I suppose its API is stable enough that it doesn't need to be shaded, I don't really know about Eclipse JDT/ECJ used by google-java-format though, that could possibly conflict with other processors…)

[tbroyer]

@ronshapiro I tried globbing top-level packages, but trying to exclude Guava (as brought in by google-java-format), but for some reason it didn't work. I ended up with a mix of relocations and filters (to trim things down a bit), specific to what google-java-format 0.1-alpha brings in: see #350

In retrospect, let's be honest: all this would have been much easier to solve by just removing google-java-format from the equation. Is it really worth it formatting generated code according to Google style guide? I mean, it's generated code, it need not even be written to disk, it's only temporary!

[ronshapiro]

Why exclude Guava?

I do believe it's important to format the code, we want the code to be as
readable as possible and formatting definitely helps. Especially in large
components, without the formatter things can get kinda ugly.

On Sun, Apr 10, 2016, 11:02 AM Thomas Broyer [email protected]
wrote:

@ronshapiro https://github.com/ronshapiro I tried globbing top-level
packages, but trying to exclude Guava (as brought in by
google-java-format), but for some reason it didn't work. I ended up with a
mix of relocations and filters (to trim things down a bit), specific to
what google-java-format 0.1-alpha brings in: see #350
#350

In retrospect, let's be honest: all this would have been much easier to
solve by just removing google-java-format from the equation. Is it really
worth it formatting generated code according to Google style guide? I mean,
it's generated code, it need not even be written to disk, it's only
temporary!

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#228 (comment)

[tbroyer]

Guava (19.0) is used as a dependency (not bundled in the fat-jar), but Guava (18.0) is also bundled in the google-java-format (0.1-alpha) JAR and causes conflicts (see #356 ). For some reason, matching com and trying to exclude com.google.common from relocation didn't work (bytecode was still rewritten to reference the relocated classes, and those classes weren't included in the JAR).

Re. formatting; how about making google-java-format optional? If it's on the classpath, then Dagger uses it, otherwise it's bare JavaPoet.

[ronshapiro]

Formatting is important, we value the readability of generated code highly and IIRC JavaPoet doesn't wrap really long lines (which come up often with large components).

But if google-java-format needs Guava 18, and Dagger needs 19, isn't that the perfect case for shading?

[tbroyer]

But if google-java-format needs Guava 18, and Dagger needs 19, isn't that the perfect case for shading?

The foremost problem is that we have both Guava 18 and Guava 19 in the build path. Because Dagger uses Guava 19 APIs, Guava 18 must be excluded somehow. One solution could be to shade Guava 18 and depend on Guava 19, but in addition to creating an artificially-bloated JAR (google-java-format works great with Guava 19), I don't think this is possible with the maven-shade-plugin alone (possibly by first creating a shaded google-java-format though); or we could exclude Guava 18 and then choose to either shade Guava 19 or depend on it (what I've done; in addition to excluding a few other files and unneeded dependencies).

But please note that google-java-format has been fixed upstream, and all we need is a new release! Then google-java-format would depend on Guava, which Maven would resolve as Guava 19 given Dagger's own dependency on it, and then we can choose whichever shading pattern we want.

[ronshapiro]

Oh, doh. We can't shade Guava since Producers generates code that uses it. We could restrict things like ListenableFuture, Futures and supporting classes from relocation, but that might be tricky and brittle.

@cgruber is there a way we can enforce that Guava 19 gets picked up here?
@cushon is there enough to merit a new gjf release?

[JakeWharton]

You could also use ClassName.get("com.google.common.util.concurrent", "ListenableFuture"); instead of ClassName.get(ListenableFuture.class) for all generated types.

[cushon]

Sorry, the g-j-f release is long overdue. I'm on it.

[cgruber]

Apparently I missed this (copy @gk5885's rants about github notifications). anyway, the warnings actually show up in the shade output, when I went to re-examine this from the 2.3 tag. I'ma check and see if maven-shade-plugin warnings can be turned into errors, so we can have it break if hte shading gets... er... shady. Regardless, I just sent out an internal change for review that reverts the integration tests' workaround for this, so we'll catch this in the functional (maven invoker-run) tests in the future.

[cgruber]

Also, to 100% shading, we would have to stop using class literals in our code-gen stuff. Namely, we couldn't write out any guava types, which we do in the case of producers stuff (ListenableFuture, etc.) if we did it by class literals that got relocated by the shade plugin, as they would generate code with dagger.shaded.com.google.common.blahblah as their package. That's why we don't shade guava ourselves now, becasue we "vend" guava types in a few occasions.

So we'll have to move dagger compiler to either use strings or ClassName or some other abstraction instead of class literals. There's a fixed number of these usages, so it's not that bad. I prototyped it about a year ago, but there wasn't a lot of urgency then. I just created a bunch of constants which I used instead of class literals. Worked just fine. It then permitted me to make the shade plugin just relocate everything in its deps graph except for API jars it shares with client code, and minimize (eliminate unused code from the jar).

[jart]

Looking at the latest Dagger Compiler jar. It's 8MB. I'm noticing it:

• Depends on:
  • Dagger
  • Dagger Producers
  • Guava
  • Auto Value
  • Auto Service
• Includes in jar with shading:
  • Auto Common
• Includes in jar without shading:
  • com.google.errorprone error_prone_annotations
  • com.google.code.findbugs jsr305
  • com.google.googlejavaformat google-java-format
  • com.squareup javapoet

This isn't causing any substantial problems for me. So I'm not demanding it be changed. I just find it unusual.

[ronshapiro]

The latest compiler should have javapoet, jsr305, and error prone annotations removed. GJF is also removed, and that dependency is much smaller. Let's continue the discussion on shading in #350