-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
Remove annotation-only Jars from runtime classpath (in Gradle) #6606
base: master
Are you sure you want to change the base?
Conversation
|
Sadly, |
POM's At build time of the local project, all scopes may have some meaning for the Maven Tool. In the repository – POM as metadata – only these two have meaning:
Anything with another scope (or with optional=true) has no meaning in metadata. It is ignored by both Maven and Gradle when parsing the file as metadata of an already built dependency. Another interesting tidbit is that the meaning of runtime is a bit different in the pulished metadata. You can use it there for dependencies that you needed to compile your own code, but want to hide on the compile classpath of consumers. Because at this point the code is already compiled, runtime is sufficient. In Gradle, for the local project you have more scopes that influence the metadata. But only some of them map correctly to POM as metadata (which Gradle generates in addition to the Gradle Metadata for Maven consumers).
|
In the past compiler bugs like JDK-8152174, scala/bug#7751, and earlier meant putting annotations on the classpath was the safest option. Would this change allow for encountering those types of bugs again? (No preference, just backstory) |
That's what this is about: not running into problems like #6606 (comment) again but still solving #2824.
For Gradle, we can solve this now: have dependencies at compile time, but not at runtime. That's what this PR does for the annotation libraries. |
https://github.com/google/guava/wiki/UseGuavaInYourBuild#what-about-guavas-own-dependencies mentions at least 2 runtime problems, one of which is still probably a concern for one of our deps:
JDK-8152174 is potential trouble, albeit only for JDK-8247797 is probably fine, which is interesting: Despite what its writeup suggests, I can successfully reflect on
It looks like the problem actually surfaces only when
|
Thanks for @cpovirk for clarifying that some issues are also about runtime. My understanding though is that you have to be in a very specific situation to get into such an issue. You need to use reflection on a class that uses these annotations. Which you could do an a Guava class. (But is that an intended use of Guava?) I guess my take on this is, that if users do some of these things, it is most likely that they should directly declare dependencies on the annotation libraries they want to deal with and not rely on Guava brining them in transitively. I don't know "real" projects doing something like this, though. So my assumption here might be wrong. I don't know how big the part of the user base is that still uses JDK8 and does something so specific that they run into the bug. It feels like it could be ok to document that you need to add a certain We could also drive the variant setup further and have a separate dependency setup for JDK8 (that keeps one or all dependencies) and do the clean setup for everyone using JDK9+. |
There seem to be frameworks out there that try to automatically (e.g.) serialize arbitrary objects by looking at their fields. That is of course not a great idea, and it's one of the reasons that the Java module system exists (and perhaps a reason for us to release a jar with a I would probably still start with omitting the other deps first, but I'm warming a little to trying this out for checker-qual, too. We'll see if others have something to add. Given the flood of releases lately, we'll probably wait a little before publishing another one, so we have time to figure this out. |
Well, type annotations add information to the type. For instance, a JSON parser might user object constructor when parsing JSON, and it might inspect if the parameters are |
@vlsi in such cases the tool depends on those annotation types itself. Or in the case of "nullable" just looks at the name and none of the details of the annotation. |
If the annotation class is not provided in the runtime, the reflection API does not return the annotation, so the tool can't even detect annotation was present in the first place. Of course, there's a backdoor like In other words, I believe it is fair to expect that "types used in the public signatures" (e.g. return types of the public APIs) should be present in "direct runtime dependencies". At the same time, the annotations from error-prone do not sound like "type annotations", and they are much more like "compile-time-only" annotations, so having them as |
ec56503
to
c0f413d
Compare
Also reverts #9ed0fa
c0f413d
to
1f41f73
Compare
(breadcrumb: If we do this, we'll want to watch out for GWT issues like #7134. That's not a big concern, and I remain more nervous about omitting dependencies for other reasons than for GWT reasons.) |
Proposal to remove the following Jars from the runtime classpath (in Gradle projects):
checker-qual-*.jar
error_prone_annotations-*.jar
jsr305-*.jar
Afterwards, they are still transitively visible on the compile classpath, which can not be expressed in POMs, but in Gradle Metadata which is published starting with the next Guava release (33). In a Gradle build setup, this would be "compileOnlyApi" scope. See also #2824 (comment) and the linked Maven feature request for the discussion about having such a "compileOnlyApi scope" in Maven.
Context:
runtimeOnly
. You could consider them as an "optional feature" that you have to add yourself if needed.