Fix Gradle 9.7 incompatibilities and remove dead pre-8.11 shims - #375
Fix Gradle 9.7 incompatibilities and remove dead pre-8.11 shims#375lmb439 wants to merge 3 commits into
Conversation
|
Awaiting review and approval from repo Maintainers |
There was a problem hiding this comment.
Hi @lmb439, your PR and time is very much appreciated.
I've tagged a couple of places where comments explaining a change should be removed, please. Comments tend to just get out of date quickly. People can look at git history / PRs for why something was changed.
Otherwise, the fix looks good AFAIK.
Couple of points on building / releasing this change:
Target JDK
I believe the binaries are built targeting JDK 11, but because Gradle 9.x requires JDK 17, tests are run at JDK 17.
In terms of fixing Gradle 9.7 support this PR adds, it seems we don't need to update the JDK, right? I guess the build will prove this...
Bumping Gradle wrapper
As you mention in your description #372 currently fails and you're suggesting we merge this PR first, then rebase #372, which will then fully exercise code paths.
If possible, I'd prefer for this PR to also bump the gradle version using in the build, so that this PRs build fully tests all code paths.
Are you OK to pull in those changes?
Tasks:
- Bump Gradle wrapper.
The plugin fails against Gradle 9 in two distinct ways. 1. AbstractMethodError on Gradle 9.7+ (user-facing) ModularCreateStartScripts.getMainClassName() called super.getMainClass(), which javac compiles to invokespecial. CreateStartScripts.getMainClass() became abstract after 9.2.0 -- Gradle supplies the body only in the decorated subclass it generates -- so invokespecial has no body to enter and throws AbstractMethodError, breaking installDist/start scripts. Calling getMainClass() emits invokevirtual, which resolves the declaration and dispatches to the decorated implementation on every supported version. 2. Missing covariant bridge method for setMain On Gradle 8.x, JavaExecSpec.setMain(String):JavaExecSpec and JavaExec.setMain(String):JavaExec cause javac to synthesise a bridge in ModularJavaExec carrying the interface's exact descriptor. Gradle 9 removed the deprecated getMain/setMain, so the override bridges nothing. A plugin compiled against Gradle 9 and run on 8.x therefore loses that bridge: a Groovy DSL `main = "module/Class"` assignment, resolved dynamically through the JavaExecSpec contract, lands on Gradle's setMain instead of the plugin's, stripModule() never runs, and AbstractExecutionMutator prefixes the module a second time, producing "Could not find or load main class module/Class in module module". Kotlin projects are unaffected because their `main =` compiles to a static call on the declared method. Stripping the prefix where it is consumed makes the result independent of which setter ran. Also in this change: - Remove dead legacy branches guarded below the declared 8.11 floor. They are unreachable, but javac type-checks every branch, so the removed JavaPluginConvention, Project.getConvention(), AbstractCompile.getDestinationDir() and JavaCompile.setDestinationDir() APIs broke compilation against Gradle 9 regardless. - Declare ModularJavaExec and ModularCreateStartScripts abstract. Gradle 9 keeps adding abstract @Inject getters to these task types, and the missing getter changes between minors (getJavaModuleDetector() on 9.0.0, getGitRef() on 9.7.1), so implementing them by name is a treadmill. Gradle's decorator supplies them. Nothing instantiates these types directly. - Add @DisableCachingByDefault, matching the parent task types. Gradle 9 turns the missing annotation into a validatePlugins error. - Keep gradleApi() off the TestKit plugin classpath. gradle-api-9.7.1.jar contains classes at major version 69, which Gradle < 9.2 cannot instrument. The target Gradle supplies its own API. javaparser reached TestKit only as a passenger inside gradleApi(), so the classpath manifest now includes configurations.plugin explicitly. - Add Gradle 9.7.1 to the smoke-test matrix. Without it this class of regression is invisible. Verified: 120 tests, 0 failures, 19/19 on each of Gradle 8.11, 8.14.3, 9.0, 9.2.0 and 9.7.1, both with the 8.11 wrapper and with the wrapper bumped to 9.7.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bumps [gradle-wrapper](https://github.com/gradle/gradle) from 8.11 to 9.7.1. - [Release notes](https://github.com/gradle/gradle/releases) - [Commits](gradle/gradle@v8.11.0...v9.7.1) --- updated-dependencies: - dependency-name: gradle-wrapper dependency-version: 9.7.1 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Address review feedback: comments explaining why the code is the way it is go stale, so the reasoning moves into test names and assertion messages instead. AbstractExecutionMutatorTest covers the module-qualification contract, including the case the removed comment described: a mainClass that already carries a "module/" prefix must not be qualified a second time. Reverting the stripModule() call fails that test and only that test. ModulePluginSmokeTest asserts the two plugin-classpath invariants the build.gradle comment described -- the Gradle API stays off it, javaparser stays on it -- so a regression there reports itself instead of surfacing as "Unsupported class file major version 69" across every smoke test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c114ddc to
7935300
Compare
|
Incorporated Review suggestions (tests instead of ad-hoc comments) and cherrypicked Dependabot's bump into this PR for full code path coverage on updated 9.7 compatible build |
What breaks
Two distinct failures against Gradle 9:
AbstractMethodErrorand theinstallDist/ start-scripts path fails. This is not covered by any open issue — Please add Gradle 9 support #299 was closed as "v2.0.0 supports Gradle 9", which holds only through 9.2.0.gradleApi()(7 errors on 9.0.0, 8 on 9.7.1), which is what Upgrade build to Gradle 9 #351 describes and why Bump gradle-wrapper from 8.11 to 9.7.1 #372 is red.Root cause
1.
invokespecialinto a now-abstract method.ModularCreateStartScripts.getMainClassName()calledsuper.getMainClass(), which javac compiles toinvokespecial.CreateStartScripts.getMainClass()became abstract after 9.2.0 — Gradle supplies the body only in the decorated subclass it generates — so there is no body to enter. CallinggetMainClass()emitsinvokevirtual, which resolves the declaration and dispatches to the decorated implementation on every supported version.2. A missing covariant bridge method for
setMain. On Gradle 8.x,JavaExecSpec.setMain(String):JavaExecSpecandJavaExec.setMain(String):JavaExeccause javac to synthesise a bridge inModularJavaExeccarrying the interface's exact descriptor. Gradle 9 removed the deprecatedgetMain/setMain, so the override bridges nothing. A plugin compiled against Gradle 9 but run on 8.x therefore loses that bridge: a Groovy DSLmain = "module/Class"assignment — resolved dynamically through theJavaExecSpeccontract — lands on Gradle'ssetMaininstead of the plugin's,stripModule()never runs, andAbstractExecutionMutatorprefixes the module a second time, producingCould not find or load main class module/Class in module module. Kotlin projects are unaffected because theirmain =compiles to a static call on the declared method.This second one is why simply bumping the wrapper is not enough: it only appears once the plugin is compiled against 9.x and then run against the 8.x floor, which no existing test covered.
Changes
ModularCreateStartScripts— dropsuper.;AbstractExecutionMutator— strip the module prefix where it is consumed, so the result no longer depends on which setter ran.JavaPluginConvention,Project.getConvention(),AbstractCompile.getDestinationDir(),JavaCompile.setDestinationDir()). They are unreachable, but javac type-checks every branch, so removed APIs broke compilation regardless.ModularJavaExec/ModularCreateStartScriptsabstract. Gradle 9 keeps adding abstract@Injectgetters and the missing one changes between minors (getJavaModuleDetector()on 9.0.0,getGitRef()on 9.7.1), so implementing them by name is a treadmill; Gradle's decorator supplies them. Nothing instantiates these types directly.@DisableCachingByDefault, matching the parent task types — Gradle 9 turns the missing annotation into avalidatePluginserror.gradleApi()off the TestKit plugin classpath (compileOnly).gradle-api-9.7.1.jarcontains classes at major version 69, which Gradle < 9.2 cannot instrument. javaparser previously reached TestKit only as a passenger insidegradleApi(), so the classpath manifest now listsconfigurations.pluginexplicitly.Verification
./gradlew build(the CI command) on JDK 17: 120 tests, 0 failures, 1 pre-existing@Disabledskip.Run green both with the wrapper at 8.11 (as in this PR) and with it bumped to 9.7.1, so the 8.11 floor is preserved in both configurations. Published
shadowJarre-checked: onlyorg/javamodularity, class file majors 52/55.Merge order
This PR deliberately does not touch the wrapper — #372 owns those files.
@dependabot rebaseon Bump gradle-wrapper from 8.11 to 9.7.1 #372 — GitHub does not re-runpull_requestchecks when the base branch moves, so Bump gradle-wrapper from 8.11 to 9.7.1 #372 needs an explicit rebase to go green.Refs #351. Refs #299. Unblocks #372.
Not included
build.gradlehascompile.extendsFrom plugin, butcompilewas removed in Gradle 7, so that line is dead: the plugin compiles against javaparser-core 3.27.1 bundled inside Gradle's API whileshadowJarships the declared 3.28.2. Fix iscompileOnly.extendsFrom plugin(notimplementation, which would publish javaparser into the POM beside the shaded copy). Happy to raise separately.2.0.0 | 8.11.+ -> 9.+row is accurate only through 9.2.0.ModularJavaExec.execFixEffectiveArguments()is gated onGradleVersion < 6.4and unreachable at the 8.11 floor; it is the only remaining user ofjoorand of reflection intoorg.gradle.process.internal.🤖 Generated with Claude Code