make AutoValue work in PubSub #1481
Conversation
googlebot
added
the
cla: yes
pongad
changed the title
This commit digs us out of the AutoValue dependency hell. Previously - We require Guava 20 for its Stats class. - Guava source has a `@CanIgnoreReturnValue` annotation, this annotation is in errorprone package. - Errorprone package required Java8 to run. So, I just took dependency on it without running to make the build pass in both 7 and 8. - Errorprone shades class javax.lang.model.type.IntersectionType. This class is Java8-only, but because it wasn't run, it didn't cause a problem. - AutoValue conditionally loads IntersectionType. It catches Exception and behaves properly if the class is not found; this is why it works on Java 7. However, because of the previous step, we have the class in our path. Because the class is 8-only, JVM7 loading it results in UnsupportedVersionError. This is an Error and not an Exception, and so AutoValue (probably wisely) ignore it and crashes. The fix is simple. Since #1480 already removed Stats, we can just go back to Guava 19. @garrettjonesgoogle is full of wisdom!
|
Changes Unknown when pulling 5d8b386 on pongad:autovalue into ** on GoogleCloudPlatform:pubsub-hp**. |
|
Changes Unknown when pulling 58be575 on pongad:autovalue into ** on GoogleCloudPlatform:pubsub-hp**. |
|
I don't really understand the explanation. If ErrorProne shades IntersectionType, nothing else should see it, even if explicitly loading javax.lang.model.type.IntersectionType, right? Also, I don't see where it is that AutoValue loads that class. |
|
@eamonnmcmanus I'm really not a Java expert but here's what I pieced together. I ran mvn dependency:build-classpath | tr : '\n' | grep error | xargs -l1 zipinfo | grep -e ^Ar -e IntersectionTypeThe command shows that AutoValue seems to be using |
|
To step back a bit, the reason I care is that it does not seem right to go back to an earlier version of Guava. You should be able to use the latest version always. The code in com.google.auto.common that you reference could catch Throwable rather than Exception. We'd have to make new releases of auto.common and auto.value for that to apply. I'm very puzzled at the dependency from Error Prone that you show. It really shouldn't be polluting the class path with what appear to be classes from the Java 9 javac. Perhaps @eaftan can clarify? It's also not clear to me from the description why Error Prone needs to be in the picture at all. If it requires Java 8 to run, and you're supposed to be compatible with Java 7, then it sounds as if you are making life difficult for yourselves. Perhaps you could have a Java 8 configuration with Error Prone and a Java 7 configuration without? |
|
@eamonnmcmanus I agree that it's not an ideal approach. Thank you for working with me on this. Including errorprone, in hindsight, seems to be a mistake. However, it seems to be necessary to use guava20. Without it Grepping the source code, the annotation is used in 184 files in guava20, while it is not used at all in guava19. |
|
Hmm, we were aware that Error Prone is polluting the classpath for our non-Maven integrations: google/error-prone#492 But we thought our Maven integration was working correctly. Also, you shouldn't need Error Prone just to use Guava 20. You should be able to depend on our error-prone-annotations artifact to get the |
| </dependency> | ||
| <dependency> | ||
| <groupId>com.google.errorprone</groupId> | ||
| <artifactId>error_prone_core</artifactId> |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@eaftan Thank you for the suggestion, this seems to make everything work now. |
| } | ||
|
|
||
| @AutoValue | ||
| public abstract class PublisherStats { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Changes Unknown when pulling e8220b4 on pongad:autovalue into ** on GoogleCloudPlatform:pubsub-hp**. |
|
PTAL. I have left SubscriberStats.Stats empty for now, since I'm still not sure what kind of stats we want. |
|
Changes Unknown when pulling 6c6bc99 on pongad:autovalue into ** on GoogleCloudPlatform:pubsub-hp**. |
This commit digs us out of the AutoValue dependency hell.
Previously
@CanIgnoreReturnValueannotation,this annotation is in errorprone package.
on it without running to make the build pass in both 7 and 8.
This class is Java8-only, but because it wasn't run, it didn't cause a problem.
It catches Exception and behaves properly if the class is not found;
this is why it works on Java 7.
However, because of the previous step, we have the class in our path.
Because the class is 8-only, JVM7 loading it results in
UnsupportedVersionError. This is an Error and not an Exception,
and so AutoValue (probably wisely) ignore it and crashes.
The fix is simple.
Since #1480
already removed Stats, we can just go back to Guava 19.
@garrettjonesgoogle is full of wisdom!