Repository navigation
Upgrade Gradle Build Compatibility for Gradle 9 and JDK 25 - #40810
alisha-1000 wants to merge 1 commit into
Conversation
github-actions
Bot
added
python
java
go
build
infra
examples
io
extensions
runners
learning
spark
sql
dataflow
aws
clickhouse
gcp
kafka
core
flink
fn-execution
zetasketch
kotlin
spanner
bigtable
iceberg
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR modernizes Gradle build scripts across the repository by migrating deprecated task APIs (project.exec, archivePath, archivesBaseName, mainClassName, fileMode, etc.) and upgrading several build plugins and tools (Checker Framework, Error Prone, SpotBugs, Protobuf, Google Java Format, and Kotlin).
🔍 General Feedback
- When converting ad-hoc
doLast { exec { ... } }tasks to typedExectasks (withtasks.register<Exec>), closures passed toregisterrun at task configuration time rather than task execution time. Several migrated tasks inlearning/tour-of-beamandplaygroundhave broken task dependencies or premature property evaluations as a result. - In
BeamModulePlugin.groovy, the newcom.google.protobuf:protobuf-gradle-plugin:0.9.6changes the default directory layout for generated protobuf/gRPC Java sources compared to0.8.13, which should be verified against the hardcodedgeneratedProto*paths.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
| tasks.register("generateCode") { | ||
| dependsOn(":playground:frontend:playground_components:generateCode") | ||
|
|
||
| tasks.register<org.gradle.api.tasks.Exec>("generateCode") { |
There was a problem hiding this comment.
🟠 High: dependsOn(":playground:frontend:playground_components:generateCode") was removed from generateCode, and no ordering constraint (mustRunAfter) exists between cleanFlutter and pubGet. Without the dependency on playground_components:generateCode and ordering pubGet after cleanFlutter, running generateCode can fail due to missing generated component files or race between cleaning and fetching packages.
| "" | ||
| } | ||
|
|
||
| val projectId = project.rootProject.extra["playground_gke_project"] |
There was a problem hiding this comment.
🔴 Critical: Because applyMigrations is now a typed Exec task, its configuration block executes during the configuration phase—before setPlaygroundGkeProject (or takeConfig) has executed its doLast action to populate project.rootProject.extra["playground_gke_project"]. Accessing project.rootProject.extra["playground_gke_project"] here will throw an UnknownExtraPropertyException whenever applyMigrations is configured (e.g., when running gkebackend). Additionally, applyMigrations does not declare dependsOn("setPlaygroundGkeProject"). Consider keeping this as a doLast block using providers.exec or using an CommandLineArgumentProvider coupled with dependsOn("setPlaygroundGkeProject").
| "run", | ||
| "cmd/remove_unused_snippets/remove_unused_snippets.go", | ||
| "cleanup", | ||
| "-day_diff", System.getProperty("dayDiff"), |
There was a problem hiding this comment.
🟠 High: Moving args(...) with System.getProperty(...) out of doLast into the Exec task configuration block causes System.getProperty(...) to be evaluated when the task is configured rather than only when removeUnusedSnippet (and similarly removeSnippet below on line 73) is executed. If any task in the project realizes all tasks (or during IDE sync) without -DdayDiff, -DprojectId, and -Dnamespace set, args(...) will receive null values or fail. Use argumentProviders.add(CommandLineArgumentProvider { ... }) or providers.exec inside doLast.
| val base = file(directory) | ||
| if (base.exists()) { | ||
| fileTree(base).files.filter { | ||
| regex.matches(it.relativeTo(base).invariantSeparatorsPath) |
There was a problem hiding this comment.
🟡 Medium: Previously, find assets lib test -regex ... matched against paths prefixed with the directory name (e.g. lib/foo.g.dart). Using it.relativeTo(base) strips the directory prefix (producing foo.g.dart), and println(it) now prints absolute File paths instead of relative paths. While the current patterns start with .*, using it.relativeTo(projectDir).invariantSeparatorsPath would preserve the original regex matching semantics and output format.
| regex.matches(it.relativeTo(base).invariantSeparatorsPath) | |
| regex.matches(it.relativeTo(projectDir).invariantSeparatorsPath) |
| ) | ||
| standardOutput = FileOutputStream("playground/frontend/playground_components/assets/symbols/go.g.yaml") | ||
| } | ||
| layout.projectDirectory.file("assets/symbols/go.g.yaml") |
There was a problem hiding this comment.
🟠 High: Previously, FileOutputStream("playground/frontend/playground_components/assets/symbols/go.g.yaml") resolved relative to the root project working directory because exec resolved stream paths from the Gradle daemon's CWD, whereas layout.projectDirectory.file("assets/symbols/go.g.yaml") resolves properly inside playground_components. However, neither extractBeamSymbolsGo nor extractBeamSymbolsPython declares dependsOn("ensureSymbolsDirectoryExists") (only extractBeamSymbols depends on all of them in parallel without ordering), so writeBytes will throw FileNotFoundException if assets/symbols does not yet exist when extractBeamSymbolsGo or extractBeamSymbolsPython finishes before ensureSymbolsDirectoryExists.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40810 +/- ##
==========================================
- Coverage 80.04% 79.90% -0.15%
==========================================
Files 490 493 +3
Lines 82916 83473 +557
==========================================
+ Hits 66373 66697 +324
- Misses 16543 16776 +233
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
/gemini review |
|
man is gemini stuck or something? |
Summary
This PR updates Apache Beam's Gradle build configuration to improve compatibility with Gradle 9 and JDK 25.
Changes
Validation
help) succeeded on JDK 25.Known limitation
On Apple Silicon, the
protoc-gen-grpc-java1.69.0 artifact downloaded for theosx-aarch_64classifier was identified as an x86_64 executable, preventing a fresh local protobuf generation run without a workaround. This remains to be investigated independently.Further CI validation is needed across the affected modules.