-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Fix compilation in Java 9 #3638
Conversation
There's quite a bit of confusion on what to do:
javax.annotation-api seems the thing to use, but I'd really like to find some clear documentation saying to use it and which version to use (without becoming an expert on the JSR). I would have also hoped for clear documentation (like, "we deploy to Maven Central with artifact id BLAH") at https://github.com/javaee/javax.annotation, but alas. I'll note, it mentions module Because of the poor state of dependency clarity, I think it's very safe to believe all the deps will be mixed. Luckily, that may not break anything because In related news, I continue to dislike the poor usability of the JSR website. |
build.gradle
Outdated
@@ -214,6 +215,8 @@ subprojects { | |||
netty_proxy_handler: "io.netty:netty-handler-proxy:${nettyVersion}", | |||
netty_tcnative: 'io.netty:netty-tcnative-boringssl-static:2.0.7.Final', | |||
|
|||
conscrypt: 'org.conscrypt:conscrypt-openjdk-uber:1.0.0', |
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.
intended?
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.
Yes. Jetty ALPN doesn't exist in Java 9. So for OkHttp TLS testing it will use Conscrypt instead.
if (ServerBuilder.class.isAssignableFrom(clazz) && clazz != ServerBuilder.class) { | ||
classes.add(clazz); | ||
} else if (ManagedChannelBuilder.class.isAssignableFrom(clazz) | ||
&& clazz != ManagedChannelBuilder.class ) { |
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.
nit: extra space
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.
It was pre-existing, but done.
@@ -48,19 +49,39 @@ | |||
*/ | |||
@Parameters(name = "class={0}") | |||
public static Collection<Object[]> params() throws Exception { | |||
@SuppressWarnings("unchecked") |
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.
This somewhat defeats the purpose of this test. It showed that every class that gRPC knows about overrides the static ctors, but since you enumerate it, new builders will be missed. (Like the recently added Alts Channel)
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.
Not so. We have a hard-coded list, but it is verified when possible (JDK < 9). So that means the list will be kept up-to-date. It is true that if we only ran on JDK 9 the list would likely get out-of-date, but in that case it seems like the best we could do is use the hard-coded list or delete the test. Looking at ClassPath.Scanner.getClassLoaderUrls, it seems impossible to fix for Java 9.
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.
If this doesn't work for Java 9, it might as well not run. The list will get out of date, and it will give false confidence.
It would be better to check the java version and call Assume.assumeTrue(javaVersion() <= 8) in a BeforeClass. If that isn't possible you could make a dummy test that asserts there is more than one class to check and throw an assumption exception otherwise.
If this test doesn't work on java 9 then so be it. That's okay.
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.
@BeforeClass
runs after params()
, so an assume in it doesn't work. Similarly, a failing assume in params()
causes a test failure. And a dummy test doesn't work because no tests are run if params()
returns an empty list.
I'm detecting when getTopLevelClassesRecursive()
returns nothing and returning no params in that case. Note that returning an empty list of params causes the test class to be excluded from test reports, since by definition there were no tests within the class. So there isn't any "ignored" count or similar.
@carl-mastrangelo, PTAL |
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.
LGTM
Previously it was just erroring: [[: command not found This has been broken since #3638
Tests still fail; at the very least because of missing Jetty ALPN.Idon't know why the dependency on javax.annotations is unnecessary in the
compiler tests and the example when using Maven.
This depends on #3637. This isn't quite enough for #3633, because we've
not yet clear in our README what's necessary. The annotations stuff is still
very confusing to me; I think our users will suffer unless we have a pretty good
understanding of what's going on and clearly document what they need to do.
CC @carl-mastrangelo