build: clear Gradle 10 deprecations, bump wrapper to 9.7.1 - #5362
build: clear Gradle 10 deprecations, bump wrapper to 9.7.1#5362soloturn wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe build configuration now uses dependency notation strings, explicit subproject paths, root-project publishing properties, direct version provider values, and a standalone script rewriter. The Gradle wrapper updates to version 9.7.1. ChangesBuild tooling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change updates Gradle compatibility and build configuration, with the supplied build and validation checks passing; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
92432a8 to
ed47b61
Compare
Addresses review confusion on #5362: the bare `it` in subprojects.forEach { api(project(it.path)) } reads ambiguously and was mistaken for a self-dependency. Verified via 'gradlew :modules:dependencies --configuration api' both before and after - each of the 54 subprojects is declared exactly once, no self-reference to :modules. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Gradle reports 100 deprecation warnings "making it incompatible with Gradle 10". Three of the sites are ours, and between them account for 61 of the 100: - modules/build.gradle.kts passed the Project object itself to api(), once per subproject. Using a Project as dependency notation fails with an error in Gradle 10; it now depends on project(path). - GradleDependencyInfo.asMap() produced the group:/name:/version: map form, which terasology-module.gradle.kts applies to every module. Multi-string notation also fails in Gradle 10, so it now produces single-string "group:module:version". Version ranges are unaffected - they contain no colons, so the coordinate still splits unambiguously. - config/gradle/publish.gradle read publishRepo/mavenUser/mavenPass as bare names, an implicit lookup in a parent project. Those sit inside hasProperty guards, so they would have broken publishing, not building. Every remaining warning traces into libs/gestalt or libs/TeraNUI, which are independent repos reached through includeBuild and need their own PRs. The module-attributed ones were never the module repos' fault: every modules/*/build.gradle is byte-identical to templates/build.gradle and only applies terasology-module, so those were this repo's build-logic reported once per project. Not addressed: the protoc coordinate warning attributed to :engine comes from protobuf-gradle-plugin 0.9.4's own ToolsLocator, which tokenizes the single-string coordinate we give it and rebuilds it as a map. engine/build.gradle.kts is already correct; that needs a plugin upgrade. Seeing those warnings at all means running 9.7.1, which this build could not do. Gradle 9.7.1 ships Kotlin 2.4, which enforces Transformer's declared OUT : Any bound. ContentFilterable.filter takes Transformer<String?, String> - the OUT is nullable because returning null there drops the line - so naming that type is now a compile error (KTLC-358), and facades/PC named it twice: implementing the interface, and casting to it. The net effect is that Gradle's own API is not implementable from the Kotlin that Gradle ships with. Passing a lambda instead lets SAM conversion supply the type argument, so the script never names it; ScriptClasspathRewriter keeps its logic and never returned null anyway. Only distributionUrl moves. The wrapper jar and gradlew scripts are left alone - they are just the bootstrapper, and the existing ones drive 9.7.1 fine, so regenerating them would be diff noise. Staying on -bin too: the wrapper task would have switched it to -all, making every CI run additionally download sources and docs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
engine/build.gradle.kts and engine-tests/build.gradle.kts each called .toString() on libs.versions.protobuf.get(), which already returns String - kotlinc flagged both as a redundant conversion. Both also cast JsonSlurper().parseText()'s Any! result straight to Map<String, String> to read module.txt; kotlinc can't verify that at runtime, hence the unchecked-cast warning. Suppressed rather than restructured - module.txt's shape is fixed and this mirrors how the Kotlin stdlib itself documents suppressing verified-safe erasure casts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses review confusion on #5362: the bare `it` in subprojects.forEach { api(project(it.path)) } reads ambiguously and was mistaken for a self-dependency. Verified via 'gradlew :modules:dependencies --configuration api' both before and after - each of the 54 subprojects is declared exactly once, no self-reference to :modules. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ded3e8b to
1a70dae
Compare
Summary
Gradle 10 deprecations — 100 warnings before, 39 after
modules/build.gradle.ktspassed theProjectobject itself toapi(), once per subproject. Using aProjectas dependency notation fails with an error in Gradle 10; it now depends onproject(path).GradleDependencyInfo.asMap()produced thegroup:/name:/version:map form, whichterasology-module.gradle.ktsapplies to every module. Multi-string notation also fails in Gradle 10, so it now produces single-string"group:module:version". Version ranges are unaffected — they contain no colons, so the coordinate still splits unambiguously.config/gradle/publish.gradlereadpublishRepo/mavenUser/mavenPassas bare names, an implicit lookup in a parent project. These sit insidehasPropertyguards, so they would have broken publishing rather than building.libs/gestaltorlibs/TeraNUI. Those are independent repos reached viaincludeBuildand need their own PRs; nothing here can fix them. The module-attributed warnings were never the module repos' fault either — everymodules/*/build.gradleis byte-identical totemplates/build.gradleand only appliesterasology-module, so those were this repo's build-logic reported once per project.:engineis emitted inside protobuf-gradle-plugin 0.9.4's ownToolsLocator, which tokenizes the single-string coordinate we hand it and re-assembles it as a map.engine/build.gradle.kts:157is already correct single-string notation; that one needs a plugin upgrade, which belongs in its own change.Getting to a Gradle that reports them
Transformer's declaredOUT : Anybound.ContentFilterable.filtertakesTransformer<String?, String>— theOUTis nullable because returning null there drops the line — so naming that type is a compile error (KTLC-358), andfacades/PC/build.gradle.ktsnamed it twice: implementing the interface, and casting to it. The net effect is that Gradle's own API is not implementable from the Kotlin that Gradle ships with. Passing a lambda instead lets SAM conversion supply the type argument, so the script never names it;ScriptClasspathRewriterkeeps its logic and never returned null anyway.distributionUrlmoves. The wrapper jar andgradlewscripts are left untouched — they are just the bootstrapper, and the existing ones drive 9.7.1 fine (verified), so regenerating them would be pure diff noise. Staying on-bintoo: thewrappertask would have switched it to-all, making every CI run additionally download sources and docs.Test plan
All run through the repo's existing (unmodified)
gradlew, which picks up 9.7.1 from the bumpeddistributionUrl, on JDK 21../gradlew compileJava— whole build configures and compiles, includingfacades/PCand all 55 modules. This is the step that fails outright before the change under Gradle 9.7.1../gradlew unitTest— passes across all modules../gradlew :engine:check :engine-tests:check :facades:PC:check -x test— checkstyle, PMD and SpotBugs clean; only the pre-existing findings in test sources, unchanged../gradlew :facades:PC:testDistForLauncher— passes. This is the behavioural check for the Kotlin change: it builds the launcher distribution and asserts the rewrittenlib/→libs/paths, which is exactly whatScriptClasspathRewriterproduces../gradlew :modules:reportModuleOrder— all 55 modules still resolve and topologically sort. This is the behavioural check for theasMap()→ single-string change, since it exercises the dependency graph that change feeds../gradlew --warning-mode all help— 100 Gradle deprecation warnings before, 39 after, none of the remainder in files this repo tracks.Related