The PR is rebased on current main, which should read better than the version
you looked at: https://github.com/apache/calcite/pull/5213
Since your review a NullAway release fixed several of the issues I filed
upstream, so a number of the workarounds in the earlier draft are gone. GitHub
reports the change as 1510 files, +5263/-5583.
It is not green, and the reason is not NullAway.
Five jobs fail, and they are exactly the ones that compile the tree with an old
javac: Windows (JDK 8), Linux (JDK 8, latest Guava), Linux (JDK 8, oldest
Guava), Linux (JDK 11, Pacific/Chatham) and Linux (JDK 11, Avatica main). The
failures are in bytecodeCheck:
> Task :elasticsearch:bytecodeCheck FAILED
Invalid bytecode file: .../elasticsearch/.../ElasticsearchJson.class
Caused by: Invalid start label (must be visited first)
at org.objectweb.asm.util.CheckMethodAdapter.visitLocalVariableAnnotation
with :cassandra:bytecodeCheck failing the same way on JDK 11. JDK 17, 21 and 24
pass, on Linux, Windows and macOS alike, as does the ErrorProne and NullAway
job.
Sergey, this is the answer to your question on the JIRA, and it is CALCITE-6393
again: same exception, same ASM call site. That one came from a downstream user
who could not process the 1.37.0 rc4 jars, and your note there says the
workaround is to build with JDK 11 or later. That is no longer sufficient --
cassandra fails on 11 today.
On verification, briefly, because there are two checkers in the tree and they
are easy to conflate. The jandex plugin has always been applied and validates
bytecode as a side effect of building the annotation index, but nothing in CI
ever invoked it. An in-house ASM checker was later added in buildSrc, and its
bytecodeCheck task is wired into check, so it does run in every job that runs
build. The guard is therefore in place and working; what this PR changes is the
input. Replacing the Checker Framework annotations with JSpecify ones puts type
annotations on local variables where there were none, and that is what javac 8
and javac 11 get wrong. I have not tried to pin it to a particular javac bug
id, because the fix is the same either way: compile with a current javac.
The cause is upstream of all of this. Calcite compiles with whichever JDK
happens to run Gradle: build.gradle.kts sets sourceCompatibility and
targetCompatibility to 1.8 and declares no Java toolchain, so the JDK 8 and
JDK 11 jobs really do run javac 8 and javac 11 over everything. Without
--release, a build on a newer JDK has the opposite problem: it emits version 8
bytecode while linking against that JDK's class library, so code that compiles
cleanly can fail on Java 8 at runtime, and whether a release artifact has that
problem depends on the JDK the release manager happened to use.
What I would like to change is the build mode: compile once on a current JDK
(25) with --release 8, and use 8, 11, 17, 21 and 25 for running the tests
rather than for compiling them. pgjdbc and JMeter are both set up that way. I
maintain both of those build setups, so treat that as one person's experience
rather than two independent reports; the workflow files are public if you would
rather judge the maintenance cost than take my word for it. JMeter is an ASF
project, so its release process already runs this way.
The alternative is to exclude the affected classes from bytecodeCheck, which
puts us back where CALCITE-6393 started, so I would rather not.
Unless someone objects to the direction, I will file a JIRA and send it as its
own PR. Two things I will raise separately rather than here.
First, if CI invoked jandex, the in-house checker would be
redundant, and dropping it would take buildSrc compilation off the critical
path. Second, once the old JDKs become test axes rather than build
environments, and the existing OS, Guava and time zone variations multiply with
them, the hand-written job list stops scaling; that is a question about the CI
matrix rather than about this change, and it deserves its own thread.
This PR is ready otherwise.
Vladimir
