Upgrade compiler to JDK17, fixes #1367. #1397
Conversation
devinrsmith
force-pushed
the
upgrade-java/compiler
branch
from
00a51a7
to
38633dd
Compare
Notes: * Gradle was upgraded to 6.9.1 to fix issue with needing to manually open jdk.compiler/com.sun.tools.javac.code. See gradle/gradle#15538, https://docs.gradle.org/6.9/release-notes.html, https://docs.gradle.org/6.9.1/release-notes.html * Gradle 6.9.1 can't be run from JDK17. As part of the CI setup, we need to make sure a non-JDK17 is set as the default. See https://docs.gradle.org/6.9.1/userguide/compatibility.html * zulu seems to provide more consistent release versions. It may be that we "adopt" is no longer terminilogy that should be used. See https://blog.adoptopenjdk.net/2021/03/transition-to-eclipse-an-update/ * Groovy does not like to compile the test code for open-api-lang-tools with JDK17. Various things were tried. Ended up setting Groovy compiler to 8 for now. May need to bump groovy version? * Some additional hinting was needing in the lambdas in TableHandleManagerSerial. I didn't dig in further why. * The gradle property org.gradle.java.installations.auto-download=false was removed from gradle.properties. Users wishing to guard against auto-download should set it as appropriate in ~/.gradle/gradle.properties. Note: auto-download is still disabled for CI.
Notes: * Groovy compiler _can_ be bumped up to Java 17, but the construction in gradle is a bit hacky, so it's not enabled. * The only module that needs groovy for compilation is open-api-lang-tools for testing. * Spock version in open-api-lang-tools for testing needed to be upgraded. * Newer spock needed useJUnitPlatform() for tests to run. * Various cleanup of open-api-lang-tools dependencies.
devinrsmith
force-pushed
the
upgrade-java/compiler
branch
from
38633dd
to
433eb7b
Compare
| distribution: 'zulu' | ||
| java-version: '17.0.0' | ||
|
|
||
| # Note: setting up JDK 8 last, the current version of gradle can't be run off of 17 yet. |
There was a problem hiding this comment.
i dont think order matters, as long as you set JAVA_HOME to the desired runtime jdk?
There was a problem hiding this comment.
Also, is that because we're still on an old gradle, or does even current gradle have that issue?
There was a problem hiding this comment.
Order doesn't matter, but by running this command last the tool is setting it up as the default (and/or exporting JAVA_HOME to it).
Current gradle can't be run off of 17 either.
There was a problem hiding this comment.
I understand, but I'm suggesting that it might be easier to install a set of JDKs and then specify the version to use in a single place, rather than reorder the list (in N places) when we want to change which one we're currently using. One line of comment traded for one env var, and never need to reorder the lists based on current preference?
| # We do not want gradle to auto-download a JDK - we are assuming that developers should be setting | ||
| # up their JDK environments as a prerequisite. | ||
| # See https://docs.gradle.org/6.8.3/userguide/toolchains.html#sec:provisioning | ||
| org.gradle.java.installations.auto-download=false |
There was a problem hiding this comment.
do you want to set this from our .github/workflow files so that ci never downloads what we didn't explicitly ask for?
There was a problem hiding this comment.
Yep, we already do that: .github/env/Linux/gradle.properties
| @@ -49,7 +49,8 @@ public TableHandle execute(TableSpec table) throws TableHandleException, Interru | |||
| final TableHandleManager manager = new TableHandleManagerSerial(session, tracker); | |||
| final TableAdapterResults<TableHandle, TableHandle> results; | |||
| try { | |||
| results = checked(() -> TableCreator.create(manager, i -> i, i -> i, table)); | |||
| // noinspection RedundantTypeArguments | |||
There was a problem hiding this comment.
I think I commented on this before, but is this a java type inference regression? It seems odd to need to be explicit when adding newer compilers.
There was a problem hiding this comment.
Yep. This is the only "weird" thing I couldn't figure out, but it appears to be the only place in our codebase where the JDK17 compiler wasn't able to compile (at least against release JDK8).
|
|
||
| testCompileOnly 'org.junit.jupiter:junit-jupiter-api:5.4.2' | ||
| testRuntimeOnly 'org.junit.jupiter:junit-jupiter-api:5.4.2' | ||
| testImplementation(platform('org.junit:junit-bom:5.7.2')) |
There was a problem hiding this comment.
@nate is landing a patch where this can be added automatically, rather than needing to set a new version in N files.
There was a problem hiding this comment.
Merged in main, fixed.
|
|
||
| testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.4.2' | ||
| testCompile "org.codehaus.groovy:groovy-templates:$GROOVY_VERSION" |
There was a problem hiding this comment.
I'm confused - I see that you removed the line above where GROOVY_VERSION was set, but I don't see where the new one was added? Is it automatically picked up from classpaths.gradle (and assuming so, can we avoid that and prefer Classpaths.groovy instead, our stopgap until platform/catalog)
There was a problem hiding this comment.
The standard groovy dependency is being picked up from DB. The -templates part is necessary for some test code that breaks otherwise:
> Task :open-api-lang-tools:compileTestGroovy FAILED
startup failed:
/home/devin/dev/deephaven/deephaven-core/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerParserTest.groovy: 3: unable to resolve class groov
y.text.SimpleTemplateEngine
@ line 3, column 1.
import groovy.text.SimpleTemplateEngine
^
1 error
It may be that the groovy packaging was modularized in Groovy 3, so we have to add -templates back in here for this functionality.
I'll be more explicit here and migrate to Classpaths.
There was a problem hiding this comment.
After the Classpaths change:
$ ls -la grpc-api/server/docker/build/docker/libs | grep groovy
-rw-r--r--. 1 devin devin 8004986 Oct 14 09:01 groovy-3.0.9.jar
-rw-r--r--. 1 devin devin 133032 Oct 14 09:01 groovy-json-3.0.9.jar
Things picked up correctly.
Notes: