Repository navigation
feat: upgrade to Java 17 (--release 17) - #11
tobydrinkall wants to merge 1 commit into
Conversation
Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| <artifactId>jmh-generator-annprocess</artifactId> | ||
| <version>1.17.4</version> | ||
| <version>1.37</version> |
There was a problem hiding this comment.
📝 Info: Annotation processor kept on compile classpath — works on 17 but breaks on JDK 21+ defaults
jmh-generator-annprocess stays a plain compile-scope dependency rather than being moved to maven-compiler-plugin's <annotationProcessorPaths>. This still works with JDK 17 (implicit annotation processing from the classpath is enabled), but on JDK 21+ javac defaults to -proc:none-like behaviour for classpath-discovered processors, so the JMH BenchmarkList/generated benchmark classes would silently not be generated and benchmarks.jar would list nothing. Since the CI workflow pins Temurin 17 this is latent, but it makes the build fragile for developers on newer JDKs and is worth addressing while the build is being modernized. It also means the processor jar gets shaded into the uber jar unnecessarily.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid future-hardening point, but out of scope for this change: the target is Java 17 and CI pins Temurin 17, where classpath annotation processing works (verified — benchmarks.jar -l lists all 5 benchmarks). Moving jmh-generator-annprocess into the compiler plugin's <annotationProcessorPaths> (and dropping it from the shaded jar) is the right JDK-21+ fix, but it's a behavior change beyond the requested version bump, so I'll leave it for the maintainer to decide rather than silently expand scope.
| # Eclipse, Netbeans and IntelliJ files | ||
| /.* | ||
| !.gitignore | ||
| !.github |
There was a problem hiding this comment.
📝 Info: .gitignore negation for .github verified to work despite the broad /.* rule
I verified locally that git check-ignore -v .github/workflows/build.yml reports no match, so the !.github negation placed after /.* does correctly un-ignore the new workflow directory (last matching pattern wins for the directory itself, and git then descends into it). Note the negation is unanchored, so it also un-ignores any nested .github directory — harmless here.
Was this helpful? React with 👍 or 👎 to provide feedback.
| <configuration> | ||
| <source>1.9</source> | ||
| <target>1.9</target> | ||
| <release>17</release> |
There was a problem hiding this comment.
📝 Info: Source compiles under --release 17 with no code changes (confirmed)
I compiled all sources with javac --release 17 -encoding UTF-8 -proc:none locally; the only failures were the missing JMH dependency imports and (when the encoding flag is omitted) non-ASCII characters in a comment in src/main/java/lambdasinaction/chap5/PuttingIntoPractice.java:54. The latter is covered by project.build.sourceEncoding=UTF-8, so the claim that no source changes are needed holds. The single JUnit-using class lives in src/main (lambdasinaction/chap10/ReadPositiveIntParam.java), which is why the compile-scope junit dependency is required and why the newly pinned surefire plugin currently has no tests to run.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Superseded by the Java 17 fan-out tracked on Devin board UNT52 (https://app.devin.ai/boards/board-9cb8fbbff9e04d81be8174384344309f); a fresh Java 17 PR will replace this one. |
Summary
Java 9 → Java 17 upgrade. Purely a build modernization — no source changes were needed; all imports are
java.util/java.util.stream/ JMH and nothing hit removed APIs under--release 17.pom.xml:<properties>: addmaven.compiler.release=173.1→3.13.0; replace<source>1.9</source><target>1.9</target>with<release>17</release>3.6.2(previously resolved whatever was newest on Central — non-reproducible; Maven 3.9.6 also warns on version-less plugins)pluginManagement: pin maven-surefire-plugin3.5.6(so any futuremvn testworks on 17)1.17.4→1.37(old annotation processor fails on modern JDKs)4.11→4.13.2Also added (both optional in the plan):
.github/workflows/build.yml— the repo had no CI: Temurin 17,mvn -B clean package, plus ajava -jar target/benchmarks.jar -lsmoke step..gitignore: ignore shade's generateddependency-reduced-pom.xml; add!.githubnegation (the existing/.*rule would otherwise ignore the new workflow dir).Verification — passed
Toolchain: Temurin/OpenJDK 17.0.13, Maven 3.9.6.
mvn -B clean package→ BUILD SUCCESS, no compilation errors. Only shade's usual "overlapping resources" warnings (LICENSE,THIRD-PARTY,META-INF/MANIFEST.MF). Confirms JMH 1.37 annotation processing works under compiler-plugin 3.13.0.java -jar target/benchmarks.jar -l→ shaded JMH uber-jar (org.openjdk.jmh.Mainentry point) builds and lists all 5 benchmarks:Per plan convention: this repo has no test suite, so "verified" = compile + package + smoke (
-l) only. Benchmarks were listed, not executed (ParallelStreamBenchmarkhard-codes@Fork(jvmArgs={"-Xms4G","-Xmx4G"})), so runtime benchmark measurement semantics under JMH 1.37 are unverified. The new CI workflow's first run is on this PR.Environment note
Maven Central returned HTTP 429 (IP rate limit) from the build box, so the build used the read-only GCS mirror of Central via a session-local
~/.m2/settings.xml. That is not part of the diff.Note
A near-identical prior run already opened #10 (branch
java-17-upgrade, CI green). This PR is the independently-verified deliverable with the requested title/branch; #10 can be closed as a duplicate.Link to Devin session: https://app.devin.ai/sessions/a9cac9a7127043c2bb86552f57b3f1c5
Requested by: @tobydrinkall
Devin Review