-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Separate AutoValue Annotations and Processor into different dependencies. #268
Comments
Best workaround I've found so far is using |
Also, +1 😀 |
AutoValue doesn't have any runtime dependencies (since the
|
This is what everyone is doing. It's bad for two reasons:
|
While I tend to agree:
I'd note that https://immutables.github.io went the exact opposite way in their 2.0, putting everything into a single JAR under the argument that it confused users (if you ask me, their mistake was to use a "standalone" JAR containing the processor and the annotations, in addition to an annotation-only JAR, rather than making a "processor" JAR with a dependency on the annotation-only JAR). That said, did I say I tend to agree? Having the same dependency in two distinct Gradle configurations ( |
For the sake of discussion on the user confusion point, I'd like to point out that this is exactly what Dagger and it seems to work well. Correct me if I'm wrong, but with the processing jar having a dependency on the annotations jar, there wouldn't actually be any difference in the workflow for Maven users since, as you said, Maven doesn't have an apt scope. |
@rharter Absolutely, wouldn't change much things, and would have a few happy consequences. So to clarify: I'm +1 to making 2 artifacts the same way Dagger (both 1 and 2) do, i.e. with a processor (or compiler, I don't care) one depending on the annotations (or core) one; not the way Immutables 1 did ( |
Previous discussion on #250 as well. @eamonnmcmanus recently reopened it. I'm not gonna restate my argument but, yeah, separating annotation and processor would be a great thing, especially now that AutoValue is getting extensions. If |
It's more that they go into independent "classpaths" (to me at least): annotations are needed by the code that you compile (so goes into the compile classpath), and the processor goes into processor path. Technically, the processor doesn't even need to depend on the annotation (see how https://github.com/tbroyer/bullet/blob/aff7e4d3d4a944aef2c891a81d68919fa0000c63/compiler/src/main/java/bullet/impl/ComponentProcessor.java didn't depend on Dagger). |
I agree, I'd like to see the minimal code required by the compiled code in On Sun, Sep 20, 2015, 4:03 AM Thomas Broyer [email protected]
|
+1 on separate JARs. It's really confusing to see Guava classes appear all over the place in code completion, especially if they are dependencies to the processor. I just reviewed a PR where someone accidentally used a shaded Guava class, and this would crash at runtime because the dependency is merely part of the |
If anyone is interested in how to solve this:
|
And that assumes you have an |
Yes that assumes the apt dependency is still in place. On Fri, Sep 25, 2015, 16:51 Jake Wharton [email protected] wrote:
|
@JakeWharton about the NullPointerException when It might be due to differences in how the |
Got it. I'm using a deployed version of Ryan's extensions PR based on On Thu, Oct 1, 2015 at 5:28 AM Matthias Käppler [email protected]
|
Ah, makes sense. Good to hear this will be resolved in 1.2 then. On Thu, Oct 1, 2015 at 4:36 PM Jake Wharton [email protected]
|
I am interested in this as well. I would also like to see a similar change made to auto-factory. |
We're a little behind while I am working on our open-source tooling, but I On Thu, 8 Oct 2015 at 12:56 Andrew Crichton [email protected]
|
Any updates on this? Will be great to have separate jars in 1.2-rc2! |
We might or might not split the artifacts for 1.2, but the urgency of doing so is much less since all dependencies have been shaded with a |
We're on to 1.3 coming up, would be great to see this given more consideration. |
I opened a pull request (#352) to do this for auto-factory. I intend to do it for auto-value as well (time permitting I will do it for auto-service), but want to make sure what I have done is desired way. I have done it by making the factory module have 2 sub-modules (core and compiler). I could see wanting to do it by splitting the module directly at the root, but given that the project root isn't meant to be built "normally", I figured this method would be preferable. |
The linter in the Android Gradle 3.0-alpha3 plugin now gives an InvalidPackage error because javax is not included in Android. This is with the following configuration:
I assume that if there were another annotation-only artifact for the compileOnly side then lint wouldn't see the javax usage. |
@simtel12 Figured out any way to sort it out? |
@VeeraAnudeep Honestly, I'm not sure. I updated to 3.0-alpha4, a bunch of other stuff changed in our source, and I'm no longer getting the lint error. I may have worked around it, or it may have been fixed in alpha4. :\ My suggestion, try updating to alpha4. |
The lack of a separate artifact is now breaking users of Dagger. The Dagger compiler sees the transitive Guava dependency and generates code assuming it will be available at runtime. |
@JakeWharton Guava is not a transitive dependency, it's shaded into auto-value (into |
Damn. I'm just trying to push this issue through 😞 . It's annoying to have to maintain a separate annotations artifact. |
I've stopped worrying and just use compileOnly 'com.google.auto.value:auto-value:1.5.1'
annotationProcessor 'com.google.auto.value:auto-value:1.5.1' Dependencies are properly shaded, and "hidden" by prefixing the class names with a |
Here are my 2 cents for splitting up the jar: It will improve performance and keep the classpath clean. Quote from Use the annotation processor dependency configuration:
Another disadvantage of the single-jar approach is that Android users must usually disable an error check |
Another quote from Android Gradle Plugin 3.0.0 migration guide:
So android-gradle-plugin 3.0.0 users are forced to set includeCompileClasspath option until next plugin release or to use standalone annotations artifact by Jake Wharton to build projects. |
Also because of this Dagger version 2.12 thinks that guava is available and uses ImmutableSet from guava. Which basically breaks compilation. |
This is probably not true. I had same problem and found out it was actually auto-value-gson who adds Guava to classpath and not AutoValue. See conversation above - #268 (comment). |
This is definitely possible. We did have gson library and did the same change together. I assumed it is a problem in both libraries. Gson one does provide separate annotation artifact though. |
Was just discussing this with @eamonnmcmanus - we're thinking that creating |
@JakeWharton mentioned offline that the above comment makes sense to him. I'm going to try and get this taken care of. |
Fixes #268 I followed the instructions in https://maven.apache.org/guides/mini/guide-using-one-source-directory.html because it seemed like the easiest way forward without moving around any files. RELNOTES=`@AutoValue`, `@AutoAnnotation`, `@AutoOneOf`, and `@Memoized` are now in a separate artifact, `auto-value-annotations`. This allows users to specify the annotations in compile scope and the processor in an annotation processing scope, without leaking the processor to a release binary. To upgrade to this version of auto-value, you'll need to add this new artifact as a dependency. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=188505001
Fixes #268 I followed the instructions in https://maven.apache.org/guides/mini/guide-using-one-source-directory.html because it seemed like the easiest way forward without moving around any files. RELNOTES=`@AutoValue`, `@AutoAnnotation`, `@AutoOneOf`, and `@Memoized` are now in a separate artifact, `auto-value-annotations`. This allows users to specify the annotations in compile scope and the processor in an annotation processing scope, without leaking the processor to a release binary. To upgrade to this version of auto-value, you'll need to add this new artifact as a dependency. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=188505001
compileOnly 'com.google.auto.value:auto-value:1.5.2' |
No code changes, just moving files, adjusting POM files, and updating the README. See issue google#268.
Reopening in light of #352 (comment) |
No, wait, sorry, this one is about AutoValue... :) |
It would be really helpful, especially in constrained environments like Android, to have the Annotations and the Processor separated into different dependencies. The current structure leads to unnecessarily bloated output binaries.
Currently on Android, if the user includes AutoValue using
compile 'com.google.auto.value:auto-value:1.2'
then all of the processing classes and all of the shaded classes are included in the final binary. There are ways around this, but they are hacky and unclear.Separating the annotations from the processor would allow something like the following:
This naming would allow existing users to update smoothly (assuming
auto-value
depends onauto-value-annotation
)This would only include the minimal set of classes (annotations) required in the output binary and ensure that all other classes, including shaded classes, aren't unnecessarily included.
The text was updated successfully, but these errors were encountered: